Revert of Make SkGLContext lifetime more well-defined (patchset #7 id:120001 of https...

Reason for revert:
Broke tests on Android, iOS, Mac and Windows.

Original issue's description:
> Make SkGLContext lifetime more well-defined
>
> Remove refcounting from SkGLContext.
>
> SkGLContext is expected to behave like GrContextFactory would own
> it, as implied by the GrContextFactory function.
>
> If it is refcounted, this does not hold.
>
> Also other use sites, such as in SkOSWindow_win (command buffer gl
> object), confirm the behavior. The object is explicitly owned and
> destroyed, not shared.
>
> Also fixes potential crashes from using GL context of an abandoned
> context.
>
> Also fixes potential crashes in DM/nanobench, if the GrContext lives
> longer than GLContext through internal refing of GrContext.
>
> Moves the non-trivial implementations from GrContextFactory.h to
> .cpp, just for consistency sake.
>
> Changes pathops_unittest.gyp. The pathops_unittest uses
> GrContextFactory, but did not link to its implementation. The reason
> they worked was that the implementation used (constructors, destructors)
> happened to be in the .h file.
>
> This works towards being able to use command buffer and NVPR from
> the SampleApp.
>
> BUG=skia:2992
> GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1511773005
>
> Committed: https://skia.googlesource.com/skia/+/830e012187f951d49d7e46e196ac8d1e653a25da

TBR=bsalomon@google.com,kkinnunen@nvidia.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:2992

Review URL: https://codereview.chromium.org/1555053003

diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index 827ae03..b785898 100644 (file)
--- a/bench/nanobench.cpp
+++ b/bench/nanobench.cpp
@@ -175,7 +175,7 @@ struct GPUTarget : public Target {
                                                          SkSurface::kNo_Budgeted, info,
                                                          this->config.samples, &props));
         this->gl = gGrFactory->getContextInfo(this->config.ctxType,
-                                              this->config.ctxOptions).fGLContext;
+                                              this->config.ctxOptions)->fGLContext;
         if (!this->surface.get()) {
             return false;
         }

diff --git a/dm/DM.cpp b/dm/DM.cpp
index ba8ad92..ace4907 100644 (file)
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -1147,16 +1147,16 @@ typedef void(*TestWithGrContext)(skiatest::Reporter*, GrContext*);
 typedef void(*TestWithGrContextAndGLContext)(skiatest::Reporter*, GrContext*, SkGLContext*);
 #if SK_SUPPORT_GPU
 template<typename T>
-void call_test(T test, skiatest::Reporter* reporter, const GrContextFactory::ContextInfo& context);
+void call_test(T test, skiatest::Reporter* reporter, GrContextFactory::ContextInfo* context);
 template<>
 void call_test(TestWithGrContext test, skiatest::Reporter* reporter,
-               const GrContextFactory::ContextInfo& context) {
-    test(reporter, context.fGrContext);
+               GrContextFactory::ContextInfo* context) {
+    test(reporter, context->fGrContext);
 }
 template<>
 void call_test(TestWithGrContextAndGLContext test, skiatest::Reporter* reporter,
-               const GrContextFactory::ContextInfo& context) {
-    test(reporter, context.fGrContext, context.fGLContext);
+               GrContextFactory::ContextInfo* context) {
+    test(reporter, context->fGrContext, context->fGLContext);
 }
 #endif
 } // namespace
