|
|
Created:
7 years, 9 months ago by ccameron Modified:
7 years, 8 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, jam, penghuang+watch_chromium.org, sail+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDo not create one GL context per CompositingIOSurface
Do not create one GL context per CompositingIOSurface, rather,
share a single GL context across all CompositingIOSurfaces that
are drawing to a given NSWindow (as determined by window number)
and have the same window ordering (above or below).
This cuts the regression in the ShutdownTests.TwentyTabs* from
81% to 47%. Most of the rest is from the cost of destroying the
renderers' GL contexts in the GPU process.
BUG=180463
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192111
Patch Set 1 #
Total comments: 9
Patch Set 2 : Incorporate review feedback #Patch Set 3 : Minimize diffs #Patch Set 4 : Resolve against https://codereview.chromium.org/13363002/ #
Total comments: 10
Patch Set 5 : Re-resolve #Patch Set 6 : More and better comments #
Total comments: 4
Patch Set 7 : Fix pair syntax #Patch Set 8 : Re-resolve against HEAD #
Messages
Total messages: 19 (0 generated)
This takes care of half of the shutdown regression. https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/compositing_iosurface_mac.mm (right): https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.mm:241: if (order == SURFACE_ORDER_BELOW_WINDOW) { I have yet to find an instance where just skipping this logic results in incorrect rendering. Is there an example that I can try? I tried the scenarios listed in crbug.com/161603 Of note is that if I take this branch inappropriately, there is incorrect rendering (and setting below/above dynamically before draw also results in incorrect rendering). https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.mm:325: CompositingIOSurfaceMac::ContextData::window_context_map_ = NULL; This needs to be a pointer because we have warnings to disallow at-exit dtors (having it as a pointer feels a bit hacky)
https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/compositing_iosurface_mac.mm (right): https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.mm:241: if (order == SURFACE_ORDER_BELOW_WINDOW) { For tab web contents, we set this flag so that the find bar will display above web contents. For web contents used in things like extensions, we don't bother with this flag for sanity's sake. If you want to verify this logic, type Command-F and make sure you can see the find bar. Without hole punching and this ordering stuff, the find bar won't be visible.
On 2013/03/26 20:59:39, Avi wrote: > https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... > File content/browser/renderer_host/compositing_iosurface_mac.mm (right): > > https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... > content/browser/renderer_host/compositing_iosurface_mac.mm:241: if (order == > SURFACE_ORDER_BELOW_WINDOW) { > For tab web contents, we set this flag so that the find bar will display above > web contents. For web contents used in things like extensions, we don't bother > with this flag for sanity's sake. > > If you want to verify this logic, type Command-F and make sure you can see the > find bar. Without hole punching and this ordering stuff, the find bar won't be > visible. Perfect, that shows it (if I get rid of this, then Command-F is hidden). Thanks!
Ping
Sorry for the delay. LGTM with a couple of small issues. https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/compositing_iosurface_mac.h (right): https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.h:220: int window_number_; The public fields here violate the style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control I realize this is only used internally so this isn't a big deal, but consider whether this should be a struct (would also violate the style guide since it isn't entirely a passive object). https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/compositing_iosurface_mac.mm (right): https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.mm:314: DCHECK(window_context_map_->find(key) == window_context_map_->end()); Did you run this in debug mode? Isn't the sense of this DCHECK reversed? https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.mm:325: CompositingIOSurfaceMac::ContextData::window_context_map_ = NULL; On 2013/03/26 20:52:43, ccameron1 wrote: > This needs to be a pointer because we have warnings to disallow at-exit dtors > (having it as a pointer feels a bit hacky) Consider using base's LazyInstance or Singleton for this; it looks like they will reduce the amount of error checking code throughout.
Thanks!! https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/compositing_iosurface_mac.h (right): https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.h:220: int window_number_; Thanks -- I changed this to use accessors, and make the members private. It's more consistent that way. https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/compositing_iosurface_mac.mm (right): https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.mm:314: DCHECK(window_context_map_->find(key) == window_context_map_->end()); On 2013/03/28 00:55:00, kbr wrote: > Did you run this in debug mode? Isn't the sense of this DCHECK reversed? Fixed (eek... that got flipped just after I uploaded...). https://codereview.chromium.org/13097002/diff/1/content/browser/renderer_host... content/browser/renderer_host/compositing_iosurface_mac.mm:325: CompositingIOSurfaceMac::ContextData::window_context_map_ = NULL; Thanks -- LazyInstance is what I needed here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/13097002/15001
Failed to apply patch for content/browser/renderer_host/compositing_iosurface_mac.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file content/browser/renderer_host/compositing_iosurface_mac.h Hunk #1 succeeded at 10 with fuzz 1 (offset 2 lines). Hunk #2 succeeded at 47 (offset 5 lines). Hunk #3 succeeded at 71 (offset 5 lines). Hunk #4 succeeded at 114 (offset 4 lines). Hunk #5 FAILED at 209. Hunk #6 succeeded at 289 (offset 19 lines). Hunk #7 FAILED at 294. 2 out of 7 hunks FAILED -- saving rejects to file content/browser/renderer_host/compositing_iosurface_mac.h.rej Patch: content/browser/renderer_host/compositing_iosurface_mac.h Index: content/browser/renderer_host/compositing_iosurface_mac.h diff --git a/content/browser/renderer_host/compositing_iosurface_mac.h b/content/browser/renderer_host/compositing_iosurface_mac.h index 19a04a6e75c7596c99b8abca552d609f57a035af..a65b3bdb87fb8443ea02fbc3d4043e0764e06215 100644 --- a/content/browser/renderer_host/compositing_iosurface_mac.h +++ b/content/browser/renderer_host/compositing_iosurface_mac.h @@ -8,8 +8,10 @@ #import <Cocoa/Cocoa.h> #import <QuartzCore/CVDisplayLink.h> #include <QuartzCore/QuartzCore.h> +#include <map> #include "base/callback.h" +#include "base/lazy_instance.h" #include "base/mac/scoped_cftyperef.h" #include "base/memory/scoped_nsobject.h" #include "base/synchronization/lock.h" @@ -40,14 +42,16 @@ class CompositingIOSurfaceMac { // Passed to Create() to specify the ordering of the surface relative to the // containing window. enum SurfaceOrder { - SURFACE_ORDER_ABOVE_WINDOW, - SURFACE_ORDER_BELOW_WINDOW + SURFACE_ORDER_ABOVE_WINDOW = 0, + SURFACE_ORDER_BELOW_WINDOW = 1, + SURFACE_ORDER_COUNT = 2, }; // Returns NULL if IOSurface support is missing or GL APIs fail. Specify in // |order| the desired ordering relationship of the surface to the containing // window. - static CompositingIOSurfaceMac* Create(SurfaceOrder order); + static CompositingIOSurfaceMac* Create( + int window_number, SurfaceOrder order); ~CompositingIOSurfaceMac(); // Set IOSurface that will be drawn on the next NSView drawRect. @@ -62,7 +66,7 @@ class CompositingIOSurfaceMac { // will be white. |scaleFactor| is 1 in normal views, 2 in HiDPI views. // |frame_subscriber| listens to this draw event and provides output buffer // for copying this frame into. - void DrawIOSurface(NSView* view, float scale_factor, + void DrawIOSurface(NSView* view, float scale_factor, int window_number, RenderWidgetHostViewFrameSubscriber* frame_subscriber); // Copy the data of the "live" OpenGL texture referring to this IOSurfaceRef @@ -106,7 +110,7 @@ class CompositingIOSurfaceMac { // In cocoa view units / DIPs. const gfx::Size& io_surface_size() const { return io_surface_size_; } - bool is_vsync_disabled() const { return is_vsync_disabled_; } + bool is_vsync_disabled() const { return context_data_->is_vsync_disabled(); } // Get vsync scheduling parameters. // |interval_numerator/interval_denominator| equates to fractional number of @@ -205,13 +209,68 @@ class CompositingIOSurfaceMac { base::Callback<base::Closure(void*)> map_buffer_callback; }; + // The GL context that is bound to an NSView when a CompositingIOSurface is to + // drawn into the NSView. One GL context is shared for all + // CompositingIOSurfaces with the same window ordering in the same browser + // window. + class ContextData : public base::RefCounted<ContextData> { + public: + static scoped_refptr<ContextData> Get( + int window_number, SurfaceOrder order); + + int window_number() const { return window_number_; } + SurfaceOrder order() const { return order_; } + NSOpenGLContext* nsgl_context() const { return nsgl_context_; } + CGLContextObj cgl_context() const { return cgl_context_; } + GLuint shader_program_blit_rgb() const { + return shader_program_blit_rgb_; + } + GLint blit_rgb_sampler_location() const { + return blit_rgb_sampler_location_; + } + GLuint shader_program_white() const { + return shader_program_white_; + } + bool is_vsync_disabled() const { return is_vsync_disabled_; } + + private: + friend class base::RefCounted<ContextData>; + ContextData( + int window_number, + SurfaceOrder order, + NSOpenGLContext* nsgl_context, + CGLContextObj cgl_context, + GLuint shader_program_blit_rgb, + GLint blit_rgb_sampler_location, + GLuint shader_program_white, + bool is_vsync_disabled); + virtual ~ContextData(); + + int window_number_; + SurfaceOrder order_; + + // GL context + scoped_nsobject<NSOpenGLContext> nsgl_context_; + CGLContextObj cgl_context_; // weak, backed by |nsgl_context_|. + + // Shader parameters. + GLuint shader_program_blit_rgb_; + GLint blit_rgb_sampler_location_; + GLuint shader_program_white_; + + bool is_vsync_disabled_; + + // The global map from window number and window ordering to + // context data. + typedef std::map<std::pair<int,int>, ContextData*> ContextDataMap; + static base::LazyInstance<ContextDataMap> context_data_map_; + static ContextDataMap* context_data_map() { + return context_data_map_.Pointer(); + } + }; + CompositingIOSurfaceMac(IOSurfaceSupport* io_surface_support, - NSOpenGLContext* glContext, - CGLContextObj cglContext, - GLuint shader_program_blit_rgb, - GLint blit_rgb_sampler_location, - GLuint shader_program_white, - bool is_vsync_disabled, + scoped_refptr<ContextData> context_data, CVDisplayLinkRef display_link); bool IsVendorIntel(); @@ -266,9 +325,9 @@ class CompositingIOSurfaceMac { // Cached pointer to IOSurfaceSupport Singleton. IOSurfaceSupport* io_surface_support_; - // GL context - scoped_nsobject<NSOpenGLContext> glContext_; - CGLContextObj cglContext_; // weak, backed by |glContext_|. + // GL context for drawing and copying. May change when drawing, but is + // never NULL. + scoped_refptr<ContextData> context_data_; // IOSurface data. uint64 io_surface_handle_; @@ -290,15 +349,8 @@ class CompositingIOSurfaceMac { // Timer for finishing a copy operation. base::RepeatingTimer<CompositingIOSurfaceMac> copy_timer_; - // Shader parameters. - GLuint shader_program_blit_rgb_; - GLint blit_rgb_sampler_location_; - GLuint shader_program_white_; - SurfaceQuad quad_; - bool is_vsync_disabled_; - // CVDisplayLink for querying Vsync timing info and throttling swaps. CVDisplayLinkRef display_link_;
This conflicts horribly with https://codereview.chromium.org/12914002/ and basically needs to be rewritten.
Resolved now (added https://codereview.chromium.org/13363002/ as an intermediate). Adding miu to verify that this work with the new structure. This CL will move CompositingIOSurfaces from one GL context to another. I've 1. hung the shaders off of the context object 2. deferred moving from one GL context to another until all copies are complete 3. deleted the transform_ when moving GL contexts In particular, I'd like to very that for 2, copy_requests_ will "most of the time" be empty (like, say, for at least 4 out of every 5 seconds). We transition GL contexts for performance reasons -- it's quite okay if the transition is delayed for a few frames (and a few seconds won't hurt too much, though shorter would be better).
Still LGTM if it doesn't change substantially once https://codereview.chromium.org/13363002/ lands. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_mac.h (right): https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_mac.h:214: // Change context_ to the GL context for view's window, if possible. Does "if possible" mean that this should return a bool?
ccameron@, Thanks for looping me in on these changes. I'm pretty sure they will work just fine with the copy code. We should have testing in-place soon, but for now I'll do some manual testing to check for breakage after you commit. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_context_mac.h (right): https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.h:27: GLint surface_order); Use the SurfaceOrder enum as the type for |surface_order| here, and everywhere else in this patch. FWICT, the only code that needs the -1 or +1 GLint value is CompositingIOSurfaceContext::Get() line 65, so just do the conversion there. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.h:36: GLint gl_surface_order() const { return gl_surface_order_; } Should return SurfaceOrder instead of GLint (see comment above). Or, consider getting rid of this accessor if no code calls it. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.h:65: return window_map_.Pointer(); You should not in-line the window_map() function. LazyInstance::Pointer() is a templated method, and therefore non-trivial code bloat could result everywhere you call window_map(). https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_mac.mm (right): https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_mac.mm:840: bool CompositingIOSurfaceMac::PendingAsynchronousCopiesExist() const { Suggestion: I wouldn't bother adding this method since it is only called from one place. Just do this (in SwitchToContextOnNewWindow()): if (!copy_requests_.empty()) return;
Thanks!! This is a lot better for the cleanup that it conflicted with. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_context_mac.h (right): https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.h:27: GLint surface_order); On 2013/04/01 21:37:37, Yuri wrote: > Use the SurfaceOrder enum as the type for |surface_order| here, and everywhere > else in this patch. FWICT, the only code that needs the -1 or +1 GLint value is > CompositingIOSurfaceContext::Get() line 65, so just do the conversion there. Done (pulled it into the CL this is put on top of). https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.h:36: GLint gl_surface_order() const { return gl_surface_order_; } On 2013/04/01 21:37:37, Yuri wrote: > Should return SurfaceOrder instead of GLint (see comment above). Or, consider > getting rid of this accessor if no code calls it. Done. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.h:65: return window_map_.Pointer(); On 2013/04/01 21:37:37, Yuri wrote: > You should not in-line the window_map() function. LazyInstance::Pointer() is a > templated method, and therefore non-trivial code bloat could result everywhere > you call window_map(). Done. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_mac.h (right): https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_mac.h:214: // Change context_ to the GL context for view's window, if possible. On 2013/04/01 20:18:07, kbr wrote: > Does "if possible" mean that this should return a bool? If this decides not to move the context (or fails to), it just keeps using the old context, which can sometimes cause bad performance (setView stalling makes hitting 60fps hard, so it drops down to 30fps if there are multiple windows). But, because the return value isn't used, I'd prefer to keep it as void. https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_mac.mm (right): https://codereview.chromium.org/13097002/diff/23001/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_mac.mm:840: bool CompositingIOSurfaceMac::PendingAsynchronousCopiesExist() const { On 2013/04/01 21:37:37, Yuri wrote: > Suggestion: I wouldn't bother adding this method since it is only called from > one place. Just do this (in SwitchToContextOnNewWindow()): > > if (!copy_requests_.empty()) > return; Done.
lgtm https://codereview.chromium.org/13097002/diff/23002/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_context_mac.mm (right): https://codereview.chromium.org/13097002/diff/23002/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.mm:27: std::pair<int,int> key = std::make_pair( style nit: need space after comma (between "int,int") And below... https://codereview.chromium.org/13097002/diff/23002/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.mm:128: std::pair<int,GLint> key = std::make_pair( Should be std::pair<int, int>
Thanks! https://codereview.chromium.org/13097002/diff/23002/content/browser/renderer_... File content/browser/renderer_host/compositing_iosurface_context_mac.mm (right): https://codereview.chromium.org/13097002/diff/23002/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.mm:27: std::pair<int,int> key = std::make_pair( On 2013/04/02 00:08:10, Yuri wrote: > style nit: need space after comma (between "int,int") > > And below... Done. https://codereview.chromium.org/13097002/diff/23002/content/browser/renderer_... content/browser/renderer_host/compositing_iosurface_context_mac.mm:128: std::pair<int,GLint> key = std::make_pair( On 2013/04/02 00:08:10, Yuri wrote: > Should be std::pair<int, int> Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/13097002/42001
Failed to apply patch for content/browser/renderer_host/render_widget_host_view_mac.mm: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file content/browser/renderer_host/render_widget_host_view_mac.mm Hunk #1 succeeded at 504 (offset 4 lines). Hunk #2 succeeded at 901 (offset 4 lines). Hunk #3 FAILED at 1050. Hunk #4 succeeded at 1088 (offset 18 lines). Hunk #5 succeeded at 1111 (offset 18 lines). Hunk #6 succeeded at 2297 (offset 18 lines). 1 out of 6 hunks FAILED -- saving rejects to file content/browser/renderer_host/render_widget_host_view_mac.mm.rej Patch: content/browser/renderer_host/render_widget_host_view_mac.mm Index: content/browser/renderer_host/render_widget_host_view_mac.mm 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 0a36a3deda054ba18b86b2bd06836c4558f11e7f..c5d04a23ba49ae2b5d8f1800a3aec79f5ae1125a 100644 --- a/content/browser/renderer_host/render_widget_host_view_mac.mm +++ b/content/browser/renderer_host/render_widget_host_view_mac.mm @@ -500,6 +500,13 @@ void RenderWidgetHostViewMac::release_pepper_fullscreen_window_for_testing() { pepper_fullscreen_window_.reset(); } +int RenderWidgetHostViewMac::window_number() const { + NSWindow* window = [cocoa_view_ window]; + if (!window) + return -1; + return [window windowNumber]; +} + RenderWidgetHost* RenderWidgetHostViewMac::GetRenderWidgetHost() const { return render_widget_host_; } @@ -890,7 +897,7 @@ void RenderWidgetHostViewMac::SetShowingContextMenu(bool showing) { location:location modifierFlags:0 timestamp:0 - windowNumber:[window windowNumber] + windowNumber:window_number() context:nil eventNumber:0 clickCount:0 @@ -1043,7 +1050,7 @@ bool RenderWidgetHostViewMac::CompositorSwapBuffers(uint64 surface_handle, // browsers are likely to crash later for unrelated reasons). // http://crbug.com/148882 NSWindow* window = [cocoa_view_ window]; - if ([window windowNumber] <= 0) { + if (window_number() <= 0) { const char* const kCrashKey = "rwhvm_window"; if (!window) { base::debug::SetCrashKeyValue(kCrashKey, "Missing window"); @@ -1063,7 +1070,8 @@ bool RenderWidgetHostViewMac::CompositorSwapBuffers(uint64 surface_handle, CompositingIOSurfaceMac::SurfaceOrder order = allow_overlapping_views_ ? CompositingIOSurfaceMac::SURFACE_ORDER_BELOW_WINDOW : CompositingIOSurfaceMac::SURFACE_ORDER_ABOVE_WINDOW; - compositing_iosurface_.reset(CompositingIOSurfaceMac::Create(order)); + compositing_iosurface_.reset( + CompositingIOSurfaceMac::Create(window_number(), order)); } if (!compositing_iosurface_.get()) @@ -1085,6 +1093,7 @@ bool RenderWidgetHostViewMac::CompositorSwapBuffers(uint64 surface_handle, if (!about_to_validate_and_paint_) { compositing_iosurface_->DrawIOSurface(cocoa_view_, ScaleFactor(cocoa_view_), + window_number(), frame_subscriber_.get()); } return true; @@ -2270,7 +2279,10 @@ gfx::Rect RenderWidgetHostViewMac::GetScaledOpenGLPixelRect( } renderWidgetHostView_->compositing_iosurface_->DrawIOSurface( - self, ScaleFactor(self), renderWidgetHostView_->frame_subscriber()); + self, + ScaleFactor(self), + renderWidgetHostView_->window_number(), + renderWidgetHostView_->frame_subscriber()); return; }
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/13097002/48001
Message was sent while issue was closed.
Change committed as 192111 |