Fix GrBackendTextureImageGenerator to hold context lock till all proxies are gone

This fixes a bug where if we created two proxies for the same context. We would
release the context lock after one of the proxies was released instead of
waiting for all proxies to be released.

Bug: skia:
Change-Id: Ia6ed8148abb029bd1f95c85bc3d3ef003e8de408
Reviewed-on: https://skia-review.googlesource.com/102322
Commit-Queue: Greg Daniel <egdaniel@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>

src/gpu/GrBackendTextureImageGenerator.cpp

@@ -80,6 +80,7 @@ void GrBackendTextureImageGenerator::ReleaseRefHelper_TextureReleaseProc(void* c
     SkASSERT(refHelper);
 
     refHelper->fBorrowedTexture = nullptr;
+    refHelper->fBorrowingContextReleaseProc = nullptr;
     refHelper->fBorrowingContextID = SK_InvalidGenID;
     refHelper->unref();
 }
@@ -95,13 +96,28 @@ sk_sp<GrTextureProxy> GrBackendTextureImageGenerator::onGenerateTexture(
 
     auto proxyProvider = context->contextPriv().proxyProvider();
 
-    uint32_t expectedID = SK_InvalidGenID;
+    fBorrowingMutex.acquire();
-    if (!fRefHelper->fBorrowingContextID.compare_exchange(&expectedID, context->uniqueID())) {
+    sk_sp<GrReleaseProcHelper> releaseProcHelper;
+    if (SK_InvalidGenID != fRefHelper->fBorrowingContextID) {
         if (fRefHelper->fBorrowingContextID != context->uniqueID()) {
-            // Some other context is currently borrowing the texture. We aren't allowed to use it.
+            fBorrowingMutex.release();
             return nullptr;
+        } else {
+            SkASSERT(fRefHelper->fBorrowingContextReleaseProc);
+            // Ref the release proc to be held by the proxy we make below
+            releaseProcHelper = sk_ref_sp(fRefHelper->fBorrowingContextReleaseProc);
         }
+    } else {
+        SkASSERT(!fRefHelper->fBorrowingContextReleaseProc);
+        // The ref we add to fRefHelper here will be passed into and owned by the
+        // GrReleaseProcHelper.
+        fRefHelper->ref();
+        releaseProcHelper.reset(new GrReleaseProcHelper(ReleaseRefHelper_TextureReleaseProc,
+                                                        fRefHelper));
+        fRefHelper->fBorrowingContextReleaseProc = releaseProcHelper.get();
     }
+    fRefHelper->fBorrowingContextID = context->uniqueID();
+    fBorrowingMutex.release();
 
     SkASSERT(fRefHelper->fBorrowingContextID == context->uniqueID());
 
@@ -117,16 +133,11 @@ sk_sp<GrTextureProxy> GrBackendTextureImageGenerator::onGenerateTexture(
     sk_sp<GrSemaphore> semaphore = fSemaphore;
     GrBackendTexture backendTexture = fBackendTexture;
     RefHelper* refHelper = fRefHelper;
-    refHelper->ref();
 
     sk_sp<GrTextureProxy> proxy = proxyProvider->createLazyProxy(
-            [refHelper, semaphore, backendTexture]
+            [refHelper, releaseProcHelper, semaphore, backendTexture]
             (GrResourceProvider* resourceProvider, GrSurfaceOrigin* /*outOrigin*/) {
                 if (!resourceProvider) {
-                    // If we get here then we never created a texture to pass the refHelper ref off
-                    // to. Thus we must unref it ourselves.
-                    refHelper->fBorrowingContextID = SK_InvalidGenID;
-                    refHelper->unref();
                     return sk_sp<GrTexture>();
                 }
 
@@ -142,10 +153,6 @@ sk_sp<GrTextureProxy> GrBackendTextureImageGenerator::onGenerateTexture(
                     // previously created.
                     tex = sk_ref_sp(refHelper->fBorrowedTexture);
                     SkASSERT(tex);
-                    // The texture is holding onto a ref to the refHelper so since we have a ref to
-                    // the texture we don't need to hold a ref to the refHelper. This unref's the
-                    // ref we grabbed on refHelper in the lambda capture for this proxy.
-                    refHelper->unref();
                 } else {
                     // We just gained access to the texture. If we're on the original context, we
                     // could use the original texture, but we'd have no way of detecting that it's
@@ -156,21 +163,11 @@ sk_sp<GrTextureProxy> GrBackendTextureImageGenerator::onGenerateTexture(
                     tex = resourceProvider->wrapBackendTexture(backendTexture,
                                                                kBorrow_GrWrapOwnership);
                     if (!tex) {
-                        refHelper->fBorrowingContextID = SK_InvalidGenID;
-                        refHelper->unref();
                         return sk_sp<GrTexture>();
                     }
                     refHelper->fBorrowedTexture = tex.get();
 
-                    sk_sp<GrReleaseProcHelper> releaseHelper(
+                    tex->setRelease(releaseProcHelper);
-                           new GrReleaseProcHelper(ReleaseRefHelper_TextureReleaseProc, refHelper));
-
-                    // By setting this release proc on the texture we are passing our ref on the
-                    // refHelper to the texture.
-                    // DDL TODO: Once we are start reusing Lazy Proxies, we need to add a ref to the
-                    // refHelper here, we'll still move the releaseHelper though since the texture
-                    // will be the only one ever calling that release proc.
-                    tex->setRelease(std::move(releaseHelper));
                 }
 
                 return tex;

src/gpu/GrBackendTextureImageGenerator.h

@@ -10,16 +10,21 @@
 #include "SkImageGenerator.h"
 
 #include "GrBackendSurface.h"
-#include "SkAtomics.h"
+#include "SkMutex.h"
 
 class GrSemaphore;
 
 /*
  * This ImageGenerator is used to wrap a texture in one GrContext and can then be used as a source
- * in another GrContext. It holds onto a semaphore which the producing Grontext will signal and the
+ * in another GrContext. It holds onto a semaphore which the producing GrContext will signal and the
  * consuming GrContext will wait on before using the texture. Only one GrContext can ever be used
  * as a consumer (this is mostly because Vulkan can't allow multiple things to wait on the same
  * semaphore).
+ *
+ * In practice, this capability is used by clients to create backend-specific texture resources in
+ * one thread (with, say, GrContext-A) and then ship them over to another GrContext (say,
+ * GrContext-B) which will then use the texture as a source for draws. GrContext-A uses the
+ * semaphore to notify GrContext-B when the shared texture is ready to use.
  */
 class GrBackendTextureImageGenerator : public SkImageGenerator {
 public:
@@ -54,27 +59,38 @@ private:
             : fOriginalTexture(texture)
             , fOwningContextID(owningContextID)
             , fBorrowedTexture(nullptr)
+            , fBorrowingContextReleaseProc(nullptr)
             , fBorrowingContextID(SK_InvalidGenID) {}
 
         ~RefHelper();
 
-        GrTexture* fOriginalTexture;
+        GrTexture*          fOriginalTexture;
-        uint32_t fOwningContextID;
+        uint32_t            fOwningContextID;
 
         // There is never a ref associated with this pointer. We rely on our atomic bookkeeping
         // with the context ID to know when this pointer is valid and safe to use. This lets us
         // avoid releasing a ref from another thread, or get into races during context shutdown.
-        GrTexture* fBorrowedTexture;
+        GrTexture*           fBorrowedTexture;
-        SkAtomic<uint32_t> fBorrowingContextID;
+        // For the same reason as the fBorrowedTexture, there is no ref associated with this
+        // pointer. The fBorrowingContextReleaseProc is used to make sure all uses of the wrapped
+        // texture are finished on the borrowing context before we open this back up to other
+        // contexts. In general a ref to this release proc is owned by all proxies and gpu uses of
+        // the backend texture.
+        GrReleaseProcHelper* fBorrowingContextReleaseProc;
+        uint32_t             fBorrowingContextID;
     };
 
-    RefHelper* fRefHelper;
+    RefHelper*           fRefHelper;
+    // This Mutex is used to guard the borrowing of the texture to one GrContext at a time as well
+    // as the creation of the fBorrowingContextReleaseProc. The latter happening if two threads with
+    // the same consuming GrContext try to generate a texture at the same time.
+    SkMutex              fBorrowingMutex;
 
-    sk_sp<GrSemaphore> fSemaphore;
+    sk_sp<GrSemaphore>   fSemaphore;
 
-    GrBackendTexture fBackendTexture;
+    GrBackendTexture     fBackendTexture;
-    GrPixelConfig fConfig;
+    GrPixelConfig        fConfig;
-    GrSurfaceOrigin fSurfaceOrigin;
+    GrSurfaceOrigin      fSurfaceOrigin;
 
     typedef SkImageGenerator INHERITED;
 };

tests/ImageTest.cpp

@@ -891,8 +891,18 @@ static void test_cross_context_image(skiatest::Reporter* reporter, const GrConte
                     ctx, GrSamplerState::ClampNearest(), nullptr, &texColorSpace, nullptr);
             REPORTER_ASSERT(reporter, proxySecondRef);
 
-            // Releae all refs from the original context
+            // Release first ref from the original context
             proxy.reset(nullptr);
+
+            // We released one proxy but not the other from the current borrowing context. Make sure
+            // a new context is still not able to borrow the texture.
+            otherTestContext->makeCurrent();
+            otherProxy = as_IB(refImg)->asTextureProxyRef(otherCtx, GrSamplerState::ClampNearest(),
+                                                          nullptr, &texColorSpace, nullptr);
+            REPORTER_ASSERT(reporter, !otherProxy);
+
+            // Release second ref from the original context
+            testContext->makeCurrent();
             proxySecondRef.reset(nullptr);
 
             // Now we should be able to borrow the texture from the other context