@@ -1202,13 +1202,11 @@ void RunWithGPUTestContexts(T test, GPUTestContexts testContexts, Reporter* repo
         if ((testContexts & contextSelector) == 0) {
             continue;
         }
-        GrContextFactory::ContextInfo context = factory->getContextInfo(contextType);
-        if (context.fGrContext) {
+        if (GrContextFactory::ContextInfo* context = factory->getContextInfo(contextType)) {
             call_test(test, reporter, context);
         }
-        context = factory->getContextInfo(contextType,
-                                          GrContextFactory::kEnableNVPR_GLContextOptions);
-        if (context.fGrContext) {
+        if (GrContextFactory::ContextInfo* context =
+            factory->getContextInfo(contextType, GrContextFactory::kEnableNVPR_GLContextOptions)) {
             call_test(test, reporter, context);
         }
     }

diff --git a/gyp/pathops_unittest.gyp b/gyp/pathops_unittest.gyp
index 14b231c..9e3dbba 100644 (file)
--- a/gyp/pathops_unittest.gyp
+++ b/gyp/pathops_unittest.gyp
@@ -41,10 +41,6 @@
           'include_dirs': [
             '../src/gpu',
           ],
-         'sources': [
-            '../src/gpu/GrContextFactory.cpp',
-            '../src/gpu/GrContextFactory.h',
-          ]
         }],
       ],
     },

diff --git a/include/gpu/gl/SkGLContext.h b/include/gpu/gl/SkGLContext.h
index 7edfa73..3420a47 100644 (file)
--- a/include/gpu/gl/SkGLContext.h
+++ b/include/gpu/gl/SkGLContext.h
@@ -17,9 +17,9 @@
  * This class is intended for Skia's testing needs and not for general
  * use.
  */
