sync compositor: Send ipc reply in swap

content/renderer/android/synchronous_compositor_proxy.cc

Index: content/renderer/android/synchronous_compositor_proxy.cc
diff --git a/content/renderer/android/synchronous_compositor_proxy.cc b/content/renderer/android/synchronous_compositor_proxy.cc
index e176975bfdb1b8659a1143e427789fb61de25c89..61ecab133426575b97d0df659d52291286afeb3b 100644
--- a/content/renderer/android/synchronous_compositor_proxy.cc
+++ b/content/renderer/android/synchronous_compositor_proxy.cc
@@ -33,6 +33,7 @@ SynchronousCompositorProxy::SynchronousCompositorProxy(
       input_handler_proxy_(input_handler_proxy),
       input_handler_(handler),
       inside_receive_(false),
+      hardware_draw_reply_(nullptr),
       bytes_limit_(0u),
       version_(0u),
       page_scale_factor_(0.f),
@@ -145,13 +146,12 @@ void SynchronousCompositorProxy::PopulateCommonParams(
 
 void SynchronousCompositorProxy::OnMessageReceived(
     const IPC::Message& message) {
-  DCHECK(!inside_receive_);
-  base::AutoReset<bool> scoped_inside_receive(&inside_receive_, true);
   IPC_BEGIN_MESSAGE_MAP(SynchronousCompositorProxy, message)
     IPC_MESSAGE_HANDLER(SyncCompositorMsg_HandleInputEvent, HandleInputEvent)
     IPC_MESSAGE_HANDLER(SyncCompositorMsg_BeginFrame, BeginFrame)
     IPC_MESSAGE_HANDLER(SyncCompositorMsg_ComputeScroll, OnComputeScroll)
-    IPC_MESSAGE_HANDLER(SyncCompositorMsg_DemandDrawHw, DemandDrawHw)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(SyncCompositorMsg_DemandDrawHw,
+                                    DemandDrawHw)
     IPC_MESSAGE_HANDLER(SyncCompositorMsg_DemandDrawSw, DemandDrawSw)
     IPC_MESSAGE_HANDLER(SyncCompositorMsg_UpdateState, ProcessCommonParams)
   IPC_END_MESSAGE_MAP()
@@ -166,6 +166,9 @@ void SynchronousCompositorProxy::HandleInputEvent(
     const blink::WebInputEvent* event,
     SyncCompositorCommonRendererParams* common_renderer_params,
     InputEventAckState* ack) {
+  DCHECK(!inside_receive_);
+  base::AutoReset<bool> scoped_inside_receive(&inside_receive_, true);
+
   ProcessCommonParams(common_params);
   DCHECK(!input_handler_->is_null());
   ui::LatencyInfo latency;
@@ -177,6 +180,9 @@ void SynchronousCompositorProxy::BeginFrame(
     const SyncCompositorCommonBrowserParams& common_params,
     const cc::BeginFrameArgs& args,
     SyncCompositorCommonRendererParams* common_renderer_params) {
+  DCHECK(!inside_receive_);
+  base::AutoReset<bool> scoped_inside_receive(&inside_receive_, true);
+
   ProcessCommonParams(common_params);
   if (need_begin_frame_) {
     begin_frame_source_->BeginFrame(args);
@@ -187,19 +193,39 @@ void SynchronousCompositorProxy::BeginFrame(
 void SynchronousCompositorProxy::DemandDrawHw(
     const SyncCompositorCommonBrowserParams& common_params,
     const SyncCompositorDemandDrawHwParams& params,
-    SyncCompositorCommonRendererParams* common_renderer_params,
-    cc::CompositorFrame* frame) {
-  DCHECK(frame);
+    IPC::Message* reply_message) {
+  DCHECK(!inside_receive_);
+  DCHECK(reply_message);
+
+  inside_receive_ = true;

[hush (inactive), 2015/12/29 19:42:17]

Even though SwapBuffersHw will set inside_receive_ to false, you can still use
AutoReset right?

[boliu, 2015/12/29 20:22:52]

Yes technically. But I don't want to be changing a value being held by
AutoReset. Seems not very idiomatic..

Guess what I really want is DCHECK the value is false by the end of the
function, but that seems a bit overkill

+  base::AutoReset<IPC::Message*> scoped_hardware_draw_reply(
+      &hardware_draw_reply_, reply_message);
+
   ProcessCommonParams(common_params);
-  scoped_ptr<cc::CompositorFrame> frame_ptr = output_surface_->DemandDrawHw(
-      params.surface_size, params.transform, params.viewport, params.clip,
-      params.viewport_rect_for_tile_priority,
-      params.transform_for_tile_priority);
-  if (frame_ptr) {
-    frame_ptr->AssignTo(frame);
-    DeliverMessages();
+  output_surface_->DemandDrawHw(params.surface_size, params.transform,
+                                params.viewport, params.clip,
+                                params.viewport_rect_for_tile_priority,
+                                params.transform_for_tile_priority);
+  if (inside_receive_) {
+    // Did not swap.
+    Send(reply_message);

[hush (inactive), 2015/12/29 19:42:17]

I have a question about the case where the swap didn't happen:
in the previous code, the frame_ptr returned from output surface DemandDrawHw
will be null. We will at least populate the common renderer params for the
reply.

But the new code does not do so: it just send a completely empty reply. Is this
wrong?

[boliu, 2015/12/29 20:22:52]

Good question. Yes this is wrong. If you look at
SynchronousCompositorHost::DemandDrawHw, ProcessCommonParams is called
regardless of the state of the compositor frame.

Turns out if you don't write the all of the params and send the reply, the Send
on the browser side returns false, which is also wrong. So need to write the
common renderer params, and also an empty frame back.

+    inside_receive_ = false;
   }
-  PopulateCommonParams(common_renderer_params);
+}
+
+void SynchronousCompositorProxy::SwapBuffersHw(cc::CompositorFrame* frame) {
+  DCHECK(inside_receive_);
+  DCHECK(hardware_draw_reply_);
+  DCHECK(frame);
+
+  SyncCompositorCommonRendererParams common_renderer_params;
+  PopulateCommonParams(&common_renderer_params);
+  // Not using WriteParams because cc::CompositorFrame is not copy-able.
+  IPC::ParamTraits<SyncCompositorCommonRendererParams>::Write(
+      hardware_draw_reply_, common_renderer_params);
+  IPC::ParamTraits<cc::CompositorFrame>::Write(hardware_draw_reply_, *frame);
+  Send(hardware_draw_reply_);
+  inside_receive_ = false;
 }
 
 void SynchronousCompositorProxy::DemandDrawSw(
@@ -208,6 +234,9 @@ void SynchronousCompositorProxy::DemandDrawSw(
     bool* result,
     SyncCompositorCommonRendererParams* common_renderer_params,
     cc::CompositorFrame* frame) {
+  DCHECK(!inside_receive_);
+  base::AutoReset<bool> scoped_inside_receive(&inside_receive_, true);
+
   DCHECK(frame);
   ProcessCommonParams(common_params);
   *result = false;  // Early out ok.
@@ -232,20 +261,34 @@ void SynchronousCompositorProxy::DemandDrawSw(
   canvas.setMatrix(params.transform.matrix());
   canvas.setClipRegion(SkRegion(gfx::RectToSkIRect(params.clip)));
 
-  scoped_ptr<cc::CompositorFrame> frame_ptr =
-      output_surface_->DemandDrawSw(&canvas);
-  if (frame_ptr) {
+  output_surface_->DemandDrawSw(&canvas);
+  if (software_frame_holder_) {
     *result = true;
-    frame_ptr->AssignTo(frame);
+    software_frame_holder_->AssignTo(frame);
+    software_frame_holder_.reset();
     DeliverMessages();
   }
   PopulateCommonParams(common_renderer_params);
 }
 
+void SynchronousCompositorProxy::SwapBuffers(cc::CompositorFrame* frame) {
+  if (hardware_draw_reply_) {
+    SwapBuffersHw(frame);
+    return;
+  }
+
+  DCHECK(!software_frame_holder_);
+  software_frame_holder_.reset(new cc::CompositorFrame);
+  frame->AssignTo(software_frame_holder_.get());
+}
+
 void SynchronousCompositorProxy::OnComputeScroll(
     const SyncCompositorCommonBrowserParams& common_params,
     base::TimeTicks animation_time,
     SyncCompositorCommonRendererParams* common_renderer_params) {
+  DCHECK(!inside_receive_);
+  base::AutoReset<bool> scoped_inside_receive(&inside_receive_, true);
+
   ProcessCommonParams(common_params);
   if (need_animate_scroll_) {
     need_animate_scroll_ = false;