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 b785898..827ae03 100644
--- a/bench/nanobench.cpp
+++ b/bench/nanobench.cpp
@@ -175,7 +175,7 @@
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 ace4907..ba8ad92 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -1147,16 +1147,16 @@
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 @@
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 9e3dbba..14b231c 100644
--- 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 3420a47..7edfa73 100644
--- 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 @@
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 4814e78..b7e4825 100755
--- 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 @@
break;
}
if (nullptr == glCtx.get()) {
- return nullptr;
+ return ContextInfo();
}
SkASSERT(glCtx->isValid());
@@ -82,7 +122,7 @@
if (!(kEnableNVPR_GLContextOptions & options)) {
glInterface.reset(GrGLInterfaceRemoveNVPR(glInterface));
if (!glInterface) {
- return nullptr;
+ return ContextInfo();
}
}
@@ -94,18 +134,18 @@
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 1df99d6..7afa310 100644
--- a/src/gpu/GrContextFactory.h
+++ b/src/gpu/GrContextFactory.h
@@ -98,59 +98,47 @@
}
}
- 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 b5754d8..fdae3b3 100644
--- a/tests/EGLImageTest.cpp
+++ b/tests/EGLImageTest.cpp
@@ -28,7 +28,6 @@
if (GR_EGL_NO_IMAGE != image1) {
glctx1->destroyEGLImage(image1);
}
- glctx1->unref();
}
glctx0->makeCurrent();
@@ -91,7 +90,7 @@
return;
}
- SkGLContext* glCtx1 = glCtx0->createNew();
+ SkAutoTDelete<SkGLContext> glCtx1 = glCtx0->createNew();
if (!glCtx1) {
return;
}
diff --git a/tests/GrContextFactoryTest.cpp b/tests/GrContextFactoryTest.cpp
index 1b19ac6..7fe3b50 100644
--- a/tests/GrContextFactoryTest.cpp
+++ b/tests/GrContextFactoryTest.cpp
@@ -46,4 +46,28 @@
}
}
+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
