Deliver IPC messages together with SwapCompositorFrame

content/renderer/render_widget.cc

 // Copyright (c) 2012 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/render_widget.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 19 matching lines @@
 #include "content/common/input_messages.h"
 #include "content/common/swapped_out_messages.h"
 #include "content/common/view_messages.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/context_menu_params.h"
 #include "content/renderer/cursor_utils.h"
 #include "content/renderer/external_popup_menu.h"
 #include "content/renderer/gpu/compositor_output_surface.h"
 #include "content/renderer/gpu/compositor_software_output_device.h"
 #include "content/renderer/gpu/delegated_compositor_output_surface.h"
+#include "content/renderer/gpu/frame_swap_message_queue.h"
 #include "content/renderer/gpu/mailbox_output_surface.h"
 #include "content/renderer/gpu/render_widget_compositor.h"
 #include "content/renderer/ime_event_guard.h"
 #include "content/renderer/input/input_handler_manager.h"
 #include "content/renderer/pepper/pepper_plugin_instance_impl.h"
 #include "content/renderer/render_frame_impl.h"
 #include "content/renderer/render_process.h"
 #include "content/renderer/render_thread_impl.h"
 #include "content/renderer/renderer_webkitplatformsupport_impl.h"
 #include "content/renderer/resizing_mode_selector.h"
@@ skipping 97 matching lines @@
     return ui::TEXT_INPUT_MODE_DEFAULT;
   return it->second;
 }
 
 // TODO(brianderson): Replace the hard-coded threshold with a fraction of
 // the BeginMainFrame interval.
 // 4166us will allow 1/4 of a 60Hz interval or 1/2 of a 120Hz interval to
 // be spent in input hanlders before input starts getting throttled.
 const int kInputHandlingTimeThrottlingThresholdMicroseconds = 4166;
 
