GrResourceCache2 manages scratch texture.

GrResourceCache2 manages scratch texture.


BUG=skia:2889
Committed: https://skia.googlesource.com/skia/+/bcf0a52d4f4221b158e68a06ba0c4cc4db011060

[bsalomon, 2014-09-26 22:10:41]

bsalomon@google.com changed reviewers:
+ robertphillips@google.com

[bsalomon, 2014-09-26 22:10:41]

Not polished and ready for a thorough review yet, but this passes all unit tests
and gms.

[bsalomon, 2014-09-28 16:06:03]

Now that I've deleted a lot more code, I think this is ready for review.

Checking the CQ just to trigger all the bots.

[bsalomon, 2014-09-28 16:06:23]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-09-28 16:07:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608883003/180001

[commit-bot: I haz the power, 2014-09-28 16:07:49]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-28 22:07 UTC

[commit-bot: I haz the power, 2014-09-28 16:08:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-28 16:08:16]

Failed to apply patch for include/gpu/GrContext.h:
While running git apply --index -3 -p1;
  error: patch failed: include/gpu/GrContext.h:1103
  error: repository lacks the necessary blob to fall back on 3-way merge.
  error: include/gpu/GrContext.h: patch does not apply

Patch:       include/gpu/GrContext.h
Index: include/gpu/GrContext.h
diff --git a/include/gpu/GrContext.h b/include/gpu/GrContext.h
index
e1af73ccb2497c9cbea64b248aa5b9b6bf5c1c2f..3256e886b09659b22eab5d31e4adc82990cfb1f2
100644
--- a/include/gpu/GrContext.h
+++ b/include/gpu/GrContext.h
@@ -295,12 +295,6 @@ public:
     GrTexture* lockAndRefScratchTexture(const GrTextureDesc&, ScratchTexMatch
match);
 
     /**
-     *  When done with an entry, call unlockScratchTexture(entry) on it, which
returns
-     *  it to the cache, where it may be purged. This does not unref the
texture.
-     */
-    void unlockScratchTexture(GrTexture* texture);
-
-    /**
      * Creates a texture that is outside the cache. Does not count against
      * cache's budget.
      */
@@ -1051,15 +1045,7 @@ private:
                                     size_t rowBytes,
                                     bool filter);
 
