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

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

diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index b7858983f8ff6cf95bcc367059e6e5d07eb3d757..827ae037b8a929bac8c5d05ada549620f473fd39 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 ace49076ac74427c58486d13e43ea4acf59aeed1..ba8ad926ad53a486409736b535c547bc78d8ad22 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, GrContextFactory::ContextInfo* context);
+void call_test(T test, skiatest::Reporter* reporter, const GrContextFactory::ContextInfo& context);
 template<>
 void call_test(TestWithGrContext test, skiatest::Reporter* reporter,
-               GrContextFactory::ContextInfo* context) {
-    test(reporter, context->fGrContext);
+               const GrContextFactory::ContextInfo& context) {
+    test(reporter, context.fGrContext);
 }
 template<>
 void call_test(TestWithGrContextAndGLContext test, skiatest::Reporter* reporter,
-               GrContextFactory::ContextInfo* context) {
-    test(reporter, context->fGrContext, context->fGLContext);
+               const GrContextFactory::ContextInfo& context) {
+    test(reporter, context.fGrContext, context.fGLContext);
 }
 #endif
 } // namespace
@@ -1202,11 +1202,13 @@ void RunWithGPUTestContexts(T test, GPUTestContexts testContexts, Reporter* repo
         if ((testContexts & contextSelector) == 0) {
             continue;
         }
-        if (GrContextFactory::ContextInfo* context = factory->getContextInfo(contextType)) {
+        GrContextFactory::ContextInfo context = factory->getContextInfo(contextType);
+        if (context.fGrContext) {
             call_test(test, reporter, context);
         }
-        if (GrContextFactory::ContextInfo* context =
-            factory->getContextInfo(contextType, GrContextFactory::kEnableNVPR_GLContextOptions)) {
+        context = factory->getContextInfo(contextType,
+                                          GrContextFactory::kEnableNVPR_GLContextOptions);
+        if (context.fGrContext) {
             call_test(test, reporter, context);
         }
     }

diff --git a/gyp/pathops_unittest.gyp b/gyp/pathops_unittest.gyp
index 9e3dbba0de88a5d94e12561de76a1ae97240941c..14b231c34c36ecefea6ca2d2618d08f6bd1f060b 100644 (file)
--- a/gyp/pathops_unittest.gyp
+++ b/gyp/pathops_unittest.gyp
@@ -41,6 +41,10 @@
           '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 3420a47973228594d4d1f8f6fc24cca9df685f3c..7edfa730aa746ac33e3366e327645df9f38aeea7 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 SkRefCnt {
+class SK_API SkGLContext : public SkNoncopyable {
 public:
-    ~SkGLContext() override;
+    virtual ~SkGLContext();
 
     bool isValid() const { return NULL != gl(); }
 
@@ -106,8 +106,6 @@ 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 4814e7870e8f0f16e118e824d526abea7122f0a7..b7e48254c881af6835b8c6e195b3a8c2ee1a69a7 100755 (executable)
--- a/src/gpu/GrContextFactory.cpp
+++ b/src/gpu/GrContextFactory.cpp
@@ -23,16 +23,56 @@
 #include "gl/GrGLGpu.h"
 #include "GrCaps.h"
 
-GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType type,
-                                                                GLContextOptions options) {
+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) {
     for (int i = 0; i < fContexts.count(); ++i) {
-        if (fContexts[i]->fType == type &&
-            fContexts[i]->fOptions == options) {
-            fContexts[i]->fGLContext->makeCurrent();
-            return fContexts[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);
         }
     }
-    SkAutoTUnref<SkGLContext> glCtx;
+    SkAutoTDelete<SkGLContext> glCtx;
     SkAutoTUnref<GrContext> grCtx;
     switch (type) {
         case kNative_GLContextType:
@@ -72,7 +112,7 @@ GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType ty
             break;
     }
     if (nullptr == glCtx.get()) {
-        return nullptr;
+        return ContextInfo();
     }
 
     SkASSERT(glCtx->isValid());
@@ -82,7 +122,7 @@ GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType ty
     if (!(kEnableNVPR_GLContextOptions & options)) {
         glInterface.reset(GrGLInterfaceRemoveNVPR(glInterface));
         if (!glInterface) {
-            return nullptr;
+            return ContextInfo();
         }
     }
 
@@ -94,18 +134,18 @@ GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType ty
     grCtx.reset(GrContext::Create(kOpenGL_GrBackend, p3dctx, fGlobalOptions));
 #endif
     if (!grCtx.get()) {
-        return nullptr;
+        return ContextInfo();
     }
     if (kEnableNVPR_GLContextOptions & options) {
         if (!grCtx->caps()->shaderCaps()->pathRenderingSupport()) {
-            return nullptr;
+            return ContextInfo();
         }
     }
 
-    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;
+    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);
 }

diff --git a/src/gpu/GrContextFactory.h b/src/gpu/GrContextFactory.h
index 1df99d6ff1f88d1e516ac024c302eafcc25b9159..7afa3108c69d8984e0d63cd76350570dc764d4be 100644 (file)
--- a/src/gpu/GrContextFactory.h
+++ b/src/gpu/GrContextFactory.h
@@ -98,59 +98,47 @@ public:
         }
     }
 
-    explicit GrContextFactory(const GrContextOptions& opts) : fGlobalOptions(opts) { }
-    GrContextFactory() { }
+    explicit GrContextFactory(const GrContextOptions& opts);
+    GrContextFactory();
 
-    ~GrContextFactory() { this->destroyContexts(); }
+    ~GrContextFactory();
 
-    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();
-        }
-    }
+    void destroyContexts();
+    void abandonContexts();
 
     struct ContextInfo {
-        GLContextType             fType;
-        GLContextOptions          fOptions;
-        SkGLContext*              fGLContext;
-        GrContext*                fGrContext;
+        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().
     };
+
     /**
      * 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) {
-        if (ContextInfo* info = this->getContextInfo(type, options)) {
-            return info->fGrContext;
-        }
-        return nullptr;
+    GrContext* get(GLContextType type,
+                   GLContextOptions options = kNone_GLContextOptions) {
+        return this->getContextInfo(type, options).fGrContext;
     }
     const GrContextOptions& getGlobalOptions() const { return fGlobalOptions; }
 
 private:
-    SkTArray<SkAutoTDelete<ContextInfo>, true> fContexts;
-    const GrContextOptions        fGlobalOptions;
+    struct Context {
+        GLContextType fType;
+        GLContextOptions fOptions;
+        SkGLContext*  fGLContext;
+        GrContext*    fGrContext;
+    };
+    SkTArray<Context, true> fContexts;
+    const GrContextOptions  fGlobalOptions;
 };
 
 #endif

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

diff --git a/tests/GrContextFactoryTest.cpp b/tests/GrContextFactoryTest.cpp
index 1b19ac68e39bf19793353506109f2383455f7ea3..7fe3b50ff8058d00501d6d49b712bf14a653ef62 100644 (file)
--- a/tests/GrContextFactoryTest.cpp
+++ b/tests/GrContextFactoryTest.cpp
@@ -46,4 +46,28 @@ 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