+class QueueMessageSwapPromise : public cc::SwapPromise {
+ public:
+  QueueMessageSwapPromise(
+      scoped_refptr<IPC::SyncMessageFilter> message_sender,
+      scoped_refptr<content::FrameSwapMessageQueue> message_queue,
+      int source_frame_number)
+      : message_sender_(message_sender),
+        message_queue_(message_queue),
+        source_frame_number_(source_frame_number),
+        completed_(false) {
+    DCHECK(message_sender_.get());
+    DCHECK(message_queue_.get());
+  }
+
+  virtual ~QueueMessageSwapPromise() {
+    // The promise should have either been kept or broken before it's deleted.
+    DCHECK(completed_);
+  }
+
+  virtual void DidSwap(cc::CompositorFrameMetadata* metadata) OVERRIDE {
+    DCHECK(!completed_);
+    message_queue_->AdvanceToFrame(source_frame_number_);
+    // The OutputSurface will take care of the Drain+Send.
+    completed_ = true;
+  }
+
+  virtual void DidNotSwap(DidNotSwapReason reason) OVERRIDE {
+    DCHECK(!completed_);
+    // This will also send messages explicitly queued with the WITH_NEXT_SWAP
+    // policy. It makes sense to send them if reason == SWAP_FAILS, but they may
+    // end up being sent prematurely if reason == COMMIT_FAILS.

[mkosiba (inactive), 2014/06/18 14:58:02]

Locally I have a CL which adds policy-specific sub-queues to
FrameSwapMessageQueue which should solve this particular issue, but it's pretty
big so I'd prefer to do that as a follow-up.

Do you have any specific requirements/preferences for the behavior here? Should
messages enqueued with the NEXT_SWAP policy be sent when DidNotSwap(SWAP_FAILS)
is called or should they remain in the queue till we get a successful swap?

[piman, 2014/06/19 23:02:29]

SWAP_FAILS happens when we went all the way to drawing, but there was no damage,
so we skipped. I think this is a valid reason to send immediately once we know
that (we would have sent a frame normally, not sending one is an optimization).

COMMIT_FAILS gathers several cases, that we may want to discriminate for this
purpose:
1- the renderer is hidden
2- we lost the output surface / context
3- we went through the commit but found that nothing had changed

case 3 is like SWAP_FAILS - it's an optimization but for practical purposes, we
would have committed and sent a frame normally, so it's ok to send immediately.
case 2 is definitely valid to delay.
I think case 1 too, but I'd have to double-check the specifics for e.g. resize
or plugin moves.

[mkosiba (inactive), 2014/06/24 18:29:54]

If I'm reading that right it means there are cases where we might not get a 
DidSwap for a long time. I think might be better to send on failures then,
otherwise we're creating an API that works most of the time but can also lead to
deadlocks on rare occasions (at least theoretically, say the Host won't 'unhide'
the view till some IPC arrives but the IPC is waiting for a successful DidSwap).

Does it actually matter if we end up delivering messages 'too early' for cases 1
and 2 ?

[piman, 2014/06/24 19:35:53]

I don't think that would happen. The visibility is managed by the UI side, and
would not wait on the renderer.
Yes, I think so. I would like to use this mechanism to send the
ViewHostMsg_UpdateRect together with the frame. Today it's only sent after we
receive the swap ack from the browser, which means after we've actually drawn a
frame and sent it to the browser (i.e. only when we're visible). Some code on
the browser side relies on that ordering. If we were to send the
ViewHostMsg_UpdateRect too early, this would violate the assumptions.

+    std::vector<IPC::Message> messages;
+    message_queue_->AdvanceToFrame(source_frame_number_);
+    scoped_ptr<content::FrameSwapMessageQueue::SendMessageScope>
+        send_message_scope = message_queue_->AcquireSendMessageScope();
+    message_queue_->DrainMessages(&messages);
+    for (std::vector<IPC::Message>::iterator i = messages.begin();
+         i != messages.end();
+         ++i) {
+      message_sender_->Send(&*i);

[piman, 2014/06/19 23:02:29]

Send is supposed to take ownership of the IPC::Message, it's not ok to pass it
the address of the message in the vector.

I think you want to use the other version of DrainMessages here.

[mkosiba (inactive), 2014/06/24 18:29:54]

argh, right right

+    }
+    completed_ = true;
+  }
+
+ private:
+  scoped_refptr<IPC::SyncMessageFilter> message_sender_;
+  scoped_refptr<content::FrameSwapMessageQueue> message_queue_;
+  int source_frame_number_;
+  bool completed_;

[piman, 2014/06/19 23:02:29]

Is this used only for DCHECKs? Should it be in #ifndef NDEBUG ?

[mkosiba (inactive), 2014/06/24 18:29:54]

Done.

+};
+
 }  // namespace
 
 namespace content {
 
 // RenderWidget::ScreenMetricsEmulator ----------------------------------------
 
 class RenderWidget::ScreenMetricsEmulator {
  public:
   ScreenMetricsEmulator(
       RenderWidget* widget,
@@ skipping 223 matching lines @@
       device_scale_factor_(screen_info_.deviceScaleFactor),
       is_threaded_compositing_enabled_(false),
       current_event_latency_info_(NULL),
       next_output_surface_id_(0),
 #if defined(OS_ANDROID)
       text_field_is_dirty_(false),
       outstanding_ime_acks_(0),
       body_background_color_(SK_ColorWHITE),
 #endif
       popup_origin_scale_for_emulation_(0.f),
+      frame_swap_message_queue_(new FrameSwapMessageQueue()),
       resizing_mode_selector_(new ResizingModeSelector()),
       context_menu_source_type_(ui::MENU_SOURCE_MOUSE) {
   if (!swapped_out)
     RenderProcess::current()->AddRefProcess();
   DCHECK(RenderThread::Get());
   is_threaded_compositing_enabled_ =
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableThreadedCompositing);
 }
 
@@ skipping 395 matching lines @@
 }
 
 scoped_ptr<cc::OutputSurface> RenderWidget::CreateOutputSurface(bool fallback) {
   // For widgets that are never visible, we don't start the compositor, so we
   // never get a request for a cc::OutputSurface.
   DCHECK(!never_visible_);
 
 #if defined(OS_ANDROID)
   if (SynchronousCompositorFactory* factory =
       SynchronousCompositorFactory::GetInstance()) {
-    return factory->CreateOutputSurface(routing_id());
+    return factory->CreateOutputSurface(routing_id(),
+                                        frame_swap_message_queue_);
   }
 #endif
 
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
   bool use_software = fallback;
   if (command_line.HasSwitch(switches::kDisableGpuCompositing))
     use_software = true;
 
   scoped_refptr<ContextProviderCommandBuffer> context_provider;
   if (!use_software) {
     context_provider = ContextProviderCommandBuffer::Create(
         CreateGraphicsContext3D(), "RenderCompositor");
     if (!context_provider.get()) {
       // Cause the compositor to wait and try again.
       return scoped_ptr<cc::OutputSurface>();
     }
   }
 
   uint32 output_surface_id = next_output_surface_id_++;
   if (command_line.HasSwitch(switches::kEnableDelegatedRenderer)) {
     DCHECK(is_threaded_compositing_enabled_);
     return scoped_ptr<cc::OutputSurface>(
-        new DelegatedCompositorOutputSurface(
-            routing_id(),
-            output_surface_id,
-            context_provider));
+        new DelegatedCompositorOutputSurface(routing_id(),
+                                             output_surface_id,
+                                             context_provider,
+                                             frame_swap_message_queue_));
   }
   if (!context_provider.get()) {
     scoped_ptr<cc::SoftwareOutputDevice> software_device(
         new CompositorSoftwareOutputDevice());
 
-    return scoped_ptr<cc::OutputSurface>(new CompositorOutputSurface(
-        routing_id(),
-        output_surface_id,
-        NULL,
-        software_device.Pass(),
-        true));
+    return scoped_ptr<cc::OutputSurface>(
+        new CompositorOutputSurface(routing_id(),
+                                    output_surface_id,
+                                    NULL,
+                                    software_device.Pass(),
+                                    frame_swap_message_queue_,
+                                    true));
   }
 
   if (command_line.HasSwitch(cc::switches::kCompositeToMailbox)) {
     // Composite-to-mailbox is currently used for layout tests in order to cause
     // them to draw inside in the renderer to do the readback there. This should
     // no longer be the case when crbug.com/311404 is fixed.
     DCHECK(is_threaded_compositing_enabled_ ||
            RenderThreadImpl::current()->layout_test_mode());
     cc::ResourceFormat format = cc::RGBA_8888;
 #if defined(OS_ANDROID)
     if (base::android::SysUtils::IsLowEndDevice())
       format = cc::RGB_565;
 #endif
     return scoped_ptr<cc::OutputSurface>(
-        new MailboxOutputSurface(
-            routing_id(),
-            output_surface_id,
-            context_provider,
-            scoped_ptr<cc::SoftwareOutputDevice>(),
-            format));
+        new MailboxOutputSurface(routing_id(),
+                                 output_surface_id,
+                                 context_provider,
+                                 scoped_ptr<cc::SoftwareOutputDevice>(),
+                                 frame_swap_message_queue_,
+                                 format));
   }
   bool use_swap_compositor_frame_message = false;
   return scoped_ptr<cc::OutputSurface>(
-      new CompositorOutputSurface(
-          routing_id(),
-          output_surface_id,
-          context_provider,
-          scoped_ptr<cc::SoftwareOutputDevice>(),
-          use_swap_compositor_frame_message));
+      new CompositorOutputSurface(routing_id(),
+                                  output_surface_id,
+                                  context_provider,
+                                  scoped_ptr<cc::SoftwareOutputDevice>(),
+                                  frame_swap_message_queue_,
+                                  use_swap_compositor_frame_message));
 }
 
 void RenderWidget::OnSwapBuffersAborted() {
   TRACE_EVENT0("renderer", "RenderWidget::OnSwapBuffersAborted");
   // Schedule another frame so the compositor learns about it.
   scheduleComposite();
 }
 
 void RenderWidget::OnSwapBuffersPosted() {
   TRACE_EVENT0("renderer", "RenderWidget::OnSwapBuffersPosted");
@@ skipping 319 matching lines @@
   ObserverListBase<RenderFrameImpl>::Iterator iter(video_hole_frames_);
   RenderFrameImpl* frame;
   while ((frame = iter.GetNext()) != NULL) {
     // Prevent duplicate notification of DidCommitCompositorFrame().
     if (!swapped_out_frames_.HasObserver(frame))
       frame->DidCommitCompositorFrame();
   }
 #endif  // defined(VIDEO_HOLE)
 }
 
