Move clientID into texture desc

[bsalomon, 2012-06-05 15:48:36]

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

http://codereview.appspot.com/6305044/diff/1/include/gpu/GrContext.h#newcode151
include/gpu/GrContext.h:151: const GrSamplerState* sampler) const;
Why all the adding of param names?

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

http://codereview.appspot.com/6305044/diff/1/include/gpu/GrTypes.h#newcode486
include/gpu/GrTypes.h:486: * A user-provided texture ID. It should be unique to
the texture data
^and (at end of the line)?

http://codereview.appspot.com/6305044/diff/1/include/gpu/GrTypes.h#newcode490
include/gpu/GrTypes.h:490: uint64_t               fClientID;
Let's document that this only matters for cached textures and use the word cache
in the name (e.g. fClientCacheID or even fCacheID). I say this because we have a
createUncachedTexture function on GrContext which I assume will ignore this.

We probably should have a default constructor for this struct that initializes
all the fields. I just checked and WebKit never uses static initializer syntax
with this struct. It would mean we have to change all the current internal users
of static intializier syntax, but it also means we don't have to constantly
remember to set it to something.

It'd be good to define a static const value that is reserved for scratch
textures (probably 0) and note what it means.

I think eventually we should get rid of the separate entry points on GrContext
that deal with scratch textures and this field alone will indicate the intended
use. Should we have a special value for uncached textures (say -1)? Maybe it
would be helpful if we want uncached textures to enter the cache upon deletion
if there is available space. Also, I think we will want to consider satisfying
uncached texture requests by removing a texture from the cache (and bumping the
available budget).

[robertphillips, 2012-06-06 18:37:04]

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

http://codereview.appspot.com/6305044/diff/1/include/gpu/GrContext.h#newcode151
include/gpu/GrContext.h:151: const GrSamplerState* sampler) const;
All the other ones have them?

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

http://codereview.appspot.com/6305044/diff/1/include/gpu/GrTypes.h#newcode486
include/gpu/GrTypes.h:486: * A user-provided texture ID. It should be unique to
the texture data
On 2012/06/05 15:48:36, bsalomon wrote:
> ^and (at end of the line)?
> 

Done.

http://codereview.appspot.com/6305044/diff/1/include/gpu/GrTypes.h#newcode490
include/gpu/GrTypes.h:490: uint64_t               fClientID;
docs - done
renaming - done
default ctor - done
static scratch const - done
static uncached const - done

One problem w/ using just this flag to indicate caching is that we sometimes get
key's from the client for cached textures that we then throw away.

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

http://codereview.appspot.com/6305044/diff/6001/src/gpu/GrContext.cpp#newcode436
src/gpu/GrContext.cpp:436: ScratchTexMatch match) {
Not GrAssert(gScratch == ...) because sometimes have a real ClientCacheID (which
we discard later)

http://codereview.appspot.com/6305044/diff/6001/src/gpu/GrContext.cpp#newcode454
src/gpu/GrContext.cpp:454: do {
Here is where we used to discard the ClientCacheID

http://codereview.appspot.com/6305044/diff/6001/src/gpu/GrContext.cpp#newcode487
src/gpu/GrContext.cpp:487: if (NULL != texture) {
Here too

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

http://codereview.appspot.com/6305044/diff/6001/src/gpu/GrTexture.cpp#newcode121
src/gpu/GrTexture.cpp:121: clientKey = (desc.fFlags << 8) | ((uint64_t)
desc.fConfig << 32);
Here is where we now discard the user-provided fClientCacheID

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

http://codereview.appspot.com/6305044/diff/6001/src/gpu/SkGr.cpp#newcode93
src/gpu/SkGr.cpp:93: 
Brian - it seems like flipping the equality test here makes sense but could you
double check the logic in lockCachedTexture to be sure?

[robertphillips, 2012-06-06 21:01:47]

Much better.

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

http://codereview.appspot.com/6305044/diff/8002/src/gpu/GrContext.cpp#newcode436
src/gpu/GrContext.cpp:436: ScratchTexMatch match) {
Here is where we discard now

http://codereview.appspot.com/6305044/diff/8002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/6305044/diff/8002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1840: } else {
How about using kScratch_CacheID here?

http://codereview.appspot.com/6305044/diff/8002/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

http://codereview.appspot.com/6305044/diff/8002/src/gpu/SkGr.cpp#newcode93
src/gpu/SkGr.cpp:93: 
kScratch_CacheID here?

http://codereview.appspot.com/6305044/diff/8002/src/gpu/SkGr.cpp#newcode113
src/gpu/SkGr.cpp:113: desc.fConfig =
SkGr::BitmapConfig2PixelConfig(bitmap->config());
kScratch_CacheID here?

[bsalomon, 2012-06-06 21:53:35]

LGTM

[robertphillips, 2012-06-07 12:09:15]

committed as r4201