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..60c0ea4c048e7dce01378ad2cef29f10cda29acd 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,36 @@ 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 {
+    loop_->Quit();
+  }
+  void OnDisplayModeChangeFailed(
+      ui::MultipleDisplayState failed_new_state) override {}

[Owen Lin, 2015/01/21 08:10:03]

Crash the program here ?

LOG(FATAL) 

[llandwerlin-old, 2015/01/21 08:44:19]

Acknowledged.

+
+  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 { loop_->Quit(); }

[Pawel Osciak, 2015/01/21 08:13:35]

Does this pass git cl format?

[llandwerlin-old, 2015/01/21 08:44:19]

Yes.

 
   void OnDamageRect(const gfx::Rect& damaged_region) override {}
 
@@ -110,6 +132,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);
 };
@@ -229,17 +252,35 @@ void RenderingHelper::Initialize(const RenderingHelperParams& params,
   XSelectInput(display, window_, ExposureMask);
   XMapWindow(display, window_);
 #elif defined(USE_OZONE)
-  platform_window_delegate_.reset(new RenderingHelper::StubOzoneDelegate());
+  scoped_ptr<base::RunLoop> window_loop(new base::RunLoop);

[Owen Lin, 2015/01/21 08:10:03]

Can we just use a local variable?
base::RunLoop window_loop;

Same for others.

How about renaming it to wait_resize_loop here and in the StubOzoneDelegate to
show the only purpose of the loop.

[llandwerlin-old, 2015/01/21 08:44:19]

Acknowledged.

+
+  platform_window_delegate_.reset(
+      new RenderingHelper::StubOzoneDelegate(window_loop.get()));

[Pawel Osciak, 2015/01/21 08:13:35]

Is it safe to pass a local variable pointer to the class member here?

[llandwerlin-old, 2015/01/21 08:44:19]

Thanks, I should reset the class member after the loop is stop.

   window_ = platform_window_delegate_->accelerated_widget();
 #if defined(OS_CHROMEOS)
-  display_configurator_.reset(new ui::DisplayConfigurator());
+  // 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::MessageLoop::ScopedNestableTaskAllower nest_loop(
+      base::MessageLoop::current());
+  scoped_ptr<base::RunLoop> display_loop(new base::RunLoop);

[Owen Lin, 2015/01/21 08:10:03]

Same. How about renaming it to wait_mode_change_loop.

[Pawel Osciak, 2015/01/21 08:13:35]

(new base::RunLoop());

+  scoped_ptr<DisplayConfiguratorObserver> display_observer(
+      new DisplayConfiguratorObserver(display_loop.get()));
+  display_configurator_.reset(new ui::DisplayConfigurator);
+  display_configurator_->AddObserver(display_observer.get());
   display_configurator_->Init(true);
   display_configurator_->ForceInitialConfigure(0);
+  display_configurator_->SetDisplayMode(ui::MULTIPLE_DISPLAY_STATE_SINGLE);

[Owen Lin, 2015/01/21 08:10:03]

Are we sure the "OnDisplayModeChanged" would be called? What if the current mode
is SINGLE already.

[llandwerlin-old, 2015/01/21 08:44:19]

As far as I can tell the ForceInitialConfigure will always notify the observers
:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/display/chromeo...

+  // Wait for the display to be give probe the screen's size.
+  display_loop->Run();
+  display_configurator_->RemoveObserver(display_observer.get());
   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 for the window to be resized.

[Owen Lin, 2015/01/21 08:10:03]

Here is even more vague. Why we are sure the window will be resized ?

[llandwerlin-old, 2015/01/21 08:44:19]

We initially create the window with a gfx::Rect() -> 0x0 :

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

After calling SetBounds(), will should be notified via
PlatformWindowDelegate::OnBoundsChanged().

+  window_loop->Run();
 #else
 #error unknown platform
 #endif