From 3d09c41344ee937ac4d3d24a13b83ecd01590e02 Mon Sep 17 00:00:00 2001 From: "ccameron@chromium.org" Date: Tue, 11 Feb 2014 14:24:40 +0000 Subject: Use base::ScopedTypeRef for CGL types. Use gfx::ScopedCGLSetCurrentContext for setting the current context in the browser process. When calling CGLSetCurrentContext, the pre-existing code inconsitently did either of restoring the context, setting the context to NULL, or leaving the context set. This makes the behavior consistent. It also makes error checking pervasive. BUG=245900 Review URL: https://codereview.chromium.org/147493011 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@250399 0039d316-1c4b-4281-b951-d872f2087c98 --- .../compositing_iosurface_context_mac.h | 5 +- .../compositing_iosurface_context_mac.mm | 84 +++++++--------------- .../compositing_iosurface_layer_mac.mm | 24 +++++-- .../renderer_host/compositing_iosurface_mac.mm | 51 +++++++------ content/browser/renderer_host/display_link_mac.cc | 45 +++++++----- content/browser/renderer_host/display_link_mac.h | 7 +- .../renderer_host/render_widget_host_view_mac.mm | 11 +-- 7 files changed, 117 insertions(+), 110 deletions(-) (limited to 'content/browser') diff --git a/content/browser/renderer_host/compositing_iosurface_context_mac.h b/content/browser/renderer_host/compositing_iosurface_context_mac.h index 4abb008..357925b 100644 --- a/content/browser/renderer_host/compositing_iosurface_context_mac.h +++ b/content/browser/renderer_host/compositing_iosurface_context_mac.h @@ -15,6 +15,7 @@ #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "content/browser/renderer_host/display_link_mac.h" +#include "ui/gl/scoped_cgl.h" namespace content { @@ -60,7 +61,7 @@ class CompositingIOSurfaceContext CompositingIOSurfaceContext( int window_number, NSOpenGLContext* nsgl_context, - CGLContextObj clg_context_strong, + base::ScopedTypeRef clg_context_strong, CGLContextObj clg_context, bool is_vsync_disabled_, scoped_refptr display_link, @@ -69,7 +70,7 @@ class CompositingIOSurfaceContext int window_number_; base::scoped_nsobject nsgl_context_; - CGLContextObj cgl_context_strong_; + base::ScopedTypeRef cgl_context_strong_; // Weak, backed by |nsgl_context_| or |cgl_context_strong_|. CGLContextObj cgl_context_; diff --git a/content/browser/renderer_host/compositing_iosurface_context_mac.mm b/content/browser/renderer_host/compositing_iosurface_context_mac.mm index 4699392..2daef5d 100644 --- a/content/browser/renderer_host/compositing_iosurface_context_mac.mm +++ b/content/browser/renderer_host/compositing_iosurface_context_mac.mm @@ -16,43 +16,6 @@ #include "ui/gl/gl_switches.h" #include "ui/gl/gpu_switching_manager.h" -namespace { - -template -class ScopedCGLTypeRef { - public: - ScopedCGLTypeRef() : object_(NULL) {} - - ~ScopedCGLTypeRef() { - if (object_) - Release(object_); - object_ = NULL; - } - - // Only to be used for pass-by-pointer initialization. The object must have - // been reset to NULL prior to calling. - T* operator&() { - DCHECK(object_ == NULL); - return &object_; - } - - operator T() const { - return object_; - } - - T release() WARN_UNUSED_RESULT { - T object = object_; - object_ = NULL; - return object; - } - - private: - T object_; - DISALLOW_COPY_AND_ASSIGN(ScopedCGLTypeRef); -}; - -} - namespace content { CoreAnimationStatus GetCoreAnimationStatus() { @@ -78,7 +41,7 @@ CompositingIOSurfaceContext::Get(int window_number) { CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableGpuVsync); base::scoped_nsobject nsgl_context; - ScopedCGLTypeRef cgl_context_strong; + base::ScopedTypeRef cgl_context_strong; CGLContextObj cgl_context = NULL; if (GetCoreAnimationStatus() == CORE_ANIMATION_DISABLED) { std::vector attributes; @@ -134,9 +97,10 @@ CompositingIOSurfaceContext::Get(int window_number) { } attribs.push_back(static_cast(0)); GLint number_virtual_screens = 0; - ScopedCGLTypeRef pixel_format; - error = CGLChoosePixelFormat( - &attribs.front(), &pixel_format, &number_virtual_screens); + base::ScopedTypeRef pixel_format; + error = CGLChoosePixelFormat(&attribs.front(), + pixel_format.InitializeInto(), + &number_virtual_screens); if (error != kCGLNoError) { LOG(ERROR) << "Failed to create pixel format object."; return NULL; @@ -148,7 +112,7 @@ CompositingIOSurfaceContext::Get(int window_number) { if (!window_map()->empty()) share_context = window_map()->begin()->second->cgl_context(); error = CGLCreateContext( - pixel_format, share_context, &cgl_context_strong); + pixel_format, share_context, cgl_context_strong.InitializeInto()); if (error != kCGLNoError) { LOG(ERROR) << "Failed to create context object."; return NULL; @@ -161,19 +125,20 @@ CompositingIOSurfaceContext::Get(int window_number) { // Prepare the shader program cache. Precompile the shader programs // needed to draw the IO Surface for non-offscreen contexts. - CGLSetCurrentContext(cgl_context); - scoped_ptr shader_program_cache( - new CompositingIOSurfaceShaderPrograms()); bool prepared = false; - if (window_number == kOffscreenContextWindowNumber) { - prepared = true; - } else { - prepared = ( - shader_program_cache->UseBlitProgram() && - shader_program_cache->UseSolidWhiteProgram()); + scoped_ptr shader_program_cache; + { + gfx::ScopedCGLSetCurrentContext scoped_set_current_context(cgl_context); + shader_program_cache.reset(new CompositingIOSurfaceShaderPrograms()); + if (window_number == kOffscreenContextWindowNumber) { + prepared = true; + } else { + prepared = ( + shader_program_cache->UseBlitProgram() && + shader_program_cache->UseSolidWhiteProgram()); + } + glUseProgram(0u); } - glUseProgram(0u); - CGLSetCurrentContext(0); if (!prepared) { LOG(ERROR) << "IOSurface failed to compile/link required shader programs."; return NULL; @@ -188,7 +153,7 @@ CompositingIOSurfaceContext::Get(int window_number) { return new CompositingIOSurfaceContext( window_number, nsgl_context.release(), - cgl_context_strong.release(), + cgl_context_strong, cgl_context, is_vsync_disabled, display_link, @@ -208,7 +173,7 @@ void CompositingIOSurfaceContext::MarkExistingContextsAsNotShareable() { CompositingIOSurfaceContext::CompositingIOSurfaceContext( int window_number, NSOpenGLContext* nsgl_context, - CGLContextObj cgl_context_strong, + base::ScopedTypeRef cgl_context_strong, CGLContextObj cgl_context, bool is_vsync_disabled, scoped_refptr display_link, @@ -229,9 +194,10 @@ CompositingIOSurfaceContext::CompositingIOSurfaceContext( } CompositingIOSurfaceContext::~CompositingIOSurfaceContext() { - CGLSetCurrentContext(cgl_context_); - shader_program_cache_->Reset(); - CGLSetCurrentContext(0); + { + gfx::ScopedCGLSetCurrentContext scoped_set_current_context(cgl_context_); + shader_program_cache_->Reset(); + } if (can_be_shared_) { DCHECK(window_map()->find(window_number_) != window_map()->end()); DCHECK(window_map()->find(window_number_)->second == this); @@ -241,8 +207,6 @@ CompositingIOSurfaceContext::~CompositingIOSurfaceContext() { if (found != window_map()->end()) DCHECK(found->second != this); } - if (cgl_context_strong_) - CGLReleaseContext(cgl_context_strong_); } NSOpenGLContext* CompositingIOSurfaceContext::nsgl_context() const { diff --git a/content/browser/renderer_host/compositing_iosurface_layer_mac.mm b/content/browser/renderer_host/compositing_iosurface_layer_mac.mm index b05ffe2..38752b4 100644 --- a/content/browser/renderer_host/compositing_iosurface_layer_mac.mm +++ b/content/browser/renderer_host/compositing_iosurface_layer_mac.mm @@ -102,10 +102,26 @@ // This makes the window content not lag behind the resize (at the cost of // blocking on the browser's main thread). if (cached_view->render_widget_host_) { - cached_view->about_to_validate_and_paint_ = true; - (void)cached_view->render_widget_host_->GetBackingStore(true); - cached_view->about_to_validate_and_paint_ = false; - CGLSetCurrentContext(glContext); + // Note that GetBackingStore can potentially spawn a nested run loop, which + // may change the current GL context, or, because the GL contexts are + // shared, may change the currently-bound FBO. Ensure that, when the run + // loop returns, the original GL context remain current, and the original + // FBO remain bound. + // TODO(ccameron): This is far too fragile a mechanism to rely on. Find + // a way to avoid doing this. + GLuint previous_framebuffer = 0; + glGetIntegerv(GL_FRAMEBUFFER_BINDING, + reinterpret_cast(&previous_framebuffer)); + { + gfx::ScopedCGLSetCurrentContext scoped_set_current_context(NULL); + cached_view->about_to_validate_and_paint_ = true; + (void)cached_view->render_widget_host_->GetBackingStore(true); + cached_view->about_to_validate_and_paint_ = false; + } + CHECK_EQ(CGLGetCurrentContext(), glContext) + << "original GL context failed to re-bind after nested run loop, " + << "browser crash is imminent."; + glBindFramebuffer(GL_FRAMEBUFFER, previous_framebuffer); } // If a transition to software mode has occurred, this layer should be diff --git a/content/browser/renderer_host/compositing_iosurface_mac.mm b/content/browser/renderer_host/compositing_iosurface_mac.mm index b7b6e86..31e4f22 100644 --- a/content/browser/renderer_host/compositing_iosurface_mac.mm +++ b/content/browser/renderer_host/compositing_iosurface_mac.mm @@ -257,10 +257,12 @@ CompositingIOSurfaceMac::CompositingIOSurfaceMac( CompositingIOSurfaceMac::~CompositingIOSurfaceMac() { FailAllCopies(); - CGLSetCurrentContext(offscreen_context_->cgl_context()); - DestroyAllCopyContextsWithinContext(); - UnrefIOSurfaceWithContextCurrent(); - CGLSetCurrentContext(0); + { + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( + offscreen_context_->cgl_context()); + DestroyAllCopyContextsWithinContext(); + UnrefIOSurfaceWithContextCurrent(); + } offscreen_context_ = NULL; } @@ -440,12 +442,15 @@ void CompositingIOSurfaceMac::CopyTo( DCHECK_EQ(output->rowBytesAsPixels(), dst_pixel_size.width()) << "Stride is required to be equal to width for GPU readback."; - CGLSetCurrentContext(offscreen_context_->cgl_context()); - const base::Closure copy_done_callback = CopyToSelectedOutputWithinContext( - src_pixel_subrect, gfx::Rect(dst_pixel_size), false, - output.get(), NULL, - base::Bind(&ReverseArgumentOrder, callback, base::Passed(&output))); - CGLSetCurrentContext(0); + base::Closure copy_done_callback; + { + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( + offscreen_context_->cgl_context()); + copy_done_callback = CopyToSelectedOutputWithinContext( + src_pixel_subrect, gfx::Rect(dst_pixel_size), false, + output.get(), NULL, + base::Bind(&ReverseArgumentOrder, callback, base::Passed(&output))); + } if (!copy_done_callback.is_null()) copy_done_callback.Run(); } @@ -454,10 +459,13 @@ void CompositingIOSurfaceMac::CopyToVideoFrame( const gfx::Rect& src_pixel_subrect, const scoped_refptr& target, const base::Callback& callback) { - CGLSetCurrentContext(offscreen_context_->cgl_context()); - const base::Closure copy_done_callback = CopyToVideoFrameWithinContext( - src_pixel_subrect, false, target, callback); - CGLSetCurrentContext(0); + base::Closure copy_done_callback; + { + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( + offscreen_context_->cgl_context()); + copy_done_callback = CopyToVideoFrameWithinContext( + src_pixel_subrect, false, target, callback); + } if (!copy_done_callback.is_null()) copy_done_callback.Run(); } @@ -542,9 +550,9 @@ bool CompositingIOSurfaceMac::MapIOSurfaceToTextureWithContextCurrent( } void CompositingIOSurfaceMac::UnrefIOSurface() { - CGLSetCurrentContext(offscreen_context_->cgl_context()); + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( + offscreen_context_->cgl_context()); UnrefIOSurfaceWithContextCurrent(); - CGLSetCurrentContext(0); } void CompositingIOSurfaceMac::DrawQuad(const SurfaceQuad& quad) { @@ -747,11 +755,12 @@ void CompositingIOSurfaceMac::CheckIfAllCopiesAreFinished( return; std::vector done_callbacks; - CGLContextObj previous_context = CGLGetCurrentContext(); - CGLSetCurrentContext(offscreen_context_->cgl_context()); - CheckIfAllCopiesAreFinishedWithinContext( - block_until_finished, &done_callbacks); - CGLSetCurrentContext(previous_context); + { + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( + offscreen_context_->cgl_context()); + CheckIfAllCopiesAreFinishedWithinContext( + block_until_finished, &done_callbacks); + } for (size_t i = 0; i < done_callbacks.size(); ++i) done_callbacks[i].Run(); } diff --git a/content/browser/renderer_host/display_link_mac.cc b/content/browser/renderer_host/display_link_mac.cc index b7239c0..8d26a6d 100644 --- a/content/browser/renderer_host/display_link_mac.cc +++ b/content/browser/renderer_host/display_link_mac.cc @@ -7,23 +7,36 @@ #include "base/debug/trace_event.h" #include "base/logging.h" +namespace base { + +template<> +struct ScopedTypeRefTraits { + static void Retain(CVDisplayLinkRef object) { + CVDisplayLinkRetain(object); + } + static void Release(CVDisplayLinkRef object) { + CVDisplayLinkRelease(object); + } +}; + +} // namespace base + namespace content { // static scoped_refptr DisplayLinkMac::Create() { CVReturn ret = kCVReturnSuccess; - scoped_refptr display_link_mac; - { - CVDisplayLinkRef display_link = NULL; - ret = CVDisplayLinkCreateWithActiveCGDisplays(&display_link); - if (ret != kCVReturnSuccess) { - LOG(ERROR) << "CVDisplayLinkCreateWithActiveCGDisplays failed: " << ret; - return NULL; - } - display_link_mac = new DisplayLinkMac(display_link); + base::ScopedTypeRef display_link; + ret = CVDisplayLinkCreateWithActiveCGDisplays(display_link.InitializeInto()); + if (ret != kCVReturnSuccess) { + LOG(ERROR) << "CVDisplayLinkCreateWithActiveCGDisplays failed: " << ret; + return NULL; } + scoped_refptr display_link_mac; + display_link_mac = new DisplayLinkMac(display_link); + ret = CVDisplayLinkSetOutputCallback( display_link_mac->display_link_, &DisplayLinkCallback, @@ -36,18 +49,18 @@ scoped_refptr DisplayLinkMac::Create() { return display_link_mac; } -DisplayLinkMac::DisplayLinkMac(CVDisplayLinkRef display_link) - : display_link_(display_link), - stop_timer_( - FROM_HERE, base::TimeDelta::FromSeconds(1), - this, &DisplayLinkMac::StopDisplayLink), - timebase_and_interval_valid_(false) { +DisplayLinkMac::DisplayLinkMac( + base::ScopedTypeRef display_link) + : display_link_(display_link), + stop_timer_( + FROM_HERE, base::TimeDelta::FromSeconds(1), + this, &DisplayLinkMac::StopDisplayLink), + timebase_and_interval_valid_(false) { } DisplayLinkMac::~DisplayLinkMac() { if (CVDisplayLinkIsRunning(display_link_)) CVDisplayLinkStop(display_link_); - CVDisplayLinkRelease(display_link_); } bool DisplayLinkMac::GetVSyncParameters( diff --git a/content/browser/renderer_host/display_link_mac.h b/content/browser/renderer_host/display_link_mac.h index 9398015..c708608 100644 --- a/content/browser/renderer_host/display_link_mac.h +++ b/content/browser/renderer_host/display_link_mac.h @@ -5,8 +5,9 @@ #ifndef CONTENT_BROWSER_RENDERER_HOST_DISPLAY_LINK_MAC_H_ #define CONTENT_BROWSER_RENDERER_HOST_DISPLAY_LINK_MAC_H_ -#import +#include +#include "base/mac/scoped_typeref.h" #include "base/memory/ref_counted.h" #include "base/synchronization/lock.h" #include "base/time/time.h" @@ -26,7 +27,7 @@ class DisplayLinkMac : public base::RefCounted { private: friend class base::RefCounted; - DisplayLinkMac(CVDisplayLinkRef display_link); + DisplayLinkMac(base::ScopedTypeRef display_link); virtual ~DisplayLinkMac(); void StartOrContinueDisplayLink(); @@ -42,7 +43,7 @@ class DisplayLinkMac : public base::RefCounted { void* context); // CVDisplayLink for querying VSync timing info. - CVDisplayLinkRef display_link_; + base::ScopedTypeRef display_link_; // Timer for stopping the display link if it has not been queried in // the last second. diff --git a/content/browser/renderer_host/render_widget_host_view_mac.mm b/content/browser/renderer_host/render_widget_host_view_mac.mm index 9130ced..bf1c800 100644 --- a/content/browser/renderer_host/render_widget_host_view_mac.mm +++ b/content/browser/renderer_host/render_widget_host_view_mac.mm @@ -1310,7 +1310,8 @@ void RenderWidgetHostViewMac::CompositorSwapBuffers( RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback callback; if (frame_subscriber_->ShouldCaptureFrame(present_time, &frame, &callback)) { - CGLSetCurrentContext(compositing_iosurface_context_->cgl_context()); + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( + compositing_iosurface_context_->cgl_context()); compositing_iosurface_->SetIOSurfaceWithContextCurrent( compositing_iosurface_context_, surface_handle, size, surface_scale_factor); @@ -1352,7 +1353,8 @@ void RenderWidgetHostViewMac::CompositorSwapBuffers( // Make the context current and update the IOSurface with the handle // passed in by the swap command. - CGLSetCurrentContext(compositing_iosurface_context_->cgl_context()); + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( + compositing_iosurface_context_->cgl_context()); if (!compositing_iosurface_->SetIOSurfaceWithContextCurrent( compositing_iosurface_context_, surface_handle, size, surface_scale_factor)) { @@ -1376,7 +1378,8 @@ void RenderWidgetHostViewMac::CompositorSwapBuffers( compositing_iosurface_->CopyToVideoFrame( gfx::Rect(size), frame, base::Bind(callback, present_time)); - CGLSetCurrentContext(compositing_iosurface_context_->cgl_context()); + DCHECK_EQ(CGLGetCurrentContext(), + compositing_iosurface_context_->cgl_context()); } } @@ -2914,7 +2917,7 @@ void RenderWidgetHostViewMac::TickPendingLatencyInfoDelay() { NSRectFill(dirtyRect); } - CGLSetCurrentContext( + gfx::ScopedCGLSetCurrentContext scoped_set_current_context( renderWidgetHostView_->compositing_iosurface_context_->cgl_context()); if (renderWidgetHostView_->DrawIOSurfaceWithoutCoreAnimation()) return; -- cgit v1.1