gpu: media: RenderingHelper: wait for the display & window to be ready

content/common/gpu/media/rendering_helper.cc

Index: content/common/gpu/media/rendering_helper.cc
diff --git a/content/common/gpu/media/rendering_helper.cc b/content/common/gpu/media/rendering_helper.cc
index 950ae6e71dd07e3eb0a41739e695cab20a9e4825..46f8ffb75cef71fc700cde8e2df1a43fa090a0eb 100644
--- a/content/common/gpu/media/rendering_helper.cc
+++ b/content/common/gpu/media/rendering_helper.cc
@@ -13,6 +13,7 @@
 #include "base/command_line.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/stringize_macros.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/time/time.h"
@@ -39,6 +40,7 @@
 #if defined(USE_OZONE)
 #if defined(OS_CHROMEOS)
 #include "ui/display/chromeos/display_configurator.h"
+#include "ui/display/types/display_constants.h"
 #endif  // defined(OS_CHROMEOS)
 #include "ui/ozone/public/ozone_platform.h"
 #include "ui/ozone/public/ui_thread_gpu.h"
@@ -70,16 +72,46 @@ namespace content {
 
 #if defined(USE_OZONE)
 
+class DisplayConfiguratorObserver : public ui::DisplayConfigurator::Observer {
+ public:
+  DisplayConfiguratorObserver(base::RunLoop* loop) : loop_(loop) {}
+  ~DisplayConfiguratorObserver() override {}
+
+ private:
+  // ui::DisplayConfigurator::Observer overrides:
+  void OnDisplayModeChanged(
+      const ui::DisplayConfigurator::DisplayStateList& outputs) override {
+    if (!loop_)
+      return;
+    loop_->Quit();
+    loop_ = nullptr;
+  }
+  void OnDisplayModeChangeFailed(
+      ui::MultipleDisplayState failed_new_state) override {
+    LOG(FATAL) << "Could not configure display";
+  }
+
+  base::RunLoop* loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(DisplayConfiguratorObserver);
+};
+
 class RenderingHelper::StubOzoneDelegate : public ui::PlatformWindowDelegate {
  public:
-  StubOzoneDelegate() : accelerated_widget_(gfx::kNullAcceleratedWidget) {
+  StubOzoneDelegate(base::RunLoop* loop)
+      : accelerated_widget_(gfx::kNullAcceleratedWidget), loop_(loop) {
     ui_thread_gpu_.Initialize();
     platform_window_ = ui::OzonePlatform::GetInstance()->CreatePlatformWindow(
         this, gfx::Rect());
   }
   virtual ~StubOzoneDelegate() {}
 
-  void OnBoundsChanged(const gfx::Rect& new_bounds) override {}
+  void OnBoundsChanged(const gfx::Rect& new_bounds) override {
+    if (!loop_)
+      return;
+    loop_->Quit();
+    loop_ = nullptr;
+  }
 
   void OnDamageRect(const gfx::Rect& damaged_region) override {}
 
@@ -110,6 +142,7 @@ class RenderingHelper::StubOzoneDelegate : public ui::PlatformWindowDelegate {
   ui::UiThreadGpu ui_thread_gpu_;
   scoped_ptr<ui::PlatformWindow> platform_window_;
   gfx::AcceleratedWidget accelerated_widget_;
+  base::RunLoop* loop_;
 
   DISALLOW_COPY_AND_ASSIGN(StubOzoneDelegate);
 };
@@ -167,26 +200,7 @@ RenderingHelper::~RenderingHelper() {
   Clear();
 }
 
-void RenderingHelper::Initialize(const RenderingHelperParams& params,
-                                 base::WaitableEvent* done) {
-  // Use videos_.size() != 0 as a proxy for the class having already been
-  // Initialize()'d, and UnInitialize() before continuing.
-  if (videos_.size()) {
-    base::WaitableEvent done(false, false);
-    UnInitialize(&done);
-    done.Wait();
-  }
-
-  render_task_.Reset(
-      base::Bind(&RenderingHelper::RenderContent, base::Unretained(this)));
-
-  frame_duration_ = params.rendering_fps > 0
-                        ? base::TimeDelta::FromSeconds(1) / params.rendering_fps
-                        : base::TimeDelta();
-
-  render_as_thumbnails_ = params.render_as_thumbnails;
-  message_loop_ = base::MessageLoop::current();
-
+void RenderingHelper::Setup() {
 #if defined(OS_WIN)
   window_ = CreateWindowEx(0,
                            L"Static",
@@ -229,21 +243,77 @@ void RenderingHelper::Initialize(const RenderingHelperParams& params,
   XSelectInput(display, window_, ExposureMask);
   XMapWindow(display, window_);
 #elif defined(USE_OZONE)
-  platform_window_delegate_.reset(new RenderingHelper::StubOzoneDelegate());
+  base::MessageLoop::ScopedNestableTaskAllower nest_loop(
+      base::MessageLoop::current());
+  base::RunLoop wait_window_resize;
+
+  platform_window_delegate_.reset(
+      new RenderingHelper::StubOzoneDelegate(&wait_window_resize));
   window_ = platform_window_delegate_->accelerated_widget();
 #if defined(OS_CHROMEOS)
+  // We hold onto the main loop here to wait for the DisplayController
+  // to give us the size of the display so we can create a window of
+  // the same size.
+  base::RunLoop wait_display_setup;
+  DisplayConfiguratorObserver observer(&wait_display_setup);
   display_configurator_.reset(new ui::DisplayConfigurator());
+  display_configurator_->AddObserver(&observer);
   display_configurator_->Init(true);
   display_configurator_->ForceInitialConfigure(0);
+  // Make sure all the display configuration is applied.
+  wait_display_setup.Run();
+  display_configurator_->RemoveObserver(&observer);
   platform_window_delegate_->platform_window()->SetBounds(
       gfx::Rect(display_configurator_->framebuffer_size()));
 #else
   platform_window_delegate_->platform_window()->SetBounds(gfx::Rect(800, 600));
 #endif
+  wait_window_resize.Run();

[dnicoara, 2015/01/22 19:23:17]

I'm not fine with adding more complexity to platform implementations. This can
be replaced with base::RunLoop().RunUntilIdle() to flush all the pending updates
without adding extra complexity to the platform code.

[Owen Lin, 2015/01/23 08:57:38]

I think you're talking the other CL (859943004). But correctness should be
considered before simplicity. 

Anyway, to prevent the dilemma, how about just signaling done in the end of the
function. So we don't need to wait for VSYNC info here. The current
rendering_helper has been modified to live without vsync provider. We will try
to get the VSync info when rendering a frame later.

The drawback is we will never know whether we get the VSYNC or not. I hope we
can get a performance alert in that case.

[dnicoara, 2015/01/23 15:10:13]

The current approach is correct. Platforms have 2 choices:
 1) If they use their own message loop (or connection) they need to provide
their own flushing primitives and those would need to be explicitly called from
functions such as SetBounds().
 2) If they use the built-in message loop, then they have to rely on running the
message loop to achieve the same effect. Think of base::RunLoop().RunUntilIdle()
as a flush.

Note, "wait_window_resize.Run()" essentially does the same thing.

Let me know if I should expand, I'm happy to provide more details.

[Owen Lin, 2015/01/26 07:25:28]

Thanks for the explanation. I am kind of understanding it. It makes sure all
these setup commands have been flushed to the hardware (or something) before the
vsync command.

I think the basic issue is we don't understand what we need to make
VSyncProvider works.

 #else
 #error unknown platform
 #endif
   CHECK(window_ != gfx::kNullAcceleratedWidget);
+}
+
+void RenderingHelper::TearDown() {
+#if defined(OS_WIN)
+  if (window_)
+    DestroyWindow(window_);
+#elif defined(USE_X11)
+  // Destroy resources acquired in Initialize, in reverse-acquisition order.
+  if (window_) {
+    CHECK(XUnmapWindow(gfx::GetXDisplay(), window_));
+    CHECK(XDestroyWindow(gfx::GetXDisplay(), window_));
+  }
+#elif defined(USE_OZONE)
+  platform_window_delegate_.reset();
+#if defined(OS_CHROMEOS)
+  display_configurator_->PrepareForExit();
+  display_configurator_.reset();
+#endif
+#endif
+  window_ = gfx::kNullAcceleratedWidget;
+}
+
+void RenderingHelper::Initialize(const RenderingHelperParams& params,
+                                 base::WaitableEvent* done) {
+  // Use videos_.size() != 0 as a proxy for the class having already been
+  // Initialize()'d, and UnInitialize() before continuing.
+  if (videos_.size()) {
+    base::WaitableEvent done(false, false);
+    UnInitialize(&done);
+    done.Wait();
+  }
+
+  render_task_.Reset(
+      base::Bind(&RenderingHelper::RenderContent, base::Unretained(this)));
+
+  frame_duration_ = params.rendering_fps > 0
+                        ? base::TimeDelta::FromSeconds(1) / params.rendering_fps
+                        : base::TimeDelta();
+
+  render_as_thumbnails_ = params.render_as_thumbnails;
+  message_loop_ = base::MessageLoop::current();
 
   gl_surface_ = gfx::GLSurface::CreateViewGLSurface(window_);

[dnicoara, 2015/01/22 19:23:17]

While reading more of this code I realized this is also broken.

GLSurface::InitializeOneOff() is called from the main thread while this is
called from the renderer_helper thread. This violates threading assumptions in
Ozone (and X11).

Though, given that this review is fixing a M41 beta blocker bug, perhaps just
file a bug against this to have it fixed later.

   screen_size_ = gl_surface_->GetSize();
@@ -429,11 +499,6 @@ void RenderingHelper::WarmUpRendering(int warm_up_iterations) {
 void RenderingHelper::UnInitialize(base::WaitableEvent* done) {
   CHECK_EQ(base::MessageLoop::current(), message_loop_);
 
-#if defined(USE_OZONE) && defined(OS_CHROMEOS)
-  display_configurator_->PrepareForExit();
-  display_configurator_.reset();
-#endif
-
   render_task_.Cancel();
 
   if (render_as_thumbnails_) {
@@ -578,20 +643,6 @@ void RenderingHelper::Clear() {
   frame_count_ = 0;
   thumbnails_fbo_id_ = 0;
   thumbnails_texture_id_ = 0;
-
-#if defined(OS_WIN)
-  if (window_)
-    DestroyWindow(window_);
-#elif defined(USE_X11)
-  // Destroy resources acquired in Initialize, in reverse-acquisition order.
-  if (window_) {
-    CHECK(XUnmapWindow(gfx::GetXDisplay(), window_));
-    CHECK(XDestroyWindow(gfx::GetXDisplay(), window_));
-  }
-#elif defined(USE_OZONE)
-  platform_window_delegate_.reset();
-#endif
-  window_ = gfx::kNullAcceleratedWidget;
 }
 
 void RenderingHelper::GetThumbnailsAsRGB(std::vector<unsigned char>* rgb,