-    // Needed so GrTexture's returnToCache helper function can call
-    // addExistingTextureToCache
-    friend class GrTexture;
-    friend class GrStencilAndCoverPathRenderer;
-    friend class GrStencilAndCoverTextContext;
-
-    // Add an existing texture to the texture cache. This is intended solely
-    // for use with textures released from an GrAutoScratchTexture.
-    void addExistingTextureToCache(GrTexture* texture);
+    bool addNewScratchTexture(const GrTextureDesc& desc);
 
     /**
      * These functions create premul <-> unpremul effects if it is possible to
generate a pair
@@ -1079,8 +1065,7 @@ private:
 };
 
 /**
- * Gets and locks a scratch texture from a descriptor using either exact or
approximate criteria.
- * Unlocks texture in the destructor.
+ * This is deprecated. Don't use it.
  */
 class GrAutoScratchTexture : public ::SkNoncopyable {
 public:
@@ -1103,25 +1088,16 @@ public:
 
     void reset() {
         if (fContext && fTexture) {
-            fContext->unlockScratchTexture(fTexture);
             fTexture->unref();
             fTexture = NULL;
         }
     }
 
-    /*
-     * When detaching a texture we do not unlock it in the texture cache but
-     * we do set the returnToCache flag. In this way the texture remains
-     * "locked" in the texture cache until it is freed and recycled in
-     * GrTexture::internal_dispose. In reality, the texture has been removed
-     * from the cache (because this is in AutoScratchTexture) and by not
-     * calling unlockScratchTexture we simply don't re-add it. It will be
-     * reattached in GrTexture::internal_dispose.
-     *
-     * Note that the caller is assumed to accept and manage the ref to the
-     * returned texture.
-     */
-    GrTexture* detach();
+    GrTexture* detach() {
+        GrTexture* texture = fTexture;
+        fTexture = NULL;
+        return texture;
+    }
 
     GrTexture* set(GrContext* context,
                    const GrTextureDesc& desc,

[bsalomon, 2014-09-28 18:17:01]

Oops, forgot that is dependent on https://codereview.chromium.org/596053002/
landing first.

[robertphillips, 2014-09-29 15:25:08]

https://codereview.chromium.org/608883003/diff/180001/gm/texturedomaineffect.cpp
File gm/texturedomaineffect.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/gm/texturedomaineffect....
gm/texturedomaineffect.cpp:95: SkAutoTUnref<GrTexture>
texture(GrRefCachedBitmapTexture(context, fBmp, NULL));
NULL == texture ?

https://codereview.chromium.org/608883003/diff/180001/gm/yuvtorgbeffect.cpp
File gm/yuvtorgbeffect.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/gm/yuvtorgbeffect.cpp#n...
gm/yuvtorgbeffect.cpp:90: 
Isn't "NULL ==" still the preferred syntax ?

https://codereview.chromium.org/608883003/diff/180001/include/gpu/GrGpuResour...
File include/gpu/GrGpuResource.h (right):

https://codereview.chromium.org/608883003/diff/180001/include/gpu/GrGpuResour...
include/gpu/GrGpuResource.h:118: 
typo

https://codereview.chromium.org/608883003/diff/180001/include/gpu/GrGpuResour...
include/gpu/GrGpuResource.h:122: kYes_IsScratch
Could fIsScratch be const for the time being?

https://codereview.chromium.org/608883003/diff/180001/src/core/SkBitmapProcSh...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/core/SkBitmapProcSh...
src/core/SkBitmapProcShader.cpp:463: 
NULL == ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:442: 
createNewScratchTexture ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:444: SkAutoTUnref<GrTexture>
texture(fGpu->createTexture(desc, NULL, 0));
NULL == texture ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:469: if (!this->addNewScratchTexture(*desc)) {
return false; -> return NULL; ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:508: if (!this->addNewScratchTexture(*desc)) {
return false -> return NULL ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:512: 
pull both "if (!this->addNewScratchTexture ... " blocks here ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache.cpp:172: this->attachToHead(entry);
rm this return; ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache.h:149: 
/**
 * Notify the cache that the given resource has been touched.
 */

?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache.h:158: *
rm this part of the comment ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache2.h
File src/gpu/GrResourceCache2.h (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache2.h:43: 
Does this need to be out here ?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:78: 
// If 'bitmap' is GrTexture backed, return the backing GrTexture. Otherwise,
// create a scratch texture based copy.

?

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:371: 
At some point it seems this should be "GrGetBackingTexture" and just leaving the
reffing to the client.

[bsalomon, 2014-09-29 19:58:14]

https://codereview.chromium.org/608883003/diff/180001/gm/texturedomaineffect.cpp
File gm/texturedomaineffect.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/gm/texturedomaineffect....
gm/texturedomaineffect.cpp:96: if (!texture) {
It's an AutoTUnref, not a ptr, and implictly casts to bool. I'd have to do "NULL
== texture.get()". I don't think that's an improvement.

https://codereview.chromium.org/608883003/diff/180001/include/gpu/GrGpuResour...
File include/gpu/GrGpuResource.h (right):

https://codereview.chromium.org/608883003/diff/180001/include/gpu/GrGpuResour...
include/gpu/GrGpuResource.h:118: 
On 2014/09/29 15:25:07, robertphillips wrote:
> typo

Done.

https://codereview.chromium.org/608883003/diff/180001/include/gpu/GrGpuResour...
include/gpu/GrGpuResource.h:122: kYes_IsScratch
On 2014/09/29 15:25:07, robertphillips wrote:
> Could fIsScratch be const for the time being?

It'd have to be passed through a whole lot of layers to make it to the
constructor.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:442: 
On 2014/09/29 15:25:07, robertphillips wrote:
> createNewScratchTexture ?

sure

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:469: if (!this->addNewScratchTexture(*desc)) {
On 2014/09/29 15:25:07, robertphillips wrote:
> return false; -> return NULL; ?

Done.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:508: if (!this->addNewScratchTexture(*desc)) {
On 2014/09/29 15:25:07, robertphillips wrote:
> return false -> return NULL ?

Done.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:512: 
On 2014/09/29 15:25:07, robertphillips wrote:
> pull both "if (!this->addNewScratchTexture ... " blocks here ?

Done.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache.cpp:172: this->attachToHead(entry);
On 2014/09/29 15:25:07, robertphillips wrote:
> rm this return; ?

Done.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache.h
File src/gpu/GrResourceCache.h (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache.h:149: 
On 2014/09/29 15:25:08, robertphillips wrote:
> /**
>  * Notify the cache that the given resource has been touched.
>  */
> 
> ?

It's pretty self-explanatory and this method and class are on their death beds.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache2.h
File src/gpu/GrResourceCache2.h (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache2.h:43: 
On 2014/09/29 15:25:08, robertphillips wrote:
> Does this need to be out here ?

Needs to access fIsScratch.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:78: 
On 2014/09/29 15:25:08, robertphillips wrote:
> // If 'bitmap' is GrTexture backed, return the backing GrTexture. Otherwise,
> // create a scratch texture based copy.
> 
> ?

Done.

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/608883003/diff/180001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:371: 
On 2014/09/29 15:25:08, robertphillips wrote:
> At some point it seems this should be "GrGetBackingTexture" and just leaving
the
> reffing to the client.

Hm... most of our methods across Skia that return a ref-cnt'ed object pre-ref
it. It feels dangerous to me to have getters to cache objects that haven't
already ref'ed the object. The caller has to be extremely careful to not do
something that could purge the object before a ref is taken. Also, If we ever
want GrRC to be thread-safe then we have to have already ref'ed it.

[bsalomon, 2014-09-30 01:58:42]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-09-30 01:59:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608883003/260001

[commit-bot: I haz the power, 2014-09-30 01:59:23]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-09-30 07:59 UTC

[bsalomon, 2014-09-30 02:08:28]

The CQ bit was unchecked by bsalomon@google.com

[robertphillips, 2014-09-30 13:01:18]

lgtm

[robertphillips, 2014-09-30 13:01:25]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-09-30 13:01:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608883003/260001

[commit-bot: I haz the power, 2014-09-30 13:02:28]

Committed patchset #14 (id:260001) as 3d398c876440deaab39bbf2a9b881c337e6dc8d4

[robertphillips, 2014-09-30 13:53:31]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/611383003/ by robertphillips@google.com.

The reason for reverting is: Turning bots red:
Nanobench seems to be uniformly failing on Android
(http://108.170.220.21:10117/builders/Perf-Android-Venue8-PowerVR-x86-Release/...)

Ubuntu GTX660 32bit is failing in both Debug and Release on GM generation (it
appears to be out of memory)
(http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Debug...)
.

[bsalomon, 2014-10-07 04:07:55]

Rob, I figured out why this was regressing. The rule that only extant regular
refs block scratch recycling is not valid for the clip mask case since it makes
scratch requests at flush time rather than while queueing draws. In the delta
from patchset 16 to 18 you can see this hack. My plan is to target moving the
clipping from GrGpu to GrDT after this CL.

https://codereview.chromium.org/608883003/diff/330001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/608883003/diff/330001/include/gpu/GrContext.h...
include/gpu/GrContext.h:293: * mode. (This may be addressed in a future
update.)7
will remove the stray 7 and fix up sentence in subsequent comment before
landing.

[bsalomon, 2014-10-07 04:07:57]

Rob, I figured out why this was regressing. The rule that only extant regular
refs block scratch recycling is not valid for the clip mask case since it makes
scratch requests at flush time rather than while queueing draws. In the delta
from patchset 16 to 18 you can see this hack. My plan is to target moving the
clipping from GrGpu to GrDT after this CL.

[robertphillips, 2014-10-07 13:59:35]

lgtm + some nits

https://codereview.chromium.org/608883003/diff/330001/gm/yuvtorgbeffect.cpp
File gm/yuvtorgbeffect.cpp (right):

https://codereview.chromium.org/608883003/diff/330001/gm/yuvtorgbeffect.cpp#n...
gm/yuvtorgbeffect.cpp:101: SkRect renderRect =
SkRect::MakeWH(SkIntToScalar(fBmp[0].width()),
extra ' ' here ?

https://codereview.chromium.org/608883003/diff/330001/src/gpu/GrResourceCache...
File src/gpu/GrResourceCache2.cpp (right):

https://codereview.chromium.org/608883003/diff/330001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache2.cpp:65: if (fFlags) {
f;ush ?

https://codereview.chromium.org/608883003/diff/330001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache2.cpp:67: // case no refs, either by drawing code or for
pending io operations, are allowed.
Flags -> fFlags ?

https://codereview.chromium.org/608883003/diff/330001/src/image/SkSurface_Gpu...
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/608883003/diff/330001/src/image/SkSurface_Gpu...
src/image/SkSurface_Gpu.cpp:35: 
overlength ?

[bsalomon, 2014-10-07 14:13:36]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-10-07 14:14:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608883003/370001

[bsalomon, 2014-10-07 14:14:48]

The CQ bit was unchecked by bsalomon@google.com

[bsalomon, 2014-10-07 14:20:02]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-10-07 14:20:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608883003/390001

[bsalomon, 2014-10-07 14:20:14]

https://codereview.chromium.org/608883003/diff/330001/src/gpu/GrResourceCache...
File src/gpu/GrResourceCache2.cpp (right):

https://codereview.chromium.org/608883003/diff/330001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache2.cpp:65: if (fFlags) {
On 2014/10/07 13:59:34, robertphillips wrote:
> f;ush ?

Rewrote the comment. Changed from uint32_t fFlags to bool fFlushing

https://codereview.chromium.org/608883003/diff/330001/src/gpu/GrResourceCache...
src/gpu/GrResourceCache2.cpp:67: // case no refs, either by drawing code or for
pending io operations, are allowed.
On 2014/10/07 13:59:34, robertphillips wrote:
> Flags -> fFlags ?

done (comment rewritten).

https://codereview.chromium.org/608883003/diff/330001/src/image/SkSurface_Gpu...
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/608883003/diff/330001/src/image/SkSurface_Gpu...
src/image/SkSurface_Gpu.cpp:35: 
On 2014/10/07 13:59:34, robertphillips wrote:
> overlength ?

Done.

[commit-bot: I haz the power, 2014-10-07 14:27:12]

Committed patchset #21 (id:390001) as d14e1a27643125bfef37fa0ed314b64c1fae22b7

[bsalomon, 2014-10-07 19:02:42]

A revert of this CL (patchset #21 id:390001) has been created in
https://codereview.chromium.org/634043003/ by bsalomon@google.com.

The reason for reverting is: oom on android generate gms.

[bsalomon, 2014-10-08 15:31:47]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-10-08 15:32:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608883003/430001

[commit-bot: I haz the power, 2014-10-08 15:40:15]

Committed patchset #23 (id:430001) as bcf0a52d4f4221b158e68a06ba0c4cc4db011060