Fix explicit allocation bug

Change-Id: I9866f563e02b2ab290cc46ede05f8eda21f6d3b2
Reviewed-on: https://skia-review.googlesource.com/142163
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>

7 files changed, 152 insertions, 41 deletions

diff --git a/include/private/GrSurfaceProxy.h b/include/private/GrSurfaceProxy.h
index 94626265ac..d69a294194 100644
--- a/include/private/GrSurfaceProxy.h
+++ b/include/private/GrSurfaceProxy.h
@@ -95,7 +95,6 @@ public:
 #endif
     }
 
-    int32_t getProxyRefCnt_TestOnly() const;
     int32_t getBackingRefCnt_TestOnly() const;
     int32_t getPendingReadCnt_TestOnly() const;
     int32_t getPendingWriteCnt_TestOnly() const;
@@ -164,6 +163,10 @@ protected:
         fTarget->fPendingWrites += fPendingWrites;
     }
 
+    int32_t internalGetProxyRefCnt() const {
+        return fRefCnt;
+    }
+
     bool internalHasPendingIO() const {
         if (fTarget) {
             return fTarget->internalHasPendingIO();
@@ -409,6 +412,10 @@ protected:
     friend class GrSurfaceProxyPriv;
 
     // Methods made available via GrSurfaceProxyPriv
+    int32_t getProxyRefCnt() const {
+        return this->internalGetProxyRefCnt();
+    }
+
     bool hasPendingIO() const {
         return this->internalHasPendingIO();
     }
diff --git a/src/gpu/GrResourceAllocator.cpp b/src/gpu/GrResourceAllocator.cpp
index d3895073aa..d253ca2c1a 100644
--- a/src/gpu/GrResourceAllocator.cpp
+++ b/src/gpu/GrResourceAllocator.cpp
@@ -138,7 +138,7 @@ void GrResourceAllocator::IntervalList::insertByIncreasingEnd(Interval* intvl) {
 }
 
 // 'surface' can be reused. Add it back to the free pool.
-void GrResourceAllocator::freeUpSurface(sk_sp<GrSurface> surface) {
+void GrResourceAllocator::recycleSurface(sk_sp<GrSurface> surface) {
     const GrScratchKey &key = surface->resourcePriv().getScratchKey();
 
     if (!key.isValid()) {
@@ -152,6 +152,9 @@ void GrResourceAllocator::freeUpSurface(sk_sp<GrSurface> surface) {
         return;
     }
 
+#if GR_ALLOCATION_SPEW
+    SkDebugf("putting surface %d back into pool\n", surface->uniqueID().asUInt());
+#endif
     // TODO: fix this insertion so we get a more LRU-ish behavior
     fFreePool.insert(key, surface.release());
 }
@@ -192,10 +195,18 @@ void GrResourceAllocator::expire(unsigned int curIndex) {
         Interval* temp = fActiveIntvls.popHead();
 
         if (temp->wasAssignedSurface()) {
-            this->freeUpSurface(temp->detachSurface());
+            sk_sp<GrSurface> surface = temp->detachSurface();
+
+            // If the proxy has an actual live ref on it that means someone wants to retain its
+            // contents. In that case we cannot recycle it (until the external holder lets
+            // go of it).
+            if (0 == temp->proxy()->priv().getProxyRefCnt()) {
+                this->recycleSurface(std::move(surface));
+            }
         }
 
         // Add temp to the free interval list so it can be reused
+        SkASSERT(!temp->wasAssignedSurface()); // it had better not have a ref on a surface
         temp->setNext(fFreeIntervalList);
         fFreeIntervalList = temp;
     }
@@ -222,6 +233,9 @@ bool GrResourceAllocator::assign(int* startIndex, int* stopIndex,
 
     SkDEBUGCODE(fAssigned = true;)
 
+#if GR_ALLOCATION_SPEW
+    this->dumpIntervals();
+#endif
     while (Interval* cur = fIntvlList.popHead()) {
         if (fEndOfOpListOpIndices[fCurOpListIndex] < cur->start()) {
             fCurOpListIndex++;
@@ -269,6 +283,12 @@ bool GrResourceAllocator::assign(int* startIndex, int* stopIndex,
                 SkASSERT(surface->getUniqueKey() == tex->getUniqueKey());
             }
 
+#if GR_ALLOCATION_SPEW
+            SkDebugf("Assigning %d to %d\n",
+                 surface->uniqueID().asUInt(),
+                 cur->proxy()->uniqueID().asUInt());
+#endif
+
             cur->assign(std::move(surface));
         } else {
             SkASSERT(!cur->proxy()->priv().isInstantiated());
@@ -291,3 +311,42 @@ bool GrResourceAllocator::assign(int* startIndex, int* stopIndex,
     this->expire(std::numeric_limits<unsigned int>::max());
     return true;
 }
+
+#if GR_ALLOCATION_SPEW
+void GrResourceAllocator::dumpIntervals() {
+
+    // Print all the intervals while computing their range
+    unsigned int min = fNumOps+1;
+    unsigned int max = 0;
+    for(const Interval* cur = fIntvlList.peekHead(); cur; cur = cur->next()) {
+        SkDebugf("{ %3d,%3d }: [%2d, %2d] - proxyRefs:%d surfaceRefs:%d R:%d W:%d\n",
+                 cur->proxy()->uniqueID().asUInt(),
+                 cur->proxy()->priv().isInstantiated() ? cur->proxy()->underlyingUniqueID().asUInt()
+                                                       : -1,
+                 cur->start(),
+                 cur->end(),
+                 cur->proxy()->priv().getProxyRefCnt(),
+                 cur->proxy()->getBackingRefCnt_TestOnly(),
+                 cur->proxy()->getPendingReadCnt_TestOnly(),
+                 cur->proxy()->getPendingWriteCnt_TestOnly());
+        min = SkTMin(min, cur->start());
+        max = SkTMax(max, cur->end());
+    }
+
+    // Draw a graph of the useage intervals
+    for(const Interval* cur = fIntvlList.peekHead(); cur; cur = cur->next()) {
+        SkDebugf("{ %3d,%3d }: ",
+                 cur->proxy()->uniqueID().asUInt(),
+                 cur->proxy()->priv().isInstantiated() ? cur->proxy()->underlyingUniqueID().asUInt()
+                                                       : -1);
+        for (unsigned int i = min; i <= max; ++i) {
+            if (i >= cur->start() && i <= cur->end()) {
+                SkDebugf("x");
+            } else {
+                SkDebugf(" ");
+            }
+        }
+        SkDebugf("\n");
+    }
+}
+#endif
diff --git a/src/gpu/GrResourceAllocator.h b/src/gpu/GrResourceAllocator.h
index e71e3558c2..f383ee4653 100644
--- a/src/gpu/GrResourceAllocator.h
+++ b/src/gpu/GrResourceAllocator.h
@@ -19,6 +19,9 @@
 class GrResourceProvider;
 class GrUninstantiateProxyTracker;
 
+// Print out explicit allocation information
+#define GR_ALLOCATION_SPEW 0
+
 /*
  * The ResourceAllocator explicitly distributes GPU resources at flush time. It operates by
  * being given the usage intervals of the various proxies. It keeps these intervals in a singly
@@ -74,6 +77,10 @@ public:
 
     void markEndOfOpList(int opListIndex);
 
+#if GR_ALLOCATION_SPEW
+    void dumpIntervals();
+#endif
+
 private:
     class Interval;
 
@@ -81,7 +88,7 @@ private:
     void expire(unsigned int curIndex);
 
     // These two methods wrap the interactions with the free pool
-    void freeUpSurface(sk_sp<GrSurface> surface);
+    void recycleSurface(sk_sp<GrSurface> surface);
     sk_sp<GrSurface> findSurfaceFor(const GrSurfaceProxy* proxy, bool needsStencil);
 
     struct FreePoolTraits {
diff --git a/src/gpu/GrSurfaceProxyPriv.h b/src/gpu/GrSurfaceProxyPriv.h
index 8d7d8d02d7..a5113f22d5 100644
--- a/src/gpu/GrSurfaceProxyPriv.h
+++ b/src/gpu/GrSurfaceProxyPriv.h
@@ -37,6 +37,12 @@ public:
         return fProxy->fTarget ? fProxy->fTarget->asRenderTarget() : nullptr;
     }
 
+    // Beware! Woe betide anyone whosoever calls this method.
+    // The refs on proxies and their backing GrSurfaces shift around based on whether the proxy
+    // is instantiated or not. Additionally, the lifetime of a proxy (and a GrSurface) also
+    // depends on the read and write refs (So this method can validly return 0).
+    int32_t getProxyRefCnt() const { return fProxy->getProxyRefCnt(); }
+
     // Beware! This call is only guaranteed to tell you if the proxy in question has
     // any pending IO in its current state. It won't tell you about the IO state in the
     // future when the proxy is actually used/instantiated.
diff --git a/tests/ProcessorTest.cpp b/tests/ProcessorTest.cpp
index 8263615115..1d0e70cef0 100644
--- a/tests/ProcessorTest.cpp
+++ b/tests/ProcessorTest.cpp
@@ -199,7 +199,7 @@ DEF_GPUTEST_FOR_ALL_CONTEXTS(ProcessorRefTest, reporter, ctxInfo) {
                 testingOnly_getIORefCnts(proxy1.get(), &refCnt, &readCnt, &writeCnt);
                 // IO counts should be double if there is a clone of the FP.
                 int ioRefMul = makeClone ? 2 : 1;
-                REPORTER_ASSERT(reporter, 1 == refCnt);
+                REPORTER_ASSERT(reporter, -1 == refCnt);
                 REPORTER_ASSERT(reporter, ioRefMul * 1 == readCnt);
                 REPORTER_ASSERT(reporter, ioRefMul * 0 == writeCnt);
 
diff --git a/tests/ProxyRefTest.cpp b/tests/ProxyRefTest.cpp
index 6a7dfbdcf0..8161827c2d 100644
--- a/tests/ProxyRefTest.cpp
+++ b/tests/ProxyRefTest.cpp
@@ -18,16 +18,12 @@
 #include "GrTexture.h"
 #include "GrTextureProxy.h"
 
-int32_t GrIORefProxy::getProxyRefCnt_TestOnly() const {
-    return fRefCnt;
-}
-
 int32_t GrIORefProxy::getBackingRefCnt_TestOnly() const {
     if (fTarget) {
         return fTarget->fRefCnt;
     }
 
-    return fRefCnt;
+    return -1; // no backing GrSurface
 }
 
 int32_t GrIORefProxy::getPendingReadCnt_TestOnly() const {
@@ -54,12 +50,12 @@ static void check_refs(skiatest::Reporter* reporter,
                        int32_t expectedBackingRefs,
                        int32_t expectedNumReads,
                        int32_t expectedNumWrites) {
-    REPORTER_ASSERT(reporter, proxy->getProxyRefCnt_TestOnly() == expectedProxyRefs);
+    REPORTER_ASSERT(reporter, proxy->priv().getProxyRefCnt() == expectedProxyRefs);
     REPORTER_ASSERT(reporter, proxy->getBackingRefCnt_TestOnly() == expectedBackingRefs);
     REPORTER_ASSERT(reporter, proxy->getPendingReadCnt_TestOnly() == expectedNumReads);
     REPORTER_ASSERT(reporter, proxy->getPendingWriteCnt_TestOnly() == expectedNumWrites);
 
-    SkASSERT(proxy->getProxyRefCnt_TestOnly() == expectedProxyRefs);
+    SkASSERT(proxy->priv().getProxyRefCnt() == expectedProxyRefs);
     SkASSERT(proxy->getBackingRefCnt_TestOnly() == expectedBackingRefs);
     SkASSERT(proxy->getPendingReadCnt_TestOnly() == expectedNumReads);
     SkASSERT(proxy->getPendingWriteCnt_TestOnly() == expectedNumWrites);
@@ -101,7 +97,9 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ProxyRefTest, reporter, ctxInfo) {
                 static const int kExpectedReads = 0;
                 static const int kExpectedWrites = 1;
 
-                check_refs(reporter, proxy.get(), 1, 1, kExpectedReads, kExpectedWrites);
+                int backingRefs = proxy->isWrapped_ForTesting() ? 1 : -1;
+
+                check_refs(reporter, proxy.get(), 1, backingRefs, kExpectedReads, kExpectedWrites);
 
                 proxy->instantiate(resourceProvider);
 
@@ -119,7 +117,9 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ProxyRefTest, reporter, ctxInfo) {
                 static const int kExpectedReads = 1;
                 static const int kExpectedWrites = 0;
 
-                check_refs(reporter, proxy.get(), 1, 1, kExpectedReads, kExpectedWrites);
+                int backingRefs = proxy->isWrapped_ForTesting() ? 1 : -1;
+
+                check_refs(reporter, proxy.get(), 1, backingRefs, kExpectedReads, kExpectedWrites);
 
                 proxy->instantiate(resourceProvider);
 
@@ -137,7 +137,9 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ProxyRefTest, reporter, ctxInfo) {
                 static const int kExpectedReads = 1;
                 static const int kExpectedWrites = 1;
 
-                check_refs(reporter, proxy.get(), 1, 1, kExpectedReads, kExpectedWrites);
+                int backingRefs = proxy->isWrapped_ForTesting() ? 1 : -1;
+
+                check_refs(reporter, proxy.get(), 1, backingRefs, kExpectedReads, kExpectedWrites);
 
                 proxy->instantiate(resourceProvider);
 
@@ -156,7 +158,9 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ProxyRefTest, reporter, ctxInfo) {
                 static const int kExpectedReads = 0;
                 static const int kExpectedWrites = 0;
 
-                check_refs(reporter, proxy.get(), 3, 3,kExpectedReads, kExpectedWrites);
+                int backingRefs = proxy->isWrapped_ForTesting() ? 3 : -1;
+
+                check_refs(reporter, proxy.get(), 3, backingRefs, kExpectedReads, kExpectedWrites);
 
                 proxy->instantiate(resourceProvider);
 
@@ -178,7 +182,9 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ProxyRefTest, reporter, ctxInfo) {
 
                 static const int kExpectedWrites = 1;
 
-                check_refs(reporter, proxy.get(), 2, 2, 0, kExpectedWrites);
+                int backingRefs = proxy->isWrapped_ForTesting() ? 2 : -1;
+
+                check_refs(reporter, proxy.get(), 2, backingRefs, 0, kExpectedWrites);
 
                 proxy->instantiate(resourceProvider);
 
diff --git a/tests/ResourceAllocatorTest.cpp b/tests/ResourceAllocatorTest.cpp
index 51ecdb7ed0..852fe5aa33 100644
--- a/tests/ResourceAllocatorTest.cpp
+++ b/tests/ResourceAllocatorTest.cpp
@@ -30,7 +30,7 @@ struct ProxyParams {
     // TODO: do we care about mipmapping
 };
 
-static sk_sp<GrSurfaceProxy> make_deferred(GrProxyProvider* proxyProvider, const ProxyParams& p) {
+static GrSurfaceProxy* make_deferred(GrProxyProvider* proxyProvider, const ProxyParams& p) {
     GrSurfaceDesc desc;
     desc.fFlags = p.fIsRT ? kRenderTarget_GrSurfaceFlag : kNone_GrSurfaceFlags;
     desc.fWidth  = p.fSize;
@@ -38,11 +38,20 @@ static sk_sp<GrSurfaceProxy> make_deferred(GrProxyProvider* proxyProvider, const
     desc.fConfig = p.fConfig;
     desc.fSampleCnt = p.fSampleCnt;
 
-    return proxyProvider->createProxy(desc, p.fOrigin, p.fFit, SkBudgeted::kNo);
+    auto tmp = proxyProvider->createProxy(desc, p.fOrigin, p.fFit, SkBudgeted::kNo);
+    if (!tmp) {
+        return nullptr;
+    }
+    GrSurfaceProxy* ret = tmp.release();
+
+    // Add a read to keep the proxy around but unref it so its backing surfaces can be recycled
+    ret->addPendingRead();
+    ret->unref();
+    return ret;
 }
 
-static sk_sp<GrSurfaceProxy> make_backend(GrContext* context, const ProxyParams& p,
-                                          GrBackendTexture* backendTex) {
+static GrSurfaceProxy* make_backend(GrContext* context, const ProxyParams& p,
+                                    GrBackendTexture* backendTex) {
     GrProxyProvider* proxyProvider = context->contextPriv().proxyProvider();
     GrGpu* gpu = context->contextPriv().getGpu();
 
@@ -50,7 +59,16 @@ static sk_sp<GrSurfaceProxy> make_backend(GrContext* context, const ProxyParams&
                                                        p.fConfig, false,
                                                        GrMipMapped::kNo);
 
-    return proxyProvider->wrapBackendTexture(*backendTex, p.fOrigin);
+    auto tmp = proxyProvider->wrapBackendTexture(*backendTex, p.fOrigin);
+    if (!tmp) {
+        return nullptr;
+    }
+    GrSurfaceProxy* ret = tmp.release();
+
+    // Add a read to keep the proxy around but unref it so its backing surfaces can be recycled
+    ret->addPendingRead();
+    ret->unref();
+    return ret;
 }
 
 static void cleanup_backend(GrContext* context, const GrBackendTexture& backendTex) {
@@ -60,12 +78,11 @@ static void cleanup_backend(GrContext* context, const GrBackendTexture& backendT
 // Basic test that two proxies with overlapping intervals and compatible descriptors are
 // assigned different GrSurfaces.
 static void overlap_test(skiatest::Reporter* reporter, GrResourceProvider* resourceProvider,
-                         sk_sp<GrSurfaceProxy> p1, sk_sp<GrSurfaceProxy> p2,
-                         bool expectedResult) {
+                         GrSurfaceProxy* p1, GrSurfaceProxy* p2, bool expectedResult) {
     GrResourceAllocator alloc(resourceProvider);
 
-    alloc.addInterval(p1.get(), 0, 4);
-    alloc.addInterval(p2.get(), 1, 2);
+    alloc.addInterval(p1, 0, 4);
+    alloc.addInterval(p2, 1, 2);
     alloc.markEndOfOpList(0);
 
     int startIndex, stopIndex;
@@ -83,12 +100,12 @@ static void overlap_test(skiatest::Reporter* reporter, GrResourceProvider* resou
 // Test various cases when two proxies do not have overlapping intervals.
 // This mainly acts as a test of the ResourceAllocator's free pool.
 static void non_overlap_test(skiatest::Reporter* reporter, GrResourceProvider* resourceProvider,
-                             sk_sp<GrSurfaceProxy> p1, sk_sp<GrSurfaceProxy> p2,
+                             GrSurfaceProxy* p1, GrSurfaceProxy* p2,
                              bool expectedResult) {
     GrResourceAllocator alloc(resourceProvider);
 
-    alloc.addInterval(p1.get(), 0, 2);
-    alloc.addInterval(p2.get(), 3, 5);
+    alloc.addInterval(p1, 0, 2);
+    alloc.addInterval(p2, 3, 5);
     alloc.markEndOfOpList(0);
 
     int startIndex, stopIndex;
@@ -149,10 +166,11 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ResourceAllocatorTest, reporter, ctxInfo) {
     };
 
     for (auto test : gOverlappingTests) {
-        sk_sp<GrSurfaceProxy> p1 = make_deferred(proxyProvider, test.fP1);
-        sk_sp<GrSurfaceProxy> p2 = make_deferred(proxyProvider, test.fP2);
-        overlap_test(reporter, resourceProvider,
-                     std::move(p1), std::move(p2), test.fExpectation);
+        GrSurfaceProxy* p1 = make_deferred(proxyProvider, test.fP1);
+        GrSurfaceProxy* p2 = make_deferred(proxyProvider, test.fP2);
+        overlap_test(reporter, resourceProvider, p1, p2, test.fExpectation);
+        p1->completedRead();
+        p2->completedRead();
     }
 
     int k2 = ctxInfo.grContext()->contextPriv().caps()->getRenderTargetSampleCount(2, kRGBA);
@@ -188,13 +206,17 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ResourceAllocatorTest, reporter, ctxInfo) {
     };
 
     for (auto test : gNonOverlappingTests) {
-        sk_sp<GrSurfaceProxy> p1 = make_deferred(proxyProvider, test.fP1);
-        sk_sp<GrSurfaceProxy> p2 = make_deferred(proxyProvider, test.fP2);
+        GrSurfaceProxy* p1 = make_deferred(proxyProvider, test.fP1);
+        GrSurfaceProxy* p2 = make_deferred(proxyProvider, test.fP2);
+
         if (!p1 || !p2) {
             continue; // creation can fail (i.e., for msaa4 on iOS)
         }
-        non_overlap_test(reporter, resourceProvider,
-                         std::move(p1), std::move(p2), test.fExpectation);
+
+        non_overlap_test(reporter, resourceProvider, p1, p2, test.fExpectation);
+
+        p1->completedRead();
+        p2->completedRead();
     }
 
     {
@@ -204,10 +226,14 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ResourceAllocatorTest, reporter, ctxInfo) {
         };
 
         GrBackendTexture backEndTex;
-        sk_sp<GrSurfaceProxy> p1 = make_backend(ctxInfo.grContext(), t[0].fP1, &backEndTex);
-        sk_sp<GrSurfaceProxy> p2 = make_deferred(proxyProvider, t[0].fP2);
-        non_overlap_test(reporter, resourceProvider,
-                         std::move(p1), std::move(p2), t[0].fExpectation);
+        GrSurfaceProxy* p1 = make_backend(ctxInfo.grContext(), t[0].fP1, &backEndTex);
+        GrSurfaceProxy* p2 = make_deferred(proxyProvider, t[0].fP2);
+
+        non_overlap_test(reporter, resourceProvider, p1, p2, t[0].fExpectation);
+
+        p1->completedRead();
+        p2->completedRead();
+
         cleanup_backend(ctxInfo.grContext(), backEndTex);
     }