diff options
author | ccameron@chromium.org <ccameron@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-02 01:40:18 +0000 |
---|---|---|
committer | ccameron@chromium.org <ccameron@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-02 01:40:18 +0000 |
commit | 5ca3c2c9c7bed1cfe7cdf2413893e40dab56d718 (patch) | |
tree | 510c6c2635dac593481d4804cb260b034972815f | |
parent | d013efc32e29077d3e85b007546c3a65039ae3c2 (diff) | |
download | chromium_src-5ca3c2c9c7bed1cfe7cdf2413893e40dab56d718.zip chromium_src-5ca3c2c9c7bed1cfe7cdf2413893e40dab56d718.tar.gz chromium_src-5ca3c2c9c7bed1cfe7cdf2413893e40dab56d718.tar.bz2 |
Fix browser test bugs with delegated rendering on Mac
Add a delay between requesting a snapshot and actually taking the
snapshot. This is the previously-encountered issue where the snapshot
mechanism lags CALayer content updates by an unspecified amount.
Fix places where defined(USE_AURA) was used to check if delegated
rendering is enabled. In particular, fix places where this was used to
determine if extra error should be given for tab capture tests, and some
places where this was used to determine if ImageTransportSurface should
be initialized.
Add a destroyCompositor method to BrowserCompositorViewCocoa. We
don't precisely control when BrowserCompositorViewCocoa are deallocated,
but we need to precisely control when the ui::Compositor is destroyed.
Use a separate BrowserCompositorViewPlaceholderMac to control the
lifetime of the recyclable BrowserCompositorViewCocoa, rather than
using the scheme where an "invalid" BrowserCompositorViewMac is used
as a placeholder.
BUG=314190
NOTRY=True (passed Mac bots)
Review URL: https://codereview.chromium.org/358403003
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@280936 0039d316-1c4b-4281-b951-d872f2087c98
10 files changed, 152 insertions, 79 deletions
diff --git a/content/browser/compositor/browser_compositor_view_mac.h b/content/browser/compositor/browser_compositor_view_mac.h index c04c57c..0dc358b 100644 --- a/content/browser/compositor/browser_compositor_view_mac.h +++ b/content/browser/compositor/browser_compositor_view_mac.h @@ -63,21 +63,9 @@ class BrowserCompositorViewMac { public: // This will install the NSView which is drawn by the ui::Compositor into // the NSView provided by the client. - static BrowserCompositorViewMac* CreateForClient( - BrowserCompositorViewMacClient* client); - - // This is used as a placeholder to indicate that the owner may want a full - // instance soon. One recycled instance of BrowserCompositorViewCocoa will - // be kept around as long as a non-zero number of invalid instances are - // present. - static BrowserCompositorViewMac* CreateInvalid(); - + explicit BrowserCompositorViewMac(BrowserCompositorViewMacClient* client); ~BrowserCompositorViewMac(); - // Returns true if this was created with CreateForClient, as opposed to - // CreateInvalid. - bool IsValid() const { return client_; } - // The ui::Compositor being used to render the NSView. ui::Compositor* GetCompositor() const; @@ -85,12 +73,20 @@ class BrowserCompositorViewMac { BrowserCompositorViewMacClient* GetClient() const { return client_; } private: - BrowserCompositorViewMac(BrowserCompositorViewMacClient* client); - BrowserCompositorViewMacClient* client_; base::scoped_nsobject<BrowserCompositorViewCocoa> cocoa_view_; }; +// A class to keep around whenever a BrowserCompositorViewMac may be created. +// While at least one instance of this class exists, a spare +// BrowserCompositorViewCocoa will be kept around to be recycled so that the +// next BrowserCompositorViewMac to be created will be be created quickly. +class BrowserCompositorViewPlaceholderMac { + public: + BrowserCompositorViewPlaceholderMac(); + ~BrowserCompositorViewPlaceholderMac(); +}; + } // namespace content #endif // CONTENT_BROWSER_COMPOSITOR_BROWSER_COMPOSITOR_VIEW_MAC_H_ diff --git a/content/browser/compositor/browser_compositor_view_mac.mm b/content/browser/compositor/browser_compositor_view_mac.mm index 26806d6..2151c63 100644 --- a/content/browser/compositor/browser_compositor_view_mac.mm +++ b/content/browser/compositor/browser_compositor_view_mac.mm @@ -38,10 +38,10 @@ namespace content { namespace { -// The number of invalid BrowserCompositorViewMac objects. If this reaches -// zero, then the BrowserCompositorViewCocoa being held on to for recycling, +// The number of placeholder objects allocated. If this reaches zero, then +// the BrowserCompositorViewCocoa being held on to for recycling, // |g_recyclable_cocoa_view|, will be freed. -uint32 g_invalid_view_count = 0; +uint32 g_placeholder_count = 0; // A spare BrowserCompositorViewCocoa kept around for recycling. base::LazyInstance<base::scoped_nsobject<BrowserCompositorViewCocoa>> @@ -49,47 +49,30 @@ base::LazyInstance<base::scoped_nsobject<BrowserCompositorViewCocoa>> } // namespace -BrowserCompositorViewMac* BrowserCompositorViewMac::CreateForClient( - BrowserCompositorViewMacClient* client) { - return new BrowserCompositorViewMac(client); -} - -BrowserCompositorViewMac* BrowserCompositorViewMac::CreateInvalid() { - return new BrowserCompositorViewMac(NULL); -} - BrowserCompositorViewMac::BrowserCompositorViewMac( BrowserCompositorViewMacClient* client) : client_(client) { - if (client_) { - // Try to use the recyclable BrowserCompositorViewMac if there is one, - // otherwise allocate a new one. - // TODO(ccameron): If there exists a frame in flight (swap has been called - // by the compositor, but the frame has not arrived from the GPU process - // yet), then that frame may inappropriately flash in the new view. - swap(g_recyclable_cocoa_view.Get(), cocoa_view_); - if (!cocoa_view_) - cocoa_view_.reset([[BrowserCompositorViewCocoa alloc] init]); - [cocoa_view_ setClient:client_]; - } else { - g_invalid_view_count += 1; - } + // Try to use the recyclable BrowserCompositorViewCocoa if there is one, + // otherwise allocate a new one. + // TODO(ccameron): If there exists a frame in flight (swap has been called + // by the compositor, but the frame has not arrived from the GPU process + // yet), then that frame may inappropriately flash in the new view. + swap(g_recyclable_cocoa_view.Get(), cocoa_view_); + if (!cocoa_view_) + cocoa_view_.reset([[BrowserCompositorViewCocoa alloc] init]); + [cocoa_view_ setClient:client_]; } BrowserCompositorViewMac::~BrowserCompositorViewMac() { - if (client_) { - [cocoa_view_ setClient:NULL]; - // If there are invalid views that may want need a - // BrowserCompositorViewCocoa momentarily, then allow the - // BrowserCompositorViewCocoa to be recycled. Otherwise, discard it. - if (g_invalid_view_count) - swap(g_recyclable_cocoa_view.Get(), cocoa_view_); - } else { - // If this is the last invalid view going away, then discard the - // BrowserCompositorViewCocoa that was being saved to recycle. - DCHECK_GT(g_invalid_view_count, 0u); - g_invalid_view_count -= 1; - if (!g_invalid_view_count) - g_recyclable_cocoa_view.Get().reset(); + // Make this BrowserCompositorViewCocoa recyclable for future instances. + [cocoa_view_ setClient:NULL]; + [g_recyclable_cocoa_view.Get() destroyCompositor]; + swap(g_recyclable_cocoa_view.Get(), cocoa_view_); + + // If there are no placeholders allocated, destroy the recyclable + // BrowserCompositorViewCocoa that we just populated. + if (!g_placeholder_count) { + [g_recyclable_cocoa_view.Get() destroyCompositor]; + g_recyclable_cocoa_view.Get().reset(); } } @@ -98,4 +81,23 @@ ui::Compositor* BrowserCompositorViewMac::GetCompositor() const { return [cocoa_view_ compositor]; } +//////////////////////////////////////////////////////////////////////////////// +// BrowserCompositorViewPlaceholderMac + +BrowserCompositorViewPlaceholderMac::BrowserCompositorViewPlaceholderMac() { + g_placeholder_count += 1; +} + +BrowserCompositorViewPlaceholderMac::~BrowserCompositorViewPlaceholderMac() { + DCHECK_GT(g_placeholder_count, 0u); + g_placeholder_count -= 1; + + // If there are no placeholders allocated, destroy the recyclable + // BrowserCompositorViewCocoa. + if (!g_placeholder_count) { + [g_recyclable_cocoa_view.Get() destroyCompositor]; + g_recyclable_cocoa_view.Get().reset(); + } +} + } // namespace content diff --git a/content/browser/compositor/browser_compositor_view_private_mac.h b/content/browser/compositor/browser_compositor_view_private_mac.h index 36268ee..a6e998c 100644 --- a/content/browser/compositor/browser_compositor_view_private_mac.h +++ b/content/browser/compositor/browser_compositor_view_private_mac.h @@ -27,9 +27,14 @@ class BrowserCompositorViewCocoaHelper; scoped_ptr<content::BrowserCompositorViewCocoaHelper> helper_; } -// Change the client and superview of the view. +// Change the client and superview of the view. If this is set to NULL then +// the compositor will be prepared to be recycled. - (void)setClient:(content::BrowserCompositorViewMacClient*)client; +// This is called to destroy the underlying ui::Compositor, if it is known +// that this will not be recycled again. +- (void)destroyCompositor; + // Access the underlying ui::Compositor for this view. - (ui::Compositor*)compositor; diff --git a/content/browser/compositor/browser_compositor_view_private_mac.mm b/content/browser/compositor/browser_compositor_view_private_mac.mm index 385b311..97914d7 100644 --- a/content/browser/compositor/browser_compositor_view_private_mac.mm +++ b/content/browser/compositor/browser_compositor_view_private_mac.mm @@ -54,6 +54,7 @@ client_ = client; if (client_) { + DCHECK(compositor_); compositor_->SetRootLayer(client_->BrowserCompositorRootLayer()); [client_->BrowserCompositorSuperview() addSubview:self]; } else { @@ -61,6 +62,11 @@ } } +- (void)destroyCompositor { + DCHECK(!client_); + compositor_.reset(); +} + - (void)gotAcceleratedLayerError { if (!accelerated_layer_) return; diff --git a/content/browser/renderer_host/render_widget_host_unittest.cc b/content/browser/renderer_host/render_widget_host_unittest.cc index 0e6a9d1..9eb23e4 100644 --- a/content/browser/renderer_host/render_widget_host_unittest.cc +++ b/content/browser/renderer_host/render_widget_host_unittest.cc @@ -9,6 +9,7 @@ #include "base/memory/shared_memory.h" #include "base/timer/timer.h" #include "content/browser/browser_thread_impl.h" +#include "content/browser/gpu/compositor_util.h" #include "content/browser/renderer_host/input/input_router_impl.h" #include "content/browser/renderer_host/render_widget_host_delegate.h" #include "content/browser/renderer_host/render_widget_host_view_base.h" @@ -28,13 +29,16 @@ #include "content/browser/renderer_host/render_widget_host_view_android.h" #endif -#if defined(USE_AURA) +#if defined(USE_AURA) || (defined(OS_MACOSX) && !defined(OS_IOS)) #include "content/browser/compositor/image_transport_factory.h" +#include "ui/compositor/test/in_process_context_factory.h" +#endif + +#if defined(USE_AURA) #include "content/browser/renderer_host/render_widget_host_view_aura.h" #include "content/browser/renderer_host/ui_events_helper.h" #include "ui/aura/env.h" #include "ui/aura/test/test_screen.h" -#include "ui/compositor/test/in_process_context_factory.h" #include "ui/events/event.h" #endif @@ -450,9 +454,13 @@ class RenderWidgetHostTest : public testing::Test { browser_context_.reset(new TestBrowserContext()); delegate_.reset(new MockRenderWidgetHostDelegate()); process_ = new RenderWidgetHostProcess(browser_context_.get()); +#if defined(USE_AURA) || (defined(OS_MACOSX) && !defined(OS_IOS)) + if (IsDelegatedRendererEnabled()) { + ImageTransportFactory::InitializeForUnitTests( + scoped_ptr<ui::ContextFactory>(new ui::InProcessContextFactory)); + } +#endif #if defined(USE_AURA) - ImageTransportFactory::InitializeForUnitTests( - scoped_ptr<ui::ContextFactory>(new ui::InProcessContextFactory)); aura::Env::CreateInstance(true); screen_.reset(aura::TestScreen::Create(gfx::Size())); gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, screen_.get()); @@ -474,7 +482,10 @@ class RenderWidgetHostTest : public testing::Test { #if defined(USE_AURA) aura::Env::DeleteInstance(); screen_.reset(); - ImageTransportFactory::Terminate(); +#endif +#if defined(USE_AURA) || (defined(OS_MACOSX) && !defined(OS_IOS)) + if (IsDelegatedRendererEnabled()) + ImageTransportFactory::Terminate(); #endif // Process all pending tasks to avoid leaks. diff --git a/content/browser/renderer_host/render_widget_host_view_browsertest.cc b/content/browser/renderer_host/render_widget_host_view_browsertest.cc index 4dfd18c..302f4fe 100644 --- a/content/browser/renderer_host/render_widget_host_view_browsertest.cc +++ b/content/browser/renderer_host/render_widget_host_view_browsertest.cc @@ -620,16 +620,16 @@ class CompositingRenderWidgetHostViewBrowserTestTabCapture video_frame, callback); } else { -#if defined(USE_AURA) - if (!content::GpuDataManager::GetInstance() - ->CanUseGpuBrowserCompositor()) { - // Skia rendering can cause color differences, particularly in the - // middle two columns. - SetAllowableError(2); - SetExcludeRect( - gfx::Rect(output_size.width() / 2 - 1, 0, 2, output_size.height())); + if (IsDelegatedRendererEnabled()) { + if (!content::GpuDataManager::GetInstance() + ->CanUseGpuBrowserCompositor()) { + // Skia rendering can cause color differences, particularly in the + // middle two columns. + SetAllowableError(2); + SetExcludeRect(gfx::Rect( + output_size.width() / 2 - 1, 0, 2, output_size.height())); + } } -#endif base::Callback<void(bool, const SkBitmap&)> callback = base::Bind(&CompositingRenderWidgetHostViewBrowserTestTabCapture:: diff --git a/content/browser/renderer_host/render_widget_host_view_mac.h b/content/browser/renderer_host/render_widget_host_view_mac.h index 926788d..be2f289 100644 --- a/content/browser/renderer_host/render_widget_host_view_mac.h +++ b/content/browser/renderer_host/render_widget_host_view_mac.h @@ -439,10 +439,15 @@ class CONTENT_EXPORT RenderWidgetHostViewMac scoped_ptr<DelegatedFrameHost> delegated_frame_host_; scoped_ptr<ui::Layer> root_layer_; - // Container for the NSView drawn by the browser compositor. This will never - // be NULL, but may be invalid (see its IsValid method). + // Container for the NSView drawn by the browser compositor. scoped_ptr<BrowserCompositorViewMac> browser_compositor_view_; + // Placeholder that is allocated while browser_compositor_view_ is NULL, + // indicating that a BrowserCompositorViewMac may be allocated. This is to + // help in recycling the internals of BrowserCompositorViewMac. + scoped_ptr<BrowserCompositorViewPlaceholderMac> + browser_compositor_view_placeholder_; + // This holds the current software compositing framebuffer, if any. scoped_ptr<SoftwareFrameManager> software_frame_manager_; 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 add6807..876fd52 100644 --- a/content/browser/renderer_host/render_widget_host_view_mac.mm +++ b/content/browser/renderer_host/render_widget_host_view_mac.mm @@ -398,7 +398,7 @@ namespace content { // DelegatedFrameHost, public: ui::Compositor* RenderWidgetHostViewMac::GetCompositor() const { - if (browser_compositor_view_->IsValid()) + if (browser_compositor_view_) return browser_compositor_view_->GetCompositor(); return NULL; } @@ -413,7 +413,7 @@ RenderWidgetHostImpl* RenderWidgetHostViewMac::GetHost() { void RenderWidgetHostViewMac::SchedulePaintInRect( const gfx::Rect& damage_rect_in_dip) { - if (browser_compositor_view_->IsValid()) + if (browser_compositor_view_) browser_compositor_view_->GetCompositor()->ScheduleFullRedraw(); } @@ -454,6 +454,30 @@ void RenderWidgetHostViewMac::BrowserCompositorViewFrameSwapped( if (!render_widget_host_) return; + // If the latency info is requesting a screenshot, wait a fixed delay of + // 1/6th of a second (simulating 10 60fps frames) before reporting that the + // frame was received to the browser. This is to allow the changes to flush + // through to the window server before screenshots are taken. + bool should_defer = false; + for (auto latency_info : all_latency_info) { + if (latency_info.FindLatency( + ui::WINDOW_SNAPSHOT_FRAME_NUMBER_COMPONENT, + render_widget_host_->GetLatencyComponentId(), + NULL)) { + should_defer = true; + } + } + if (should_defer) { + base::MessageLoop::current()->PostDelayedTask( + FROM_HERE, + base::Bind(&RenderWidgetHostViewMac::SendPendingLatencyInfoToHost, + weak_factory_.GetWeakPtr()), + 10 * base::TimeDelta::FromSecondsD(1. / 60)); + pending_latency_info_.insert(pending_latency_info_.end(), + all_latency_info.begin(), all_latency_info.end()); + return; + } + for (auto latency_info : all_latency_info) { latency_info.AddLatencyNumber( ui::INPUT_EVENT_LATENCY_TERMINATED_FRAME_SWAP_COMPONENT, 0, 0); @@ -485,7 +509,8 @@ RenderWidgetHostViewMac::RenderWidgetHostViewMac(RenderWidgetHost* widget) : render_widget_host_(RenderWidgetHostImpl::From(widget)), text_input_type_(ui::TEXT_INPUT_TYPE_NONE), can_compose_inline_(true), - browser_compositor_view_(BrowserCompositorViewMac::CreateInvalid()), + browser_compositor_view_placeholder_( + new BrowserCompositorViewPlaceholderMac), pending_latency_info_delay_(0), pending_latency_info_delay_weak_ptr_factory_(this), backing_store_scale_factor_(1), @@ -606,14 +631,13 @@ bool RenderWidgetHostViewMac::EnsureCompositedIOSurface() { void RenderWidgetHostViewMac::EnsureBrowserCompositorView() { if (!delegated_frame_host_) return; - if (browser_compositor_view_->IsValid()) + if (browser_compositor_view_) return; TRACE_EVENT0("browser", "RenderWidgetHostViewMac::EnsureBrowserCompositorView"); - browser_compositor_view_.reset(BrowserCompositorViewMac::CreateForClient( - this)); + browser_compositor_view_.reset(new BrowserCompositorViewMac(this)); delegated_frame_host_->AddedToWindow(); delegated_frame_host_->WasShown(); } @@ -621,12 +645,12 @@ void RenderWidgetHostViewMac::EnsureBrowserCompositorView() { void RenderWidgetHostViewMac::DestroyBrowserCompositorView() { TRACE_EVENT0("browser", "RenderWidgetHostViewMac::DestroyBrowserCompositorView"); - if (!browser_compositor_view_->IsValid()) + if (!browser_compositor_view_) return; delegated_frame_host_->WasHidden(); delegated_frame_host_->RemovingFromWindow(); - browser_compositor_view_.reset(BrowserCompositorViewMac::CreateInvalid()); + browser_compositor_view_.reset(); } void RenderWidgetHostViewMac::EnsureSoftwareLayer() { @@ -1086,6 +1110,7 @@ void RenderWidgetHostViewMac::Destroy() { DestroyBrowserCompositorView(); delegated_frame_host_.reset(); root_layer_.reset(); + browser_compositor_view_placeholder_.reset(); // We get this call just before |render_widget_host_| deletes // itself. But we are owned by |cocoa_view_|, which may be retained diff --git a/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm b/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm index 94f499e..f7ff91a 100644 --- a/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm +++ b/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm @@ -8,6 +8,8 @@ #include "base/mac/scoped_nsautorelease_pool.h" #include "base/message_loop/message_loop.h" +#include "content/browser/compositor/image_transport_factory.h" +#include "content/browser/gpu/compositor_util.h" #include "content/browser/renderer_host/render_widget_host_delegate.h" #include "content/browser/renderer_host/render_widget_host_impl.h" #include "content/common/input_messages.h" @@ -17,6 +19,7 @@ #include "testing/gtest/include/gtest/gtest.h" #include "testing/platform_test.h" #include "ui/base/layout.h" +#include "ui/compositor/test/in_process_context_factory.h" using content::RenderWidgetHostViewMac; @@ -94,6 +97,17 @@ class RenderWidgetHostEditCommandCounter : public RenderWidgetHostImpl { }; class RenderWidgetHostViewMacEditCommandHelperTest : public PlatformTest { + protected: + virtual void SetUp() { + if (IsDelegatedRendererEnabled()) { + ImageTransportFactory::InitializeForUnitTests( + scoped_ptr<ui::ContextFactory>(new ui::InProcessContextFactory)); + } + } + virtual void TearDown() { + if (IsDelegatedRendererEnabled()) + ImageTransportFactory::Terminate(); + } }; } // namespace diff --git a/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm b/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm index da766ea..ff20a5d 100644 --- a/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm +++ b/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm @@ -9,6 +9,8 @@ #include "base/mac/sdk_forward_declarations.h" #include "base/strings/utf_string_conversions.h" #include "content/browser/browser_thread_impl.h" +#include "content/browser/compositor/image_transport_factory.h" +#include "content/browser/gpu/compositor_util.h" #include "content/browser/renderer_host/render_widget_host_delegate.h" #include "content/common/gpu/gpu_messages.h" #include "content/common/input_messages.h" @@ -21,6 +23,7 @@ #include "content/test/test_render_view_host.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +#include "ui/compositor/test/in_process_context_factory.h" #include "ui/events/test/cocoa_test_event_utils.h" #import "ui/gfx/test/ui_cocoa_test_helper.h" @@ -165,6 +168,10 @@ class RenderWidgetHostViewMacTest : public RenderViewHostImplTestHarness { virtual void SetUp() { RenderViewHostImplTestHarness::SetUp(); + if (IsDelegatedRendererEnabled()) { + ImageTransportFactory::InitializeForUnitTests( + scoped_ptr<ui::ContextFactory>(new ui::InProcessContextFactory)); + } // TestRenderViewHost's destruction assumes that its view is a // TestRenderWidgetHostView, so store its view and reset it back to the @@ -185,6 +192,8 @@ class RenderWidgetHostViewMacTest : public RenderViewHostImplTestHarness { // See comment in SetUp(). test_rvh()->SetView(static_cast<RenderWidgetHostViewBase*>(old_rwhv_)); + if (IsDelegatedRendererEnabled()) + ImageTransportFactory::Terminate(); RenderViewHostImplTestHarness::TearDown(); } protected: |