Change scratch texture cache behavior to only reuse scratch textures used as render targets if they…

Change scratch texture cache behavior to only reuse scratch textures used as render targets if they will be reused as render targets.

The original behavior could sometimes confuse the driver; textures would alternate between being used as render targets and having data uploaded into them.
Committed: https://code.google.com/p/skia/source/detail?r=5252

[bsalomon, 2012-08-22 21:10:08]

http://codereview.appspot.com/6480049/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (left):

http://codereview.appspot.com/6480049/diff/1/src/gpu/GrContext.cpp#oldcode439
src/gpu/GrContext.cpp:439: } else if (desc.fFlags & kNoStencil_GrTextureFlagBit)
{
I think we want to leave this one.... but maybe change it to:

if ((desc.fFlags & kRenderTarget_GrTextureFlagBit) &&
    (desc.fFlags & kNoStencil_GrTextureFlagBit)) {

The idea here is that if the caller didn't request a stencil buffer, but we have
an RT with a stencil buffer, we give them the one with the stencil buffer.

Eventually we'll get around to lazily attaching the stencil buffer when needed
and allowing the cache to throw it away when its no longer required. But for now
we should probably leave it in.

[rileya, 2012-08-22 21:25:54]

On 2012/08/22 21:10:08, bsalomon wrote:
> http://codereview.appspot.com/6480049/diff/1/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (left):
> 
> http://codereview.appspot.com/6480049/diff/1/src/gpu/GrContext.cpp#oldcode439
> src/gpu/GrContext.cpp:439: } else if (desc.fFlags &
kNoStencil_GrTextureFlagBit)
> {
> I think we want to leave this one.... but maybe change it to:
> 
> if ((desc.fFlags & kRenderTarget_GrTextureFlagBit) &&
>     (desc.fFlags & kNoStencil_GrTextureFlagBit)) {
> 
> The idea here is that if the caller didn't request a stencil buffer, but we
have
> an RT with a stencil buffer, we give them the one with the stencil buffer.
> 
> Eventually we'll get around to lazily attaching the stencil buffer when needed
> and allowing the cache to throw it away when its no longer required. But for
now
> we should probably leave it in.

Fixed.

[bsalomon, 2012-08-22 21:28:19]

On 2012/08/22 21:25:54, rileya wrote:
> On 2012/08/22 21:10:08, bsalomon wrote:
> > http://codereview.appspot.com/6480049/diff/1/src/gpu/GrContext.cpp
> > File src/gpu/GrContext.cpp (left):
> > 
> >
http://codereview.appspot.com/6480049/diff/1/src/gpu/GrContext.cpp#oldcode439
> > src/gpu/GrContext.cpp:439: } else if (desc.fFlags &
> kNoStencil_GrTextureFlagBit)
> > {
> > I think we want to leave this one.... but maybe change it to:
> > 
> > if ((desc.fFlags & kRenderTarget_GrTextureFlagBit) &&
> >     (desc.fFlags & kNoStencil_GrTextureFlagBit)) {
> > 
> > The idea here is that if the caller didn't request a stencil buffer, but we
> have
> > an RT with a stencil buffer, we give them the one with the stencil buffer.
> > 
> > Eventually we'll get around to lazily attaching the stencil buffer when
needed
> > and allowing the cache to throw it away when its no longer required. But for
> now
> > we should probably leave it in.
> 
> Fixed.

LGTM

[robertphillips, 2012-08-23 11:04:54]

http://codereview.appspot.com/6480049/diff/5002/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6480049/diff/5002/src/gpu/GrContext.cpp#newcode436
src/gpu/GrContext.cpp:436: }
Should we even keep the kRenderTarget portion of the if test? If we removed it
this change would just boil down to removing  flexibility regarding the render
target flag.

[robertphillips, 2012-08-23 12:09:58]

Also, could you use the new texture cache stats report to see if this has any
impact on cache use?

[bsalomon, 2012-08-23 12:38:07]

http://codereview.appspot.com/6480049/diff/5002/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6480049/diff/5002/src/gpu/GrContext.cpp#newcode436
src/gpu/GrContext.cpp:436: }
On 2012/08/23 11:04:55, robertphillips wrote:
> Should we even keep the kRenderTarget portion of the if test? If we removed it
> this change would just boil down to removing  flexibility regarding the render
> target flag.

Right, that's the intent of the change. The kNoStencil_GrTextureFlagBit should
only be set if kRenderTarget_GrTextureFlagBit is set. We could assert this at
the top of the function and then just check kNoStencil here. We don't want cache
misses because a flag that has no effect (kNoStencil w/out kRT) is set.

[rileya, 2012-08-23 13:30:46]

On 2012/08/23 12:38:07, bsalomon wrote:
> http://codereview.appspot.com/6480049/diff/5002/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
http://codereview.appspot.com/6480049/diff/5002/src/gpu/GrContext.cpp#newcode436
> src/gpu/GrContext.cpp:436: }
> On 2012/08/23 11:04:55, robertphillips wrote:
> > Should we even keep the kRenderTarget portion of the if test? If we removed
it
> > this change would just boil down to removing  flexibility regarding the
render
> > target flag.
> 
> Right, that's the intent of the change. The kNoStencil_GrTextureFlagBit should
> only be set if kRenderTarget_GrTextureFlagBit is set. We could assert this at
> the top of the function and then just check kNoStencil here. We don't want
cache
> misses because a flag that has no effect (kNoStencil w/out kRT) is set.

Added an assert and a comment explaining the fix.

[robertphillips, 2012-08-23 13:50:48]

LGTM modulo comment change suggestion.

http://codereview.appspot.com/6480049/diff/10001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6480049/diff/10001/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:439: }
I think we can remove this clause "Note that unless a texture for use as a
render target was requested" and add "in situations where no RT is needed"
before the ';'.

[rileya, 2012-08-23 13:59:18]

On 2012/08/23 13:50:48, robertphillips wrote:
> LGTM modulo comment change suggestion.
> 
> http://codereview.appspot.com/6480049/diff/10001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
http://codereview.appspot.com/6480049/diff/10001/src/gpu/GrContext.cpp#newcod...
> src/gpu/GrContext.cpp:439: }
> I think we can remove this clause "Note that unless a texture for use as a
> render target was requested" and add "in situations where no RT is needed"
> before the ';'.

Done.

[rileya, 2012-08-23 14:09:10]

On 2012/08/23 13:59:18, rileya wrote:
> On 2012/08/23 13:50:48, robertphillips wrote:
> > LGTM modulo comment change suggestion.
> > 
> > http://codereview.appspot.com/6480049/diff/10001/src/gpu/GrContext.cpp
> > File src/gpu/GrContext.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6480049/diff/10001/src/gpu/GrContext.cpp#newcod...
> > src/gpu/GrContext.cpp:439: }
> > I think we can remove this clause "Note that unless a texture for use as a
> > render target was requested" and add "in situations where no RT is needed"
> > before the ';'.
> 
> Done.

And landed as 5252.