-class SK_API SkGLContext : public SkNoncopyable {
+class SK_API SkGLContext : public SkRefCnt {
 public:
-    virtual ~SkGLContext();
+    ~SkGLContext() override;
 
     bool isValid() const { return NULL != gl(); }
 
@@ -106,6 +106,8 @@ private:
     SkAutoTUnref<const GrGLInterface> fGL;
 
     friend class GLFenceSync;  // For onPlatformGetProcAddress.
+
+    typedef SkRefCnt INHERITED;
 };
 
 /** Creates platform-dependent GL context object

diff --git a/src/gpu/GrContextFactory.cpp b/src/gpu/GrContextFactory.cpp
index b7e4825..4814e78 100755 (executable)
--- a/src/gpu/GrContextFactory.cpp
+++ b/src/gpu/GrContextFactory.cpp
@@ -23,56 +23,16 @@
 #include "gl/GrGLGpu.h"
 #include "GrCaps.h"
 
-GrContextFactory::GrContextFactory() { }
-
-GrContextFactory::GrContextFactory(const GrContextOptions& opts)
-    : fGlobalOptions(opts) {
-}
-
-GrContextFactory::~GrContextFactory() {
-    this->destroyContexts();
-}
-
-void GrContextFactory::destroyContexts() {
-    for (Context& context : fContexts) {
-        if (context.fGLContext) {
-            context.fGLContext->makeCurrent();
-        }
-        if (!context.fGrContext->unique()) {
-            context.fGrContext->abandonContext();
-        }
-        context.fGrContext->unref();
-        delete(context.fGLContext);
-    }
-    fContexts.reset();
-}
-
-void GrContextFactory::abandonContexts() {
-    for (Context& context : fContexts) {
-        if (context.fGLContext) {
-            context.fGLContext->makeCurrent();
-            context.fGLContext->testAbandon();
-            delete(context.fGLContext);
-            context.fGLContext = nullptr;
-        }
-        context.fGrContext->abandonContext();
-    }
-}
-
-GrContextFactory::ContextInfo GrContextFactory::getContextInfo(GLContextType type,
-                                                               GLContextOptions options) {
+GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType type,
+                                                                GLContextOptions options) {
     for (int i = 0; i < fContexts.count(); ++i) {
-        Context& context = fContexts[i];
-        if (!context.fGLContext) {
-            continue;
-        }
-        if (context.fType == type &&
-            context.fOptions == options) {
-            context.fGLContext->makeCurrent();
-            return ContextInfo(context.fGrContext, context.fGLContext);
+        if (fContexts[i]->fType == type &&
+            fContexts[i]->fOptions == options) {
+            fContexts[i]->fGLContext->makeCurrent();
+            return fContexts[i];
         }
     }
-    SkAutoTDelete<SkGLContext> glCtx;
+    SkAutoTUnref<SkGLContext> glCtx;
     SkAutoTUnref<GrContext> grCtx;
     switch (type) {
         case kNative_GLContextType:
@@ -112,7 +72,7 @@ GrContextFactory::ContextInfo GrContextFactory::getContextInfo(GLContextType typ
             break;
     }
     if (nullptr == glCtx.get()) {
-        return ContextInfo();
+        return nullptr;
     }
 
     SkASSERT(glCtx->isValid());
@@ -122,7 +82,7 @@ GrContextFactory::ContextInfo GrContextFactory::getContextInfo(GLContextType typ
     if (!(kEnableNVPR_GLContextOptions & options)) {
         glInterface.reset(GrGLInterfaceRemoveNVPR(glInterface));
         if (!glInterface) {
-            return ContextInfo();
+            return nullptr;
         }
     }
 
@@ -134,18 +94,18 @@ GrContextFactory::ContextInfo GrContextFactory::getContextInfo(GLContextType typ
     grCtx.reset(GrContext::Create(kOpenGL_GrBackend, p3dctx, fGlobalOptions));
 #endif
     if (!grCtx.get()) {
-        return ContextInfo();
+        return nullptr;
     }
     if (kEnableNVPR_GLContextOptions & options) {
         if (!grCtx->caps()->shaderCaps()->pathRenderingSupport()) {
-            return ContextInfo();
+            return nullptr;
         }
     }
 
-    Context& context = fContexts.push_back();
-    context.fGLContext = glCtx.detach();
-    context.fGrContext = SkRef(grCtx.get());
-    context.fType = type;
-    context.fOptions = options;
-    return ContextInfo(context.fGrContext, context.fGLContext);
+    ContextInfo* ctx = fContexts.emplace_back(new ContextInfo);
+    ctx->fGLContext = SkRef(glCtx.get());
+    ctx->fGrContext = SkRef(grCtx.get());
+    ctx->fType = type;
+    ctx->fOptions = options;
+    return ctx;
 }

diff --git a/src/gpu/GrContextFactory.h b/src/gpu/GrContextFactory.h
index 7afa310..1df99d6 100644 (file)
--- a/src/gpu/GrContextFactory.h
+++ b/src/gpu/GrContextFactory.h
@@ -98,47 +98,59 @@ public:
         }
     }
 
-    explicit GrContextFactory(const GrContextOptions& opts);
-    GrContextFactory();
+    explicit GrContextFactory(const GrContextOptions& opts) : fGlobalOptions(opts) { }
+    GrContextFactory() { }
 
-    ~GrContextFactory();
+    ~GrContextFactory() { this->destroyContexts(); }
 
-    void destroyContexts();
-    void abandonContexts();
+    void destroyContexts() {
+        for (int i = 0; i < fContexts.count(); ++i) {
+            if (fContexts[i]->fGLContext) {  //  could be abandoned.
+                fContexts[i]->fGLContext->makeCurrent();
+            }
+            fContexts[i]->fGrContext->unref();
+            SkSafeUnref(fContexts[i]->fGLContext);
+        }
+        fContexts.reset();
+    }
+
+    void abandonContexts() {
+        for (int i = 0; i < fContexts.count(); ++i) {
+            if (fContexts[i]->fGLContext) {
+                fContexts[i]->fGLContext->testAbandon();
+                SkSafeSetNull(fContexts[i]->fGLContext);
+            }
+            fContexts[i]->fGrContext->abandonContext();
+        }
+    }
 
     struct ContextInfo {
-        ContextInfo()
-            : fGrContext(nullptr), fGLContext(nullptr) { }
-        ContextInfo(GrContext* grContext, SkGLContext* glContext)
-            : fGrContext(grContext), fGLContext(glContext) { }
-        GrContext* fGrContext;
-        SkGLContext* fGLContext; //! Valid until the factory destroys it via abandonContexts() or
-                                 //! destroyContexts().
+        GLContextType             fType;
+        GLContextOptions          fOptions;
+        SkGLContext*              fGLContext;
+        GrContext*                fGrContext;
     };
-
     /**
      * Get a context initialized with a type of GL context. It also makes the GL context current.
+     * Pointer is valid until destroyContexts() is called.
      */
-    ContextInfo getContextInfo(GLContextType type,
-                               GLContextOptions options = kNone_GLContextOptions);
+    ContextInfo* getContextInfo(GLContextType type,
+                                GLContextOptions options = kNone_GLContextOptions);
+
     /**
      * Get a GrContext initialized with a type of GL context. It also makes the GL context current.
      */
-    GrContext* get(GLContextType type,
-                   GLContextOptions options = kNone_GLContextOptions) {
-        return this->getContextInfo(type, options).fGrContext;
+    GrContext* get(GLContextType type, GLContextOptions options = kNone_GLContextOptions) {
+        if (ContextInfo* info = this->getContextInfo(type, options)) {
+            return info->fGrContext;
+        }
+        return nullptr;
     }
     const GrContextOptions& getGlobalOptions() const { return fGlobalOptions; }
 
 private:
-    struct Context {
-        GLContextType fType;
-        GLContextOptions fOptions;
-        SkGLContext*  fGLContext;
-        GrContext*    fGrContext;
-    };
-    SkTArray<Context, true> fContexts;
-    const GrContextOptions  fGlobalOptions;
+    SkTArray<SkAutoTDelete<ContextInfo>, true> fContexts;
+    const GrContextOptions        fGlobalOptions;
 };
 
 #endif

diff --git a/tests/EGLImageTest.cpp b/tests/EGLImageTest.cpp
index fdae3b3..b5754d8 100644 (file)
--- a/tests/EGLImageTest.cpp
+++ b/tests/EGLImageTest.cpp
@@ -28,6 +28,7 @@ static void cleanup(SkGLContext* glctx0, GrGLuint texID0, SkGLContext* glctx1, G
         if (GR_EGL_NO_IMAGE != image1) {
             glctx1->destroyEGLImage(image1);
         }
+        glctx1->unref();
     }
 
     glctx0->makeCurrent();
@@ -90,7 +91,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(EGLImageTest, reporter, context0, glCtx0) {
         return;
     }
 
-    SkAutoTDelete<SkGLContext> glCtx1 = glCtx0->createNew();
+    SkGLContext* glCtx1 = glCtx0->createNew();
     if (!glCtx1) {
         return;
     }

diff --git a/tests/GrContextFactoryTest.cpp b/tests/GrContextFactoryTest.cpp
index 7fe3b50..1b19ac6 100644 (file)
--- a/tests/GrContextFactoryTest.cpp
+++ b/tests/GrContextFactoryTest.cpp
@@ -46,28 +46,4 @@ DEF_GPUTEST(GrContextFactory_NoPathRenderingUnlessNVPRRequested, reporter, /*fac
     }
 }
 
-DEF_GPUTEST(GrContextFactory_abandon, reporter, /*factory*/) {
-    GrContextFactory testFactory;
-    for (int i = 0; i < GrContextFactory::kGLContextTypeCnt; ++i) {
-        GrContextFactory::GLContextType glCtxType = (GrContextFactory::GLContextType) i;
-        GrContextFactory::ContextInfo info1 =
-                testFactory.getContextInfo(glCtxType);
-        REPORTER_ASSERT(reporter, info1.fGrContext);
-        REPORTER_ASSERT(reporter, info1.fGLContext);
-         // Ref for comparison. The API does not explicitly say that this stays alive.
-        info1.fGrContext->ref();
-        testFactory.abandonContexts();
-
-        // Test that we get different context after abandon.
-        GrContextFactory::ContextInfo info2 =
-                testFactory.getContextInfo(glCtxType);
-        REPORTER_ASSERT(reporter, info2.fGrContext);
-        REPORTER_ASSERT(reporter, info2.fGLContext);
-        REPORTER_ASSERT(reporter, info1.fGrContext != info2.fGrContext);
-        // fGLContext should also change, but it also could get the same address.
-
-        info1.fGrContext->unref();
-    }
-}
-
 #endif