cc: Make separate interface for BeginFrame ipc from OutputSurface

content/browser/android/in_process/synchronous_compositor_impl.cc

Index: content/browser/android/in_process/synchronous_compositor_impl.cc
diff --git a/content/browser/android/in_process/synchronous_compositor_impl.cc b/content/browser/android/in_process/synchronous_compositor_impl.cc
index 06be5be5696b865d8f0a91047519bdeccc416dfe..92bcef56178646aa021ed7fbf7890bd080db849a 100644
--- a/content/browser/android/in_process/synchronous_compositor_impl.cc
+++ b/content/browser/android/in_process/synchronous_compositor_impl.cc
@@ -4,9 +4,11 @@
 
 #include "content/browser/android/in_process/synchronous_compositor_impl.h"
 
+#include "base/auto_reset.h"
 #include "base/lazy_instance.h"
 #include "base/message_loop/message_loop.h"
 #include "cc/input/input_handler.h"
+#include "content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h"
 #include "content/browser/android/in_process/synchronous_compositor_factory_impl.h"
 #include "content/browser/android/in_process/synchronous_input_event_filter.h"
 #include "content/browser/renderer_host/render_widget_host_view_android.h"
@@ -65,8 +67,10 @@ SynchronousCompositorImpl* SynchronousCompositorImpl::FromRoutingID(
 SynchronousCompositorImpl::SynchronousCompositorImpl(WebContents* contents)
     : compositor_client_(NULL),
       output_surface_(NULL),
+      begin_frame_source_(nullptr),
       contents_(contents),
       input_handler_(NULL),
+      invoking_composite_(false),
       weak_ptr_factory_(this) {
   DCHECK(contents);
 }
@@ -74,6 +78,8 @@ SynchronousCompositorImpl::SynchronousCompositorImpl(WebContents* contents)
 SynchronousCompositorImpl::~SynchronousCompositorImpl() {
   if (compositor_client_)
     compositor_client_->DidDestroyCompositor(this);
+  if (begin_frame_source_)
+    begin_frame_source_->SetCompositor(nullptr);
   SetInputHandler(NULL);
 }
 
@@ -94,6 +100,13 @@ void SynchronousCompositor::SetRecordFullDocument(bool record_full_document) {
   g_factory.Get().SetRecordFullDocument(record_full_document);
 }
 
+void SynchronousCompositorImpl::SetExternalBeginFrameSource(
+    SynchronousCompositorExternalBeginFrameSource* begin_frame_source) {
+  if (begin_frame_source)
+    DCHECK(compositor_client_);

[boliu, 2014/10/30 16:21:32]

Why? I don't think this is guaranteed in general. Even if it is, it's a bad
assumption.

[simonhong, 2014/10/31 00:22:08]

Done.

+  begin_frame_source_ = begin_frame_source;

[boliu, 2014/10/30 16:21:33]

You are assuming here that SetExternalBeginFrameSource is called before
DidBindOutputSurface, which is also a bad assumption.

In general, don't assume (SetExternalBeginFrameSource, DidBindOutputSurface,
SetClient) are called in any particular order.

[simonhong, 2014/10/31 00:22:08]

Removed that assumption related code in DidBindOutputSurface.
(removed DCHECK(begin_frame_source_))

Btw, why that assumption is bad?
In current implementation, DidBindOutputSurface is always called after
SetClient().
Also, if SetClient() is not called first than DidBindOutputSurface(),
we can't generate next frame in current code base.

+}
+
 bool SynchronousCompositorImpl::InitializeHwDraw() {
   DCHECK(CalledOnValidThread());
   DCHECK(output_surface_);
@@ -124,7 +137,11 @@ scoped_ptr<cc::CompositorFrame> SynchronousCompositorImpl::DemandDrawHw(
     const gfx::Transform& transform_for_tile_priority) {
   DCHECK(CalledOnValidThread());
   DCHECK(output_surface_);
+  DCHECK(!invoking_composite_);
+  DCHECK(compositor_client_);
 
+  base::AutoReset<bool> invoking_composite_resetter(&invoking_composite_,
+                                                    true);
   scoped_ptr<cc::CompositorFrame> frame =
       output_surface_->DemandDrawHw(surface_size,
                                     transform,
@@ -135,6 +152,11 @@ scoped_ptr<cc::CompositorFrame> SynchronousCompositorImpl::DemandDrawHw(
   if (frame.get())
     UpdateFrameMetaData(frame->metadata);
 
+  bool needs_begin_frames = false;
+  if (begin_frame_source_)

[boliu, 2014/10/30 16:21:32]

This should be a DCHECK like output_surface_.

compositor_client_ will only call DemandDrawHw/Sw between
DidInitializeCompositor and DidDestroyCompositor. So don't call
DidInitializeCompositor until both OS and BFS are ready, and call
DidDestroyCompositor as soon as one of them goes away.

Then you can make this a DCHECK.

[simonhong, 2014/10/31 00:22:08]

yep, right. It should be always not null.

+    needs_begin_frames = begin_frame_source_->NeedsBeginFrames();
+  compositor_client_->SetContinuousInvalidate(needs_begin_frames);
+
   return frame.Pass();
 }
 
@@ -147,10 +169,21 @@ void SynchronousCompositorImpl::ReturnResources(
 bool SynchronousCompositorImpl::DemandDrawSw(SkCanvas* canvas) {
   DCHECK(CalledOnValidThread());
   DCHECK(output_surface_);
+  DCHECK(!invoking_composite_);
+  DCHECK(compositor_client_);
 
-  scoped_ptr<cc::CompositorFrame> frame = output_surface_->DemandDrawSw(canvas);
+  base::AutoReset<bool> invoking_composite_resetter(&invoking_composite_,
+                                                    true);
+  scoped_ptr<cc::CompositorFrame> frame =
+      output_surface_->DemandDrawSw(canvas);
   if (frame.get())
     UpdateFrameMetaData(frame->metadata);
+
+  bool needs_begin_frames = false;
+  if (begin_frame_source_)
+    needs_begin_frames = begin_frame_source_->NeedsBeginFrames();
+  compositor_client_->SetContinuousInvalidate(needs_begin_frames);
+
   return !!frame.get();
 }
 
@@ -176,15 +209,18 @@ void SynchronousCompositorImpl::DidChangeRootLayerScrollOffset() {
 }
 
 void SynchronousCompositorImpl::DidBindOutputSurface(
-      SynchronousCompositorOutputSurface* output_surface) {
+    SynchronousCompositorOutputSurface* output_surface) {
   DCHECK(CalledOnValidThread());
   output_surface_ = output_surface;
   if (compositor_client_)
     compositor_client_->DidInitializeCompositor(this);
+
+  DCHECK(begin_frame_source_);
+  output_surface_->SetBeginFrameSource(begin_frame_source_);
 }
 
 void SynchronousCompositorImpl::DidDestroySynchronousOutputSurface(
-       SynchronousCompositorOutputSurface* output_surface) {
+    SynchronousCompositorOutputSurface* output_surface) {
   DCHECK(CalledOnValidThread());
 
   // Allow for transient hand-over when two output surfaces may refer to
@@ -194,6 +230,10 @@ void SynchronousCompositorImpl::DidDestroySynchronousOutputSurface(
     if (compositor_client_)
       compositor_client_->DidDestroyCompositor(this);
     compositor_client_ = NULL;
+
+    DCHECK(begin_frame_source_);
+    begin_frame_source_->SetCompositor(nullptr);
+    begin_frame_source_ = nullptr;
   }
 }
 
@@ -226,10 +266,14 @@ void SynchronousCompositorImpl::DidStopFlinging() {
     rwhv->DidStopFlinging();
 }
 
-void SynchronousCompositorImpl::SetContinuousInvalidate(bool enable) {
+void SynchronousCompositorImpl::SetContinuousInvalidate(
+    bool needs_begin_frames) const {
   DCHECK(CalledOnValidThread());
+  if (invoking_composite_)
+    return;
+
   if (compositor_client_)
-    compositor_client_->SetContinuousInvalidate(enable);
+    compositor_client_->SetContinuousInvalidate(needs_begin_frames);
 }
 
 InputEventAckState SynchronousCompositorImpl::HandleInputEvent(