Set texture dirty flag only when texture has really changed

[bsalomon, 2011-06-14 20:36:02]

I tested this in DRT, no failures introduced. Also visited a bunch of canvas
sites and didn't see any issues.

[reed1, 2011-06-14 20:42:37]

LGTM

[bsalomon, 2011-06-15 14:01:06]

Checked in at r1592

[junov1, 2011-06-15 15:25:16]

http://codereview.appspot.com/4571073/diff/1/gpu/src/GrGpuGL.cpp
File gpu/src/GrGpuGL.cpp (right):

http://codereview.appspot.com/4571073/diff/1/gpu/src/GrGpuGL.cpp#newcode1783
gpu/src/GrGpuGL.cpp:1783: fDirtyFlags.fTextureChangedMask |= (1 << s);
To me this looks like a potential recipe for a bug.  I think the above 'if'
condition should perform a deeper comparison. Or perhaps GrGLTexture should have
its own dirty flag.

[bsalomon, 2011-06-15 15:54:55]

http://codereview.appspot.com/4571073/diff/1/gpu/src/GrGpuGL.cpp
File gpu/src/GrGpuGL.cpp (right):

http://codereview.appspot.com/4571073/diff/1/gpu/src/GrGpuGL.cpp#newcode1783
gpu/src/GrGpuGL.cpp:1783: fDirtyFlags.fTextureChangedMask |= (1 << s);
On 2011/06/15 15:25:16, junov1 wrote:
> To me this looks like a potential recipe for a bug.  I think the above 'if'
> condition should perform a deeper comparison. Or perhaps GrGLTexture should
have
> its own dirty flag.

I'm not sure I understand. The dirty flag exists to indicate to later parts of
the flush that there is a newly bound texture. Later code (e.g. texture matrix
flush) whose GL state is dependent on aspects of the bound texture such as
width/height know they need to resend.

[junov1, 2011-06-15 18:22:45]

My point is that checking that the GrGLTexture* (the address) is the same does
not actually prove that the texture is the same. We are not using ref counting
here so it is possible for the old texture pointer to be stale, and the new
texture pointer might be a different texture that coincidentally landed at the
same memory location.

[junov1, 2011-06-15 18:39:10]

I guess that what I am trying to say is that for this code to be truly safe,
fHWDrawState would need to store its own copy of the texture attributes we care
about (that affect the shader).  Or GrGLTexture could have a dirty flag to
indicate whether it is synchronized with the hardware state, and we could check
that flag, rather than perform a comparison on the texture object.

[bsalomon, 2011-06-15 18:40:37]

I see, gotcha. We actually call GrGpuGL::notifyTextureDelete() when a
texture is deleted and NULL out any entries in fHWDrawState that point to
it.

On Wed, Jun 15, 2011 at 2:22 PM, <junov@chromium.org> wrote:

> My point is that checking that the GrGLTexture* (the address) is the
> same does not actually prove that the texture is the same. We are not
> using ref counting here so it is possible for the old texture pointer to
> be stale, and the new texture pointer might be a different texture that
> coincidentally landed at the same memory location.
>
>
>
http://codereview.appspot.com/**4571073/<http://codereview.appspot.com/4571073/>
>

[junov1, 2011-06-15 18:49:56]

Ah, then LGTM