From 9552137be6d97cbdc02b26b9292b3268284ccec6 Mon Sep 17 00:00:00 2001 From: "sievers@google.com" Date: Wed, 2 Apr 2014 18:37:12 +0000 Subject: [PATCH] gpu: Bind dummy GL API when no context is current Also make platform behavior consistent in always releasing any previously current context when MakeCurrent() fails. This catches GL call sites with no context current. It also avoids problems with GL implementations potentially not liking this (and crashing) or even us ending up calling into the wrong context (for example accidentally deleting a resource in the wrong context). BUG=355275 R=piman@chromium.org Review URL: https://codereview.chromium.org/221433004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@261153 0039d316-1c4b-4281-b951-d872f2087c98 --- .../common/gpu/texture_image_transport_surface.cc | 20 +++++--- .../service/gles2_cmd_decoder_unittest.cc | 10 ++++ ui/gl/generate_bindings.py | 54 +++++++++++++++++----- ui/gl/gl_context.cc | 14 ++++++ ui/gl/gl_context.h | 12 +++++ ui/gl/gl_context_cgl.cc | 3 +- ui/gl/gl_context_egl.cc | 3 +- ui/gl/gl_context_glx.cc | 4 +- ui/gl/gl_context_nsview.mm | 3 +- ui/gl/gl_context_osmesa.cc | 4 +- ui/gl/gl_context_wgl.cc | 3 +- ui/gl/gl_gl_api_implementation.cc | 14 ++++++ ui/gl/gl_gl_api_implementation.h | 22 +++++++-- 13 files changed, 135 insertions(+), 31 deletions(-) diff --git a/content/common/gpu/texture_image_transport_surface.cc b/content/common/gpu/texture_image_transport_surface.cc index 45ce615c4951..413d1d4d6367 100644 --- a/content/common/gpu/texture_image_transport_surface.cc +++ b/content/common/gpu/texture_image_transport_surface.cc @@ -30,8 +30,7 @@ namespace content { namespace { bool IsContextValid(ImageTransportHelper* helper) { - return helper->stub()->decoder()->GetGLContext()->IsCurrent(NULL) || - helper->stub()->decoder()->WasContextLost(); + return helper->stub()->decoder()->GetGLContext()->IsCurrent(NULL); } } // namespace @@ -175,19 +174,26 @@ void TextureImageTransportSurface::OnResize(gfx::Size size, } void TextureImageTransportSurface::OnWillDestroyStub() { - DCHECK(IsContextValid(helper_.get())); + bool have_context = IsContextValid(helper_.get()); helper_->stub()->RemoveDestructionObserver(this); // We are losing the stub owning us, this is our last chance to clean up the // resources we allocated in the stub's context. - ReleaseBackTexture(); - ReleaseFrontTexture(); + if (have_context) { + ReleaseBackTexture(); + ReleaseFrontTexture(); + } else { + backbuffer_ = NULL; + back_mailbox_ = Mailbox(); + frontbuffer_ = NULL; + front_mailbox_ = Mailbox(); + } - if (fbo_id_) { + if (fbo_id_ && have_context) { glDeleteFramebuffersEXT(1, &fbo_id_); CHECK_GL_ERROR(); - fbo_id_ = 0; } + fbo_id_ = 0; stub_destroyed_ = true; } diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc index 16783b8e6fd0..882a1249c21a 100644 --- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc +++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc @@ -6622,6 +6622,11 @@ void GLES2DecoderWithShaderTest::CheckRenderbufferChangesMarkFBOAsNotComplete( } else { EXPECT_TRUE(framebuffer_manager->IsComplete(framebuffer)); } + // Cleanup + DoDeleteFramebuffer( + client_framebuffer_id_, kServiceFramebufferId, + bound_fbo, GL_FRAMEBUFFER, 0, + bound_fbo, GL_FRAMEBUFFER, 0); } TEST_F(GLES2DecoderWithShaderTest, @@ -6711,6 +6716,11 @@ void GLES2DecoderWithShaderTest::CheckTextureChangesMarkFBOAsNotComplete( } else { EXPECT_TRUE(framebuffer_manager->IsComplete(framebuffer)); } + // Cleanup + DoDeleteFramebuffer( + client_framebuffer_id_, kServiceFramebufferId, + bound_fbo, GL_FRAMEBUFFER, 0, + bound_fbo, GL_FRAMEBUFFER, 0); } TEST_F(GLES2DecoderWithShaderTest, TextureChangesMarkFBOAsNotCompleteBoundFBO) { diff --git a/ui/gl/generate_bindings.py b/ui/gl/generate_bindings.py index 6bf14dd48986..4778595a07fd 100755 --- a/ui/gl/generate_bindings.py +++ b/ui/gl/generate_bindings.py @@ -1652,6 +1652,15 @@ namespace gfx { } """ % set_name.upper()) + def MakeArgNames(arguments): + argument_names = re.sub( + r'(const )?[a-zA-Z0-9_]+\** ([a-zA-Z0-9_]+)', r'\2', arguments) + argument_names = re.sub( + r'(const )?[a-zA-Z0-9_]+\** ([a-zA-Z0-9_]+)', r'\2', argument_names) + if argument_names == 'void' or argument_names == '': + argument_names = '' + return argument_names + # Write GLApiBase functions for func in functions: function_name = func['known_as'] @@ -1660,12 +1669,7 @@ namespace gfx { file.write('\n') file.write('%s %sApiBase::%sFn(%s) {\n' % (return_type, set_name.upper(), function_name, arguments)) - argument_names = re.sub( - r'(const )?[a-zA-Z0-9_]+\** ([a-zA-Z0-9_]+)', r'\2', arguments) - argument_names = re.sub( - r'(const )?[a-zA-Z0-9_]+\** ([a-zA-Z0-9_]+)', r'\2', argument_names) - if argument_names == 'void' or argument_names == '': - argument_names = '' + argument_names = MakeArgNames(arguments) if return_type == 'void': file.write(' driver_->fn.%sFn(%s);\n' % (function_name, argument_names)) @@ -1682,12 +1686,7 @@ namespace gfx { file.write('\n') file.write('%s Trace%sApi::%sFn(%s) {\n' % (return_type, set_name.upper(), function_name, arguments)) - argument_names = re.sub( - r'(const )?[a-zA-Z0-9_]+\** ([a-zA-Z0-9_]+)', r'\2', arguments) - argument_names = re.sub( - r'(const )?[a-zA-Z0-9_]+\** ([a-zA-Z0-9_]+)', r'\2', argument_names) - if argument_names == 'void' or argument_names == '': - argument_names = '' + argument_names = MakeArgNames(arguments) file.write(' TRACE_EVENT_BINARY_EFFICIENT0("gpu", "TraceGLAPI::%s")\n' % function_name) if return_type == 'void': @@ -1698,6 +1697,37 @@ namespace gfx { (set_name.lower(), function_name, argument_names)) file.write('}\n') + # Write NoContextGLApi functions + if set_name.upper() == "GL": + for func in functions: + function_name = func['known_as'] + return_type = func['return_type'] + arguments = func['arguments'] + file.write('\n') + file.write('%s NoContextGLApi::%sFn(%s) {\n' % + (return_type, function_name, arguments)) + argument_names = MakeArgNames(arguments) + no_context_error = "Trying to call %s() without current GL context" % function_name + file.write(' NOTREACHED() << "%s";\n' % no_context_error) + file.write(' LOG(ERROR) << "%s";\n' % no_context_error) + default_value = { 'GLenum': 'static_cast(0)', + 'GLuint': '0U', + 'GLint': '0', + 'GLboolean': 'GL_FALSE', + 'GLbyte': '0', + 'GLubyte': '0', + 'GLbutfield': '0', + 'GLushort': '0', + 'GLsizei': '0', + 'GLfloat': '0.0f', + 'GLdouble': '0.0', + 'GLsync': 'NULL'} + if return_type.endswith('*'): + file.write(' return NULL;\n') + elif return_type != 'void': + file.write(' return %s;\n' % default_value[return_type]) + file.write('}\n') + file.write('\n') file.write('} // namespace gfx\n') diff --git a/ui/gl/gl_context.cc b/ui/gl/gl_context.cc index cd9a28c387dc..eb0a9dd835dc 100644 --- a/ui/gl/gl_context.cc +++ b/ui/gl/gl_context.cc @@ -26,6 +26,18 @@ base::LazyInstance >::Leaky current_real_context_ = LAZY_INSTANCE_INITIALIZER; } // namespace +GLContext::ScopedReleaseCurrent::ScopedReleaseCurrent() : canceled_(false) {} + +GLContext::ScopedReleaseCurrent::~ScopedReleaseCurrent() { + if (!canceled_ && GetCurrent()) { + GetCurrent()->ReleaseCurrent(NULL); + } +} + +void GLContext::ScopedReleaseCurrent::Cancel() { + canceled_ = true; +} + GLContext::GLContext(GLShareGroup* share_group) : share_group_(share_group) { if (!share_group_.get()) share_group_ = new GLShareGroup; @@ -125,6 +137,8 @@ GLContext* GLContext::GetRealCurrent() { void GLContext::SetCurrent(GLSurface* surface) { current_context_.Pointer()->Set(surface ? this : NULL); GLSurface::SetCurrent(surface); + if (!surface) + SetGLApiToNoContext(); } GLStateRestorer* GLContext::GetGLStateRestorer() { diff --git a/ui/gl/gl_context.h b/ui/gl/gl_context.h index f40b1bba7864..63589c7cf382 100644 --- a/ui/gl/gl_context.h +++ b/ui/gl/gl_context.h @@ -117,6 +117,18 @@ class GL_EXPORT GLContext : public base::RefCounted { protected: virtual ~GLContext(); + // Will release the current context when going out of scope, unless canceled. + class ScopedReleaseCurrent { + public: + ScopedReleaseCurrent(); + ~ScopedReleaseCurrent(); + + void Cancel(); + + private: + bool canceled_; + }; + // Sets the GL api to the real hardware API (vs the VirtualAPI) static void SetRealGLApi(); virtual void SetCurrent(GLSurface* surface); diff --git a/ui/gl/gl_context_cgl.cc b/ui/gl/gl_context_cgl.cc index 151c484d550f..141d9352a42c 100644 --- a/ui/gl/gl_context_cgl.cc +++ b/ui/gl/gl_context_cgl.cc @@ -178,6 +178,7 @@ bool GLContextCGL::MakeCurrent(GLSurface* surface) { if (IsCurrent(surface)) return true; + ScopedReleaseCurrent release_current; TRACE_EVENT0("gpu", "GLContextCGL::MakeCurrent"); if (CGLSetCurrentContext( @@ -191,7 +192,6 @@ bool GLContextCGL::MakeCurrent(GLSurface* surface) { SetCurrent(surface); if (!InitializeDynamicBindings()) { - ReleaseCurrent(surface); return false; } @@ -200,6 +200,7 @@ bool GLContextCGL::MakeCurrent(GLSurface* surface) { return false; } + release_current.Cancel(); return true; } diff --git a/ui/gl/gl_context_egl.cc b/ui/gl/gl_context_egl.cc index 856d84c89cb5..2554f8c5b1d0 100644 --- a/ui/gl/gl_context_egl.cc +++ b/ui/gl/gl_context_egl.cc @@ -93,6 +93,7 @@ bool GLContextEGL::MakeCurrent(GLSurface* surface) { if (IsCurrent(surface)) return true; + ScopedReleaseCurrent release_current; TRACE_EVENT2("gpu", "GLContextEGL::MakeCurrent", "context", context_, "surface", surface); @@ -116,7 +117,6 @@ bool GLContextEGL::MakeCurrent(GLSurface* surface) { SetCurrent(surface); if (!InitializeDynamicBindings()) { - ReleaseCurrent(surface); return false; } @@ -125,6 +125,7 @@ bool GLContextEGL::MakeCurrent(GLSurface* surface) { return false; } + release_current.Cancel(); return true; } diff --git a/ui/gl/gl_context_glx.cc b/ui/gl/gl_context_glx.cc index 30b07c7ca7d7..d5ab073e40da 100644 --- a/ui/gl/gl_context_glx.cc +++ b/ui/gl/gl_context_glx.cc @@ -98,6 +98,7 @@ bool GLContextGLX::MakeCurrent(GLSurface* surface) { if (IsCurrent(surface)) return true; + ScopedReleaseCurrent release_current; TRACE_EVENT0("gpu", "GLContextGLX::MakeCurrent"); if (!glXMakeContextCurrent( display_, @@ -114,18 +115,17 @@ bool GLContextGLX::MakeCurrent(GLSurface* surface) { SetCurrent(surface); if (!InitializeDynamicBindings()) { - ReleaseCurrent(surface); Destroy(); return false; } if (!surface->OnMakeCurrent(this)) { LOG(ERROR) << "Could not make current."; - ReleaseCurrent(surface); Destroy(); return false; } + release_current.Cancel(); return true; } diff --git a/ui/gl/gl_context_nsview.mm b/ui/gl/gl_context_nsview.mm index 8f38d08166d0..bc4a87b910bc 100644 --- a/ui/gl/gl_context_nsview.mm +++ b/ui/gl/gl_context_nsview.mm @@ -55,6 +55,7 @@ void GLContextNSView::Destroy() { } bool GLContextNSView::MakeCurrent(GLSurface* surface) { + ScopedReleaseCurrent release_current; TRACE_EVENT0("gpu", "GLContextNSView::MakeCurrent"); AcceleratedWidget view = static_cast(surface->GetHandle()); @@ -67,7 +68,6 @@ bool GLContextNSView::MakeCurrent(GLSurface* surface) { SetRealGLApi(); SetCurrent(surface); if (!InitializeDynamicBindings()) { - ReleaseCurrent(surface); return false; } @@ -76,6 +76,7 @@ bool GLContextNSView::MakeCurrent(GLSurface* surface) { return false; } + release_current.Cancel(); return true; } diff --git a/ui/gl/gl_context_osmesa.cc b/ui/gl/gl_context_osmesa.cc index e7ccba99039c..524fe42bb171 100644 --- a/ui/gl/gl_context_osmesa.cc +++ b/ui/gl/gl_context_osmesa.cc @@ -52,6 +52,7 @@ bool GLContextOSMesa::MakeCurrent(GLSurface* surface) { gfx::Size size = surface->GetSize(); + ScopedReleaseCurrent release_current; if (!OSMesaMakeCurrent(context_, surface->GetHandle(), GL_UNSIGNED_BYTE, @@ -70,7 +71,6 @@ bool GLContextOSMesa::MakeCurrent(GLSurface* surface) { SetCurrent(surface); if (!InitializeDynamicBindings()) { - ReleaseCurrent(surface); return false; } @@ -79,6 +79,7 @@ bool GLContextOSMesa::MakeCurrent(GLSurface* surface) { return false; } + release_current.Cancel(); return true; } @@ -87,6 +88,7 @@ void GLContextOSMesa::ReleaseCurrent(GLSurface* surface) { return; SetCurrent(NULL); + // TODO: Calling with NULL here does not work to release the context. OSMesaMakeCurrent(NULL, NULL, GL_UNSIGNED_BYTE, 0, 0); } diff --git a/ui/gl/gl_context_wgl.cc b/ui/gl/gl_context_wgl.cc index 41f1b6349e02..abe47e4f9f7a 100644 --- a/ui/gl/gl_context_wgl.cc +++ b/ui/gl/gl_context_wgl.cc @@ -74,6 +74,7 @@ bool GLContextWGL::MakeCurrent(GLSurface* surface) { if (IsCurrent(surface)) return true; + ScopedReleaseCurrent release_current; TRACE_EVENT0("gpu", "GLContextWGL::MakeCurrent"); if (!wglMakeCurrent(static_cast(surface->GetHandle()), context_)) { @@ -86,7 +87,6 @@ bool GLContextWGL::MakeCurrent(GLSurface* surface) { SetCurrent(surface); if (!InitializeDynamicBindings()) { - ReleaseCurrent(surface); return false; } @@ -95,6 +95,7 @@ bool GLContextWGL::MakeCurrent(GLSurface* surface) { return false; } + release_current.Cancel(); return true; } diff --git a/ui/gl/gl_gl_api_implementation.cc b/ui/gl/gl_gl_api_implementation.cc index f925f64ffa88..42a937062ed4 100644 --- a/ui/gl/gl_gl_api_implementation.cc +++ b/ui/gl/gl_gl_api_implementation.cc @@ -22,6 +22,9 @@ namespace gfx { static GLApi* g_gl; // A GL Api that calls directly into the driver. static RealGLApi* g_real_gl; +// A GL Api that does nothing but warn about illegal GL calls without a context +// current. +static NoContextGLApi* g_no_context_gl; // A GL Api that calls TRACE and then calls another GL api. static TraceGLApi* g_trace_gl; // GL version used when initializing dynamic bindings. @@ -258,6 +261,7 @@ void InitializeStaticGLBindingsGL() { if (!g_real_gl) { g_real_gl = new RealGLApi(); g_trace_gl = new TraceGLApi(g_real_gl); + g_no_context_gl = new NoContextGLApi(); } g_real_gl->Initialize(&g_driver_gl); g_gl = g_real_gl; @@ -280,6 +284,10 @@ void SetGLToRealGLApi() { SetGLApi(g_gl); } +void SetGLApiToNoContext() { + SetGLApi(g_no_context_gl); +} + void InitializeDynamicGLBindingsGL(GLContext* context) { g_driver_gl.InitializeCustomDynamicBindings(context); DCHECK(context && context->IsCurrent(NULL) && !g_version_info); @@ -356,6 +364,12 @@ void RealGLApi::Initialize(DriverGL* driver) { TraceGLApi::~TraceGLApi() { } +NoContextGLApi::NoContextGLApi() { +} + +NoContextGLApi::~NoContextGLApi() { +} + VirtualGLApi::VirtualGLApi() : real_context_(NULL), current_context_(NULL) { diff --git a/ui/gl/gl_gl_api_implementation.h b/ui/gl/gl_gl_api_implementation.h index c87dbd1de088..d1896912779a 100644 --- a/ui/gl/gl_gl_api_implementation.h +++ b/ui/gl/gl_gl_api_implementation.h @@ -7,7 +7,6 @@ #include "base/compiler_specific.h" #include "ui/gl/gl_bindings.h" -#include "ui/gl/gl_export.h" namespace gpu { namespace gles2 { @@ -29,8 +28,9 @@ bool SetNullDrawGLBindingsEnabledGL(bool enabled); void ClearGLBindingsGL(); void SetGLToRealGLApi(); void SetGLApi(GLApi* api); +void SetGLApiToNoContext(); -class GL_EXPORT GLApiBase : public GLApi { +class GLApiBase : public GLApi { public: // Include the auto-generated part of this class. We split this because // it means we can easily edit the non-auto generated parts right here in @@ -46,7 +46,7 @@ class GL_EXPORT GLApiBase : public GLApi { }; // Implemenents the GL API by calling directly into the driver. -class GL_EXPORT RealGLApi : public GLApiBase { +class RealGLApi : public GLApiBase { public: RealGLApi(); virtual ~RealGLApi(); @@ -54,7 +54,7 @@ class GL_EXPORT RealGLApi : public GLApiBase { }; // Inserts a TRACE for every GL call. -class GL_EXPORT TraceGLApi : public GLApi { +class TraceGLApi : public GLApi { public: TraceGLApi(GLApi* gl_api) : gl_api_(gl_api) { } virtual ~TraceGLApi(); @@ -68,10 +68,22 @@ class GL_EXPORT TraceGLApi : public GLApi { GLApi* gl_api_; }; +// Catches incorrect usage when GL calls are made without a current context. +class NoContextGLApi : public GLApi { + public: + NoContextGLApi(); + virtual ~NoContextGLApi(); + + // Include the auto-generated part of this class. We split this because + // it means we can easily edit the non-auto generated parts right here in + // this file instead of having to edit some template or the code generator. + #include "gl_bindings_api_autogen_gl.h" +}; + // Implementents the GL API using co-operative state restoring. // Assumes there is only one real GL context and that multiple virtual contexts // are implemented above it. Restores the needed state from the current context. -class GL_EXPORT VirtualGLApi : public GLApiBase { +class VirtualGLApi : public GLApiBase { public: VirtualGLApi(); virtual ~VirtualGLApi(); -- 2.11.4.GIT