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>
diff --git a/include/private/GrSurfaceProxy.h b/include/private/GrSurfaceProxy.h
index 9462626..d69a294 100644
--- a/include/private/GrSurfaceProxy.h
+++ b/include/private/GrSurfaceProxy.h
@@ -95,7 +95,6 @@
#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 @@
fTarget->fPendingWrites += fPendingWrites;
}
+ int32_t internalGetProxyRefCnt() const {
+ return fRefCnt;
+ }
+
bool internalHasPendingIO() const {
if (fTarget) {
return fTarget->internalHasPendingIO();
@@ -409,6 +412,10 @@
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 d389507..d253ca2 100644
--- a/src/gpu/GrResourceAllocator.cpp
+++ b/src/gpu/GrResourceAllocator.cpp
@@ -138,7 +138,7 @@
}
// '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 @@
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 @@
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 @@
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 @@
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 @@
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 e71e355..f383ee4 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 @@
void markEndOfOpList(int opListIndex);
+#if GR_ALLOCATION_SPEW
+ void dumpIntervals();
+#endif
+
private:
class Interval;
@@ -81,7 +88,7 @@
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 8d7d8d0..a5113f2 100644
--- a/src/gpu/GrSurfaceProxyPriv.h
+++ b/src/gpu/GrSurfaceProxyPriv.h
@@ -37,6 +37,12 @@
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 8263615..1d0e70c 100644
--- a/tests/ProcessorTest.cpp
+++ b/tests/ProcessorTest.cpp
@@ -199,7 +199,7 @@
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 6a7dfbd..8161827 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 51ecdb7..852fe5a 100644
--- a/tests/ResourceAllocatorTest.cpp
+++ b/tests/ResourceAllocatorTest.cpp
@@ -30,7 +30,7 @@
// 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 @@
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 @@
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 @@
// 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 @@
// 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 @@
};
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 @@
};
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 @@
};
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);
}