AutoScratchTexture can now release its texture and it will return to the texture cache when freed

[robertphillips, 2012-05-30 14:22:38]

http://codereview.appspot.com/6262043/diff/1/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6262043/diff/1/include/gpu/GrContext.h#newcode214
include/gpu/GrContext.h:214: 
Could make this protected but that would require even more "friends"

http://codereview.appspot.com/6262043/diff/1/include/gpu/GrContext.h#newcode803
include/gpu/GrContext.h:803: 
Could make addExistingTextureToCache public to remove GrTexture friending

http://codereview.appspot.com/6262043/diff/1/include/gpu/GrContext.h#newcode894
include/gpu/GrContext.h:894: */
So this is a bit wasteful in that it frees the cache entry and then reallocs it
when the texture is reattached. We could add a system to the cache where it
holds on to the freed entries and recycles them but I don't think it will be a
big performance hit

http://codereview.appspot.com/6262043/diff/1/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

http://codereview.appspot.com/6262043/diff/1/include/gpu/GrTypes.h#newcode466
include/gpu/GrTypes.h:466: uint32_t               fFlags;  //!< bitfield of
TextureFlags
The reconstruction of the descriptor in GrTexture::returnToCache gets messier if
this is an enum

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

http://codereview.appspot.com/6262043/diff/1/src/gpu/GrSoftwarePathRenderer.c...
src/gpu/GrSoftwarePathRenderer.cpp:315: translate, &ast, antiAlias)) {
I will not deliver this file but it shows how the AST's release method will be
used to prevent texture "reuse". I can't deliver this until the drawState
reffing textures & render targets gets delivered

[reed1, 2012-05-30 14:31:57]

adding ben to review the change to SkRefCnt.h

[robertphillips, 2012-05-30 15:33:23]

Abandoned in favor of different approach

[robertphillips, 2012-06-13 20:59:31]

PTAL - this version is a bit simpler then the prior one since we have moved more
of the key generation/cache knowledge into the GrTexture.

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrSoftwarePathRendere...
File src/gpu/GrSoftwarePathRenderer.cpp (right):

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrSoftwarePathRendere...
src/gpu/GrSoftwarePathRenderer.cpp:312: translate, &ast, antiAlias)) {
This file is just an example of how it would be used. Not ready for delivery
until GrDrawState is reffing/unreffing its textures.

[robertphillips, 2012-06-13 21:07:26]

The improvements are mainly in GrContext's addExistingTextureToCache method
which no longer takes a descriptor (since that is in the texture) and doesn't
call gen_scratch_tex_key_values directly (since that is wrapped by
GrTexture::ComputeKey) and GrTexture's internal_dispose method which no longer
needs a "returnToCache" helper function since it doesn't need to recreate the
texture's descriptor.

[bsalomon, 2012-06-13 21:49:52]

I hope this is leading to eventually killing TextureCacheEntry and
GrAutoScratchTexture (and hiding making GrResourceEntry a private in
GrResourceCache).

Let's hold off until after the M21 branch since this is touching code that is
already very complicated and fragile.

http://codereview.appspot.com/6262043/diff/6001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6262043/diff/6001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:863: GrTexture* release() {
Can we call this detach() or something that indicates that no one else can get
the GrTexture* (until its ref==0).

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

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrContext.cpp#newcode509
src/gpu/GrContext.cpp:509: return fTextureCache->createAndLock(key, texture);
I think we leak a lock here unless the caller unlocks and
GrTexture::internal_dispose isn't unlocking.

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrSoftwarePathRendere...
File src/gpu/GrSoftwarePathRenderer.cpp (right):

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrSoftwarePathRendere...
src/gpu/GrSoftwarePathRenderer.cpp:316: GrTexture* texture = ast.release();
SkAutoTUnref<GrTexture> texture(ast.release());

[robertphillips, 2012-06-14 13:52:45]

I believe we have the same vision for the texture cache system. I have started a
design doc to collect our thoughts.

http://codereview.appspot.com/6262043/diff/6001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6262043/diff/6001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:863: GrTexture* release() {
On 2012/06/13 21:49:53, bsalomon wrote:
> Can we call this detach() or something that indicates that no one else can get
> the GrTexture* (until its ref==0). 

Done.

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

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrContext.cpp#newcode509
src/gpu/GrContext.cpp:509: return fTextureCache->createAndLock(key, texture);
Done.

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrSoftwarePathRendere...
File src/gpu/GrSoftwarePathRenderer.cpp (right):

http://codereview.appspot.com/6262043/diff/6001/src/gpu/GrSoftwarePathRendere...
src/gpu/GrSoftwarePathRenderer.cpp:316: GrTexture* texture = ast.release();
On 2012/06/13 21:49:53, bsalomon wrote:
> SkAutoTUnref<GrTexture> texture(ast.release());

Done.

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.cpp#n...
src/gpu/GrResourceCache.cpp:208: 
Although this is almost the same as createAndLock I thought it was worth having
a new entry point given where we are going with this class (i.e., this method
doesn't return a GrResourceEntry + adding a behavior flag to createAndLock
would've been odd)

[bsalomon, 2012-06-18 12:39:49]

Mostly LGTM, minor comments inline.

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.cpp#n...
src/gpu/GrResourceCache.cpp:208: 
On 2012/06/14 13:52:45, robertphillips wrote:
> Although this is almost the same as createAndLock I thought it was worth
having
> a new entry point given where we are going with this class (i.e., this method
> doesn't return a GrResourceEntry + adding a behavior flag to createAndLock
> would've been odd)

I think you're correct given that we will want to stop returning
GrResourceEntries in the future. But could we put the common guts in an private
function?

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.h#new...
src/gpu/GrResourceCache.h:252: *  Ownership of the resource is transferred to
the Entry, which will unref()
I think this comment is stale (both here and at least the name of the type in
createAndLock). Perhaps we should say "... transferred to the cache"

[robertphillips, 2012-06-18 15:24:11]

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.cpp#n...
src/gpu/GrResourceCache.cpp:208: 
On 2012/06/18 12:39:49, bsalomon wrote:
> On 2012/06/14 13:52:45, robertphillips wrote:
> > Although this is almost the same as createAndLock I thought it was worth
> having
> > a new entry point given where we are going with this class (i.e., this
method
> > doesn't return a GrResourceEntry + adding a behavior flag to createAndLock
> > would've been odd)
> 
> I think you're correct given that we will want to stop returning
> GrResourceEntries in the future. But could we put the common guts in an
private
> function?

Done.

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

http://codereview.appspot.com/6262043/diff/6003/src/gpu/GrResourceCache.h#new...
src/gpu/GrResourceCache.h:252: *  Ownership of the resource is transferred to
the Entry, which will unref()
On 2012/06/18 12:39:49, bsalomon wrote:
> I think this comment is stale (both here and at least the name of the type in
> createAndLock). Perhaps we should say "... transferred to the cache"

Done.

[bsalomon, 2012-06-18 15:30:15]

LGTM

[robertphillips, 2012-06-22 12:42:07]

committed as r4301