sync compositor: Send ipc reply in swap

content/renderer/android/synchronous_compositor_proxy.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/android/synchronous_compositor_proxy.h"
 
 #include "base/auto_reset.h"
 #include "base/memory/shared_memory.h"
 #include "content/common/android/sync_compositor_messages.h"
 #include "content/common/cc_messages.h"
@@ skipping 15 matching lines @@
     SynchronousCompositorExternalBeginFrameSource* begin_frame_source,
     ui::SynchronousInputHandlerProxy* input_handler_proxy,
     InputHandlerManagerClient::Handler* handler)
     : routing_id_(routing_id),
       sender_(sender),
       output_surface_(output_surface),
       begin_frame_source_(begin_frame_source),
       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),
       min_page_scale_factor_(0.f),
       max_page_scale_factor_(0.f),
       need_animate_scroll_(false),
       need_invalidate_(false),
       need_begin_frame_(false),
       did_activate_pending_tree_(false) {
   DCHECK(output_surface_);
@@ skipping 92 matching lines @@
   params->need_invalidate = need_invalidate_;
   params->need_begin_frame = need_begin_frame_;
   params->did_activate_pending_tree = did_activate_pending_tree_;
 
   need_invalidate_ = false;
   did_activate_pending_tree_ = false;
 }
 
 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()
 }
 
 bool SynchronousCompositorProxy::Send(IPC::Message* message) {
   return sender_->Send(message);
 }
 
 void SynchronousCompositorProxy::HandleInputEvent(
     const SyncCompositorCommonBrowserParams& common_params,
     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;
   *ack = input_handler_->Run(routing_id_, event, &latency);
   PopulateCommonParams(common_renderer_params);
 }
 
 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);
   }
   PopulateCommonParams(common_renderer_params);
 }
 
 void SynchronousCompositorProxy::DemandDrawHw(
     const SyncCompositorCommonBrowserParams& common_params,
     const SyncCompositorDemandDrawHwParams& params,
-    SyncCompositorCommonRendererParams* common_renderer_params,
-    cc::CompositorFrame* 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);
+  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;
+  }
+}
+
+void SynchronousCompositorProxy::SwapBuffersHw(cc::CompositorFrame* frame) {
+  DCHECK(inside_receive_);
+  DCHECK(hardware_draw_reply_);
   DCHECK(frame);
-  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();
-  }
-  PopulateCommonParams(common_renderer_params);
+
+  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(
     const SyncCompositorCommonBrowserParams& common_params,
     const SyncCompositorDemandDrawSwParams& params,
     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.
   if (!base::SharedMemory::IsHandleValid(params.shm_handle))
     return;
 
   SkImageInfo info =
       SkImageInfo::MakeN32Premul(params.size.width(), params.size.height());
   size_t stride = info.minRowBytes();
   size_t buffer_size = info.getSafeSize(stride);
   DCHECK(buffer_size);
 
   base::SharedMemory shm(params.shm_handle, false);
   if (!shm.Map(buffer_size))
     return;
   DCHECK(shm.memory());
 
   SkBitmap bitmap;
   if (!bitmap.installPixels(info, shm.memory(), stride))
     return;
   SkCanvas canvas(bitmap);
   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;
     input_handler_proxy_->SynchronouslyAnimate(animation_time);
   }
   PopulateCommonParams(common_renderer_params);
 }
 
 void SynchronousCompositorProxy::DidOverscroll(
     const DidOverscrollParams& did_overscroll_params) {
@@ skipping 14 matching lines @@
     total_scroll_offset_ = common_params.root_scroll_offset;
     input_handler_proxy_->SynchronouslySetRootScrollOffset(
         total_scroll_offset_);
   }
   if (!common_params.ack.resources.empty()) {
     output_surface_->ReturnResources(common_params.ack);
   }
 }
 
 }  // namespace content