Unified BeginFrame scheduling

ui/compositor/compositor.cc

Index: ui/compositor/compositor.cc
diff --git a/ui/compositor/compositor.cc b/ui/compositor/compositor.cc
index 9c0a5134318ca49b99ff17c17ebd313bd0c65e9f..1d28f91f157300bb0fbd8901eb5324704dd609fc 100644
--- a/ui/compositor/compositor.cc
+++ b/ui/compositor/compositor.cc
@@ -23,7 +23,6 @@
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/compositor/compositor_observer.h"
 #include "ui/compositor/compositor_switches.h"
-#include "ui/compositor/compositor_vsync_manager.h"
 #include "ui/compositor/dip_util.h"
 #include "ui/compositor/layer.h"
 #include "ui/compositor/layer_animator_collection.h"
@@ -69,11 +68,11 @@ Compositor::Compositor(gfx::AcceleratedWidget widget,
       widget_(widget),
       compositor_thread_loop_(context_factory->GetCompositorMessageLoop()),
       task_runner_(task_runner),
-      vsync_manager_(new CompositorVSyncManager()),
       device_scale_factor_(0.0f),
       disable_schedule_composite_(false),
       compositor_lock_(NULL),
-      layer_animator_collection_(this) {
+      layer_animator_collection_(this),
+      output_surface_lost_(true) {
   root_web_layer_ = cc::Layer::Create();
 
   CommandLine* command_line = CommandLine::ForCurrentProcess();
@@ -87,6 +86,15 @@ Compositor::Compositor(gfx::AcceleratedWidget widget,
   settings.main_frame_before_activation_enabled = false;
   settings.throttle_frame_production =
       !command_line->HasSwitch(switches::kDisableGpuVsync);
+#if defined(USE_AURA) && !defined(OS_CHROMEOS)

[brianderson, 2014/09/04 18:45:46]

Why !OS_CHROMOS?

+  settings.begin_frame_publisher = true;
+#elif defined(OS_MACOSX)
+  // MacOSX only uses browser compositor when delegated rendering is used.
+  if (command_line->HasSwitch(switches::kEnableDelegatedRenderer)) {

[piman, 2014/09/04 20:56:38]

This is a layering violation.

Can this configuration be passed to the constructor instead of ifdefs?

Either way, I think we want to make this a flag until we ship it. We need to be
able to disable it on a branch if it causes problems.

+    settings.begin_frame_publisher = true;
+    settings.begin_frame_receiver = true;
+  }
+#endif
 #if !defined(OS_MACOSX)
   settings.partial_swap_enabled =
       !command_line->HasSwitch(cc::switches::kUIDisablePartialSwap);
@@ -125,6 +133,8 @@ Compositor::Compositor(gfx::AcceleratedWidget widget,
 
   base::TimeTicks before_create = base::TimeTicks::Now();
   if (compositor_thread_loop_) {
+    // Unified BeginFrame scheduling is not used with threaded compositing.
+    DCHECK(!settings.begin_frame_publisher);
     host_ = cc::LayerTreeHost::CreateThreaded(
         this,
         context_factory_->GetSharedBitmapManager(),
@@ -151,6 +161,8 @@ Compositor::~Compositor() {
   CancelCompositorLock();
   DCHECK(!compositor_lock_);
 
+  DCHECK(!begin_frame_observer_list_.might_have_observers());
+
   if (root_layer_)
     root_layer_->SetCompositor(NULL);
 
@@ -228,8 +240,19 @@ void Compositor::SetBackgroundColor(SkColor color) {
   ScheduleDraw();
 }
 
-scoped_refptr<CompositorVSyncManager> Compositor::vsync_manager() const {
-  return vsync_manager_;
+void Compositor::SetAuthoritativeVSyncInterval(
+    base::TimeDelta interval) const {
+  host_->SetAuthoritativeVSyncInterval(interval);
+}
+
+void Compositor::StartBeginFrame(const cc::BeginFrameArgs& args) const {
+  if (output_surface_lost_)

[brianderson, 2014/09/04 18:45:47]

Hmm. Having to expose output_surface_lost_ to the compositor is unfortunate. I
wonder if we can get rid of StartBeginFrame from Compositor and only have the
OutputSurface interact with the platform-specific bits.

+    return;
+  task_runner_->PostTask(

[brianderson, 2014/09/04 18:45:46]

Does this need to be a post task?

+      FROM_HERE,
+      base::Bind(&cc::OutputSurface::BeginFrame,
+                 output_surface_weak_ptr_,
+                 args));
 }
 
 void Compositor::AddObserver(CompositorObserver* observer) {
@@ -258,6 +281,47 @@ bool Compositor::HasAnimationObserver(CompositorAnimationObserver* observer) {
   return animation_observer_list_.HasObserver(observer);
 }
 
+void Compositor::AddBeginFrameObserver(
+    CompositorBeginFrameObserver* observer,
+    const cc::BeginFrameArgs& last_begin_frame_args_sent_to_observer) {
+  bool need_begin_frame = false;

[brianderson, 2014/09/04 18:45:46]

bool need_begin_frame = begin_frame_observer_list_.might_have_observers();

+  if (!begin_frame_observer_list_.might_have_observers()) {
+    need_begin_frame = true;
+  }
+
+  begin_frame_observer_list_.AddObserver(observer);
+
+  if (need_begin_frame) {
+    task_runner_->PostTask(

[brianderson, 2014/09/04 18:45:46]

Does this need to be a PostTask?

+        FROM_HERE,
+        base::Bind(&Compositor::SetChildrenNeedBeginFrames,
+                   base::Unretained(this),
+                   true));
+  }
+
+  // If |last_begin_frame_args_| is still effective, send it to the new
+  // |observer| immediately.
+  if (!last_begin_frame_args_sent_to_observer.deadline.is_null() &&
+      last_begin_frame_args_sent_to_observer != last_begin_frame_args_ &&
+      last_begin_frame_args_.deadline > base::TimeTicks::Now())
+    observer->OnSendBeginFrame(last_begin_frame_args_);
+}
+
+void Compositor::RemoveBeginFrameObserver(
+    CompositorBeginFrameObserver* observer) {
+  DCHECK(begin_frame_observer_list_.might_have_observers());
+
+  begin_frame_observer_list_.RemoveObserver(observer);
+
+  if (!begin_frame_observer_list_.might_have_observers()) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&Compositor::SetChildrenNeedBeginFrames,
+                   base::Unretained(this),
+                   false));
+  }
+}
+
 void Compositor::Animate(base::TimeTicks frame_begin_time) {
   FOR_EACH_OBSERVER(CompositorAnimationObserver,
                     animation_observer_list_,
@@ -276,7 +340,22 @@ void Compositor::Layout() {
 }
 
 scoped_ptr<cc::OutputSurface> Compositor::CreateOutputSurface(bool fallback) {
-  return context_factory_->CreateOutputSurface(this, fallback);
+  scoped_ptr<cc::OutputSurface> output_surface =
+      context_factory_->CreateOutputSurface(this, fallback);
+  // Keep this here, but can be used when DidInitializeOutputSurface() is false.
+  DCHECK(!output_surface_weak_ptr_);
+  output_surface_weak_ptr_ = output_surface->GetWeakPtr();

[piman, 2014/09/04 20:56:38]

This can't work when we use the compositor thread. The output surface is
destroyed on the compositor thread, which means you can only dereference the
weak pointer on that thread.

+  return output_surface.Pass();
+}
+
+void Compositor::DidInitializeOutputSurface() {
+  DCHECK(output_surface_weak_ptr_);
+  output_surface_lost_ = false;
+}
+
+void Compositor::DidLoseOutputSurface() {
+  output_surface_lost_ = true;
+  output_surface_weak_ptr_.reset();
 }
 
 void Compositor::DidCommit() {
@@ -303,6 +382,14 @@ void Compositor::DidCompleteSwapBuffers() {
       CompositorObserver, observer_list_, OnCompositingEnded(this));
 }
 
+void Compositor::SendBeginFrameToChildren(
+    const cc::BeginFrameArgs& args) {
+  FOR_EACH_OBSERVER(CompositorBeginFrameObserver,
+                    begin_frame_observer_list_,
+                    OnSendBeginFrame(args));
+  last_begin_frame_args_ = args;
+}
+
 void Compositor::DidPostSwapBuffers() {
   base::TimeTicks start_time = gfx::FrameTime::Now();
   FOR_EACH_OBSERVER(CompositorObserver,
@@ -350,4 +437,8 @@ void Compositor::CancelCompositorLock() {
     compositor_lock_->CancelLock();
 }
 
+void Compositor::SetChildrenNeedBeginFrames(bool need_begin_frame) {
+  host_->SetChildrenNeedBeginFrames(need_begin_frame);
+}
+
 }  // namespace ui