+namespace {}  //  namespace
+
+void RenderWidget::QueueMessage(IPC::Message* msg,
+                                MessageDeliveryPolicy policy) {
+  if (!compositor_) {
+    Send(msg);
+    return;
+  }
+
+  if (policy == MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP) {
+    frame_swap_message_queue_->QueueMessage(msg);
+    return;
+  }
+
+  if (policy == MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE &&
+      // No need for lock: this gets changed only on this thread.
+      !compositor_->commitRequested() &&
+      // No need for lock: Messages are only enqueued from this thread, if we
+      // don't have any now, no other thread will add any.
+      !frame_swap_message_queue_->Empty()) {

[mkosiba (inactive), 2014/06/18 14:58:03]

so this should handle FIFO ordering correctly now. We only short-circuit if
there are no messages 'in transit' in the queue.

This runs a small chance of unnecessarily adding a message to the queue if the
queue is completely drained between now and line 1304 but I don't think that
matters (should happen infrequently).

[piman, 2014/06/19 23:02:29]

Did you mean && frame_swap_message_queue_->Empty() (no !) ?

[piman, 2014/06/19 23:02:29]

For that case, you may want to force a commit if
!compositor_->commitRequested(). That way you know the SwapPromise will make
progress (worst case with a COMMIT_FAILS) and you'll eventually send the
message.

[mkosiba (inactive), 2014/06/24 18:29:54]

yes, right, I had this method called HasMessages before and forgot to remove the
negation

[mkosiba (inactive), 2014/06/24 18:29:54]

Done.

+    Send(msg);
+    return;
+  }
+
+  DCHECK(policy == MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE ||
+         policy == MESSAGE_DELIVERY_POLICY_WITH_NEXT_COMMIT);
+  const int source_frame_number = compositor_->GetSourceFrameNumber();
+  bool first_message_for_frame = false;
+  frame_swap_message_queue_->QueueMessage(
+      source_frame_number, msg, &first_message_for_frame);
+  if (first_message_for_frame) {
+    scoped_ptr<cc::SwapPromise> promise(new QueueMessageSwapPromise(
+        RenderThreadImpl::current()->sync_message_filter(),
+        frame_swap_message_queue_,
+        source_frame_number));

[mkosiba (inactive), 2014/06/18 14:58:03]

This way uses less memory since we'll use at most one SwapPromise per
source_frame_number rather than one SwapPromise per message.

+    compositor_->QueueSwapPromise(promise.Pass());
+  }
+}
+
 void RenderWidget::didCommitAndDrawCompositorFrame() {
   // NOTE: Tests may break if this event is renamed or moved. See
   // tab_capture_performancetest.cc.
   TRACE_EVENT0("gpu", "RenderWidget::didCommitAndDrawCompositorFrame");
   // Notify subclasses that we initiated the paint operation.
   DidInitiatePaint();
 }
 
 void RenderWidget::didCompleteSwapBuffers() {
   TRACE_EVENT0("renderer", "RenderWidget::didCompleteSwapBuffers");
@@ skipping 839 matching lines @@
 void RenderWidget::RegisterVideoHoleFrame(RenderFrameImpl* frame) {
   video_hole_frames_.AddObserver(frame);
 }
 
 void RenderWidget::UnregisterVideoHoleFrame(RenderFrameImpl* frame) {
   video_hole_frames_.RemoveObserver(frame);
 }
 #endif  // defined(VIDEO_HOLE)
 
 }  // namespace content