sync compositor: Version CompositorFrameMetadata

CompositorFrameMetadata can arrive on the UI thread out of order:
Metadata is sent from compositor to UI in response to synchronous or
asynchronous draws. The synchronous draw replies is processed
immediately, even if there are replies to asynchronous draws pending. So
when there is a mix of sync and async draws, it's possible for metadata
to arrive out of order.

This has been a theoretical problem for a long time, but only became
real problem when FrameSwapMessageQueue was expecting metadata to be
ordered. The only practical fix is to simply drop out of order metadata
instead, which FrameSwapMessageQueue is designed to handle already.

Bug: 821706
Change-Id: I493862ba6d0838097f26e824359f03017eaa2a35
Reviewed-on: https://chromium-review.googlesource.com/985096Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546750}

content/browser/android/synchronous_compositor_browser_filter.cc

@@ -56,9 +56,10 @@ bool SynchronousCompositorBrowserFilter::ReceiveFrame(
   }
 
   int frame_sink_id = std::get<0>(param);
-  base::Optional<viz::CompositorFrame>& compositor_frame = std::get<1>(param);
+  uint32_t metadata_version = std::get<1>(param);
+  base::Optional<viz::CompositorFrame>& compositor_frame = std::get<2>(param);
 
-  if (!itr->second->ReceiveFrameOnIOThread(frame_sink_id,
+  if (!itr->second->ReceiveFrameOnIOThread(frame_sink_id, metadata_version,
                                            std::move(compositor_frame))) {
     bad_message::ReceivedBadMessage(this, bad_message::SCO_INVALID_ARGUMENT);
   }

content/browser/android/synchronous_compositor_host.cc

@@ -57,10 +57,11 @@ class SynchronousCompositorLegacyChromeIPC
       const content::SyncCompositorDemandDrawHwParams& draw_params,
       content::SyncCompositorCommonRendererParams* out_result,
       uint32_t* out_layer_tree_frame_sink_id,
+      uint32_t* out_metadata_version,
       base::Optional<viz::CompositorFrame>* out_frame) override {
     return sender_->Send(new SyncCompositorMsg_DemandDrawHw(
         routing_id_, draw_params, out_result, out_layer_tree_frame_sink_id,
-        out_frame));
+        out_metadata_version, out_frame));
   }
 
   void DemandDrawHw(const SyncCompositorDemandDrawHwParams& params,
@@ -84,9 +85,11 @@ class SynchronousCompositorLegacyChromeIPC
   bool DemandDrawSw(
       const content::SyncCompositorDemandDrawSwParams& draw_params,
       content::SyncCompositorCommonRendererParams* out_result,
+      uint32_t* out_metadata_version,
       base::Optional<viz::CompositorFrameMetadata>* out_meta_data) override {
     return sender_->Send(new SyncCompositorMsg_DemandDrawSw(
-        routing_id_, draw_params, out_result, out_meta_data));
+        routing_id_, draw_params, out_result, out_metadata_version,
+        out_meta_data));
   }
 
   void DemandDrawSw(const SyncCompositorDemandDrawSwParams& params,
@@ -179,9 +182,10 @@ class SynchronousCompositorControlHost
 
   // SynchronousCompositorControlHost overrides.
   void ReturnFrame(uint32_t layer_tree_frame_sink_id,
+                   uint32_t metadata_version,
                    base::Optional<viz::CompositorFrame> frame) override {
     if (!bridge_->ReceiveFrameOnIOThread(layer_tree_frame_sink_id,
-                                         std::move(frame))) {
+                                         metadata_version, std::move(frame))) {
       bad_message::ReceivedBadMessage(
           process_id_, bad_message::SYNC_COMPOSITOR_NO_FUTURE_FRAME);
     }
@@ -320,6 +324,7 @@ SynchronousCompositor::Frame SynchronousCompositorHost::DemandDrawHw(
                                           viewport_rect_for_tile_priority,
                                           transform_for_tile_priority);
   uint32_t layer_tree_frame_sink_id;
+  uint32_t metadata_version = 0u;
   base::Optional<viz::CompositorFrame> compositor_frame;
   SyncCompositorCommonRendererParams common_renderer_params;
 
@@ -330,7 +335,7 @@ SynchronousCompositor::Frame SynchronousCompositorHost::DemandDrawHw(
     if (!IsReadyForSynchronousCall() ||
         !GetSynchronousCompositor()->DemandDrawHw(
             params, &common_renderer_params, &layer_tree_frame_sink_id,
-            &compositor_frame)) {
+            &metadata_version, &compositor_frame)) {
       return SynchronousCompositor::Frame();
     }
   }
@@ -344,12 +349,19 @@ SynchronousCompositor::Frame SynchronousCompositorHost::DemandDrawHw(
   frame.frame.reset(new viz::CompositorFrame);
   frame.layer_tree_frame_sink_id = layer_tree_frame_sink_id;
   *frame.frame = std::move(*compositor_frame);
-  UpdateFrameMetaData(frame.frame->metadata.Clone());
+  UpdateFrameMetaData(metadata_version, frame.frame->metadata.Clone());
   return frame;
 }
 
 void SynchronousCompositorHost::UpdateFrameMetaData(
+    uint32_t version,
     viz::CompositorFrameMetadata frame_metadata) {
+  // Ignore if |frame_metadata_version_| is newer than |version|. This
+  // comparison takes into account when the unsigned int wraps.
+  if ((frame_metadata_version_ - version) < 0x80000000) {
+    return;
+  }
+  frame_metadata_version_ = version;
   rwhva_->SynchronousFrameMetadata(std::move(frame_metadata));
 }
 
@@ -379,14 +391,15 @@ bool SynchronousCompositorHost::DemandDrawSwInProc(SkCanvas* canvas) {
   base::Optional<viz::CompositorFrameMetadata> metadata;
   ScopedSetSkCanvas set_sk_canvas(canvas);
   SyncCompositorDemandDrawSwParams params;  // Unused.
+  uint32_t metadata_version = 0u;
   if (!IsReadyForSynchronousCall() ||
       !GetSynchronousCompositor()->DemandDrawSw(params, &common_renderer_params,
-                                                &metadata))
+                                                &metadata_version, &metadata))
     return false;
   if (!metadata)
     return false;
   UpdateState(common_renderer_params);
-  UpdateFrameMetaData(std::move(*metadata));
+  UpdateFrameMetaData(metadata_version, std::move(*metadata));
   return true;
 }
 
@@ -439,6 +452,7 @@ bool SynchronousCompositorHost::DemandDrawSw(SkCanvas* canvas) {
     return false;
 
   base::Optional<viz::CompositorFrameMetadata> metadata;
+  uint32_t metadata_version = 0u;
   SyncCompositorCommonRendererParams common_renderer_params;
   {
     mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
@@ -446,7 +460,7 @@ bool SynchronousCompositorHost::DemandDrawSw(SkCanvas* canvas) {
         allow_base_sync_primitives;
     if (!IsReadyForSynchronousCall() ||
         !GetSynchronousCompositor()->DemandDrawSw(
-            params, &common_renderer_params, &metadata)) {
+            params, &common_renderer_params, &metadata_version, &metadata)) {
       return false;
     }
   }
@@ -455,7 +469,7 @@ bool SynchronousCompositorHost::DemandDrawSw(SkCanvas* canvas) {
     return false;
 
   UpdateState(common_renderer_params);
-  UpdateFrameMetaData(std::move(*metadata));
+  UpdateFrameMetaData(metadata_version, std::move(*metadata));
 
   SkBitmap bitmap;
   if (!bitmap.installPixels(info, software_draw_shm_->shm.memory(), stride))

content/browser/android/synchronous_compositor_host.h

@@ -73,7 +73,8 @@ class SynchronousCompositorHost : public SynchronousCompositor,
 
   // Called by SynchronousCompositorSyncCallBridge.
   int routing_id() const { return routing_id_; }
-  void UpdateFrameMetaData(viz::CompositorFrameMetadata frame_metadata);
+  void UpdateFrameMetaData(uint32_t version,
+                           viz::CompositorFrameMetadata frame_metadata);
 
   // Called when the mojo channel should be created.
   void InitMojo();
@@ -151,6 +152,7 @@ class SynchronousCompositorHost : public SynchronousCompositor,
   bool need_animate_scroll_;
   uint32_t need_invalidate_count_;
   uint32_t did_activate_pending_tree_count_;
+  uint32_t frame_metadata_version_ = 0u;
 
   scoped_refptr<SynchronousCompositorSyncCallBridge> bridge_;
 

content/browser/android/synchronous_compositor_sync_call_bridge.cc

@@ -60,6 +60,7 @@ void SynchronousCompositorSyncCallBridge::RemoteClosedOnIOThread() {
 
 bool SynchronousCompositorSyncCallBridge::ReceiveFrameOnIOThread(
     int layer_tree_frame_sink_id,
+    uint32_t metadata_version,
     base::Optional<viz::CompositorFrame> compositor_frame) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   base::AutoLock lock(lock_);
@@ -77,7 +78,8 @@ bool SynchronousCompositorSyncCallBridge::ReceiveFrameOnIOThread(
         BrowserThread::UI, FROM_HERE,
         base::BindOnce(&SynchronousCompositorSyncCallBridge::
                            ProcessFrameMetadataOnUIThread,
-                       this, compositor_frame->metadata.Clone()));
+                       this, metadata_version,
+                       compositor_frame->metadata.Clone()));
     frame_ptr->frame.reset(new viz::CompositorFrame);
     *frame_ptr->frame = std::move(*compositor_frame);
   }
@@ -186,10 +188,11 @@ void SynchronousCompositorSyncCallBridge::VSyncCompleteOnUIThread() {
 }
 
 void SynchronousCompositorSyncCallBridge::ProcessFrameMetadataOnUIThread(
+    uint32_t metadata_version,
     viz::CompositorFrameMetadata metadata) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (host_)
-    host_->UpdateFrameMetaData(std::move(metadata));
+    host_->UpdateFrameMetaData(metadata_version, std::move(metadata));
 }
 
 void SynchronousCompositorSyncCallBridge::UnregisterSyncCallBridgeOnIOThread(

content/browser/android/synchronous_compositor_sync_call_bridge.h

@@ -83,6 +83,7 @@ class SynchronousCompositorSyncCallBridge
 
   // Receive a frame. Return false if the corresponding frame wasn't found.
   bool ReceiveFrameOnIOThread(int frame_sink_id,
+                              uint32_t metadata_version,
                               base::Optional<viz::CompositorFrame>);
 
   // Receive a BeginFrameResponse. Returns true if handling the response was
@@ -113,7 +114,8 @@ class SynchronousCompositorSyncCallBridge
   void VSyncCompleteOnUIThread();
 
   // Process metadata.
-  void ProcessFrameMetadataOnUIThread(viz::CompositorFrameMetadata metadata);
+  void ProcessFrameMetadataOnUIThread(uint32_t metadata_version,
+                                      viz::CompositorFrameMetadata metadata);
 
   void UnregisterSyncCallBridgeOnIOThread(
       scoped_refptr<SynchronousCompositorBrowserFilter> filter);

content/common/input/sync_compositor_messages.h

@@ -123,10 +123,11 @@ IPC_MESSAGE_ROUTED1(SyncCompositorMsg_ComputeScroll, base::TimeTicks)
 IPC_MESSAGE_ROUTED1(SyncCompositorMsg_DemandDrawHwAsync,
                     content::SyncCompositorDemandDrawHwParams)
 
-IPC_SYNC_MESSAGE_ROUTED1_3(SyncCompositorMsg_DemandDrawHw,
+IPC_SYNC_MESSAGE_ROUTED1_4(SyncCompositorMsg_DemandDrawHw,
                            content::SyncCompositorDemandDrawHwParams,
                            content::SyncCompositorCommonRendererParams,
                            uint32_t /* layer_tree_frame_sink_id */,
+                           uint32_t /* metadata_version */,
                            base::Optional<viz::CompositorFrame>)
 
 IPC_SYNC_MESSAGE_ROUTED1_2(SyncCompositorMsg_SetSharedMemory,
@@ -136,9 +137,10 @@ IPC_SYNC_MESSAGE_ROUTED1_2(SyncCompositorMsg_SetSharedMemory,
 
 IPC_MESSAGE_ROUTED0(SyncCompositorMsg_ZeroSharedMemory)
 
-IPC_SYNC_MESSAGE_ROUTED1_2(SyncCompositorMsg_DemandDrawSw,
+IPC_SYNC_MESSAGE_ROUTED1_3(SyncCompositorMsg_DemandDrawSw,
                            content::SyncCompositorDemandDrawSwParams,
                            content::SyncCompositorCommonRendererParams,
+                           uint32_t /* metadata_version */,
                            base::Optional<viz::CompositorFrameMetadata>)
 
 IPC_SYNC_MESSAGE_ROUTED2_1(SyncCompositorMsg_ZoomBy,
@@ -174,8 +176,9 @@ IPC_MESSAGE_ROUTED1(SyncCompositorHostMsg_UpdateState,
 IPC_MESSAGE_ROUTED1(SyncCompositorHostMsg_BeginFrameResponse,
                     content::SyncCompositorCommonRendererParams)
 
-IPC_MESSAGE_ROUTED2(SyncCompositorHostMsg_ReturnFrame,
+IPC_MESSAGE_ROUTED3(SyncCompositorHostMsg_ReturnFrame,
                     uint32_t /* layer_tree_frame_sink_id */,
+                    uint32_t /* metadata_version */,
                     base::Optional<viz::CompositorFrame>)
 
 // Sent by renderer to request a SyncCompositorMsg_BeginFrame message for

content/common/input/synchronous_compositor.mojom

@@ -41,6 +41,7 @@ interface SynchronousCompositor {
   DemandDrawHw(SyncCompositorDemandDrawHwParams draw_params) =>
       (SyncCompositorCommonRendererParams result,
        uint32 layer_tree_frame_sink_id,
+       uint32 metadata_version,
        viz.mojom.CompositorFrame? frame);
 
   // Synchronously sets the shared memory used for resourceless software
@@ -53,6 +54,7 @@ interface SynchronousCompositor {
   // Synchronously does a software based draw.
   [Sync] DemandDrawSw(SyncCompositorDemandDrawSwParams draw_params) =>
       (SyncCompositorCommonRendererParams result,
+       uint32 metadata_version,
        viz.mojom.CompositorFrameMetadata? meta_data);
 
   // Zero out the shared memory. This is necessary since most of the time,
@@ -110,8 +112,9 @@ interface SynchronousCompositorHost {
 interface SynchronousCompositorControlHost {
   // Response from DrawHwAsync.
   ReturnFrame(uint32 layer_tree_frame_sink_id,
+              uint32 metadata_version,
               viz.mojom.CompositorFrame? frame);
 
   // Response from BeginFrame.
   BeginFrameResponse(SyncCompositorCommonRendererParams params);
-};
+};
\ No newline at end of file

content/renderer/android/synchronous_compositor_proxy.cc

@@ -146,7 +146,7 @@ void SynchronousCompositorProxy::DemandDrawHw(
     PopulateCommonParams(&common_renderer_params);
     // Did not swap.
     std::move(hardware_draw_reply_)
-        .Run(common_renderer_params, 0u, base::nullopt);
+        .Run(common_renderer_params, 0u, 0u, base::nullopt);
   }
 }
 
@@ -191,7 +191,8 @@ void SynchronousCompositorProxy::DemandDrawSw(
     SyncCompositorCommonRendererParams common_renderer_params;
     PopulateCommonParams(&common_renderer_params);
     // Did not swap.
-    std::move(software_draw_reply_).Run(common_renderer_params, base::nullopt);
+    std::move(software_draw_reply_)
+        .Run(common_renderer_params, 0u, base::nullopt);
   }
 }
 
@@ -228,10 +229,11 @@ void SynchronousCompositorProxy::SubmitCompositorFrame(
   if (hardware_draw_reply_) {
     std::move(hardware_draw_reply_)
         .Run(common_renderer_params, layer_tree_frame_sink_id,
-             std::move(frame));
+             NextMetadataVersion(), std::move(frame));
   } else if (software_draw_reply_) {
     std::move(software_draw_reply_)
-        .Run(common_renderer_params, std::move(frame.metadata));
+        .Run(common_renderer_params, NextMetadataVersion(),
+             std::move(frame.metadata));
   } else {
     NOTREACHED();
   }
@@ -331,4 +333,8 @@ void SynchronousCompositorProxy::ZoomBy(float zoom_delta,
   std::move(zoom_by_reply_).Run(common_renderer_params);
 }
 
+uint32_t SynchronousCompositorProxy::NextMetadataVersion() {
+  return ++metadata_version_;
+}
+
 }  // namespace content

content/renderer/android/synchronous_compositor_proxy.h

@@ -92,7 +92,8 @@ class SynchronousCompositorProxy : public ui::SynchronousInputHandler,
       const content::SyncCompositorCommonRendererParams&) = 0;
   virtual void SendDemandDrawHwAsyncReply(
       const content::SyncCompositorCommonRendererParams&,
-      uint32_t,
+      uint32_t layer_tree_frame_sink_id,
+      uint32_t metadata_version,
       base::Optional<viz::CompositorFrame>) = 0;
 
   DemandDrawHwCallback hardware_draw_reply_;
@@ -103,6 +104,7 @@ class SynchronousCompositorProxy : public ui::SynchronousInputHandler,
 
  private:
   void DoDemandDrawSw(const SyncCompositorDemandDrawSwParams& params);
+  uint32_t NextMetadataVersion();
 
   struct SharedMemoryWithSize;
 
@@ -128,6 +130,7 @@ class SynchronousCompositorProxy : public ui::SynchronousInputHandler,
   bool need_animate_scroll_;
   uint32_t need_invalidate_count_;
   uint32_t did_activate_pending_tree_count_;
+  uint32_t metadata_version_ = 0u;
 
   DISALLOW_COPY_AND_ASSIGN(SynchronousCompositorProxy);
 };

content/renderer/android/synchronous_compositor_proxy_chrome_ipc.cc

@@ -78,27 +78,31 @@ void SynchronousCompositorProxyChromeIPC::OnDemandDrawSw(
 void SynchronousCompositorProxyChromeIPC::SendDemandDrawHwAsyncReply(
     const SyncCompositorCommonRendererParams& common_renderer_params,
     uint32_t layer_tree_frame_sink_id,
+    uint32_t metadata_version,
     base::Optional<viz::CompositorFrame> frame) {
-  Send(new SyncCompositorHostMsg_ReturnFrame(routing_id_,
-                                             layer_tree_frame_sink_id, frame));
+  Send(new SyncCompositorHostMsg_ReturnFrame(
+      routing_id_, layer_tree_frame_sink_id, metadata_version, frame));
 }
 
 void SynchronousCompositorProxyChromeIPC::SendDemandDrawHwReply(
     IPC::Message* reply_message,
     const SyncCompositorCommonRendererParams& common_renderer_params,
     uint32_t layer_tree_frame_sink_id,
+    uint32_t metadata_version,
     base::Optional<viz::CompositorFrame> frame) {
   SyncCompositorMsg_DemandDrawHw::WriteReplyParams(
-      reply_message, common_renderer_params, layer_tree_frame_sink_id, frame);
+      reply_message, common_renderer_params, layer_tree_frame_sink_id,
+      metadata_version, frame);
   Send(reply_message);
 }
 
 void SynchronousCompositorProxyChromeIPC::SendDemandDrawSwReply(
     IPC::Message* reply_message,
     const SyncCompositorCommonRendererParams& common_renderer_params,
+    uint32_t metadata_version,
     base::Optional<viz::CompositorFrameMetadata> metadata) {
   SyncCompositorMsg_DemandDrawSw::WriteReplyParams(
-      reply_message, common_renderer_params, metadata);
+      reply_message, common_renderer_params, metadata_version, metadata);
   Send(reply_message);
 }
 

content/renderer/android/synchronous_compositor_proxy_chrome_ipc.h

@@ -35,7 +35,8 @@ class SynchronousCompositorProxyChromeIPC : public SynchronousCompositorProxy {
       const content::SyncCompositorCommonRendererParams&) final;
   void SendDemandDrawHwAsyncReply(
       const content::SyncCompositorCommonRendererParams&,
-      uint32_t,
+      uint32_t layer_tree_frame_sink_id,
+      uint32_t metadata_version,
       base::Optional<viz::CompositorFrame>) final;
 
  private:
@@ -57,10 +58,12 @@ class SynchronousCompositorProxyChromeIPC : public SynchronousCompositorProxy {
       IPC::Message* reply_message,
       const SyncCompositorCommonRendererParams& common_renderer_params,
       uint32_t layer_tree_frame_sink_id,
+      uint32_t metadata_version,
       base::Optional<viz::CompositorFrame> frame);
   void SendDemandDrawSwReply(
       IPC::Message* reply_message,
       const SyncCompositorCommonRendererParams& common_renderer_params,
+      uint32_t metadata_version,
       base::Optional<viz::CompositorFrameMetadata> metadata);
   void ZoomByReply(SyncCompositorCommonRendererParams* output_params,
                    const SyncCompositorCommonRendererParams& params);

content/renderer/android/synchronous_compositor_proxy_mojo.cc

@@ -15,8 +15,10 @@ SynchronousCompositorProxyMojo::~SynchronousCompositorProxyMojo() {}
 void SynchronousCompositorProxyMojo::SendDemandDrawHwAsyncReply(
     const content::SyncCompositorCommonRendererParams&,
     uint32_t layer_tree_frame_sink_id,
+    uint32_t metadata_version,
     base::Optional<viz::CompositorFrame> frame) {
-  control_host_->ReturnFrame(layer_tree_frame_sink_id, std::move(frame));
+  control_host_->ReturnFrame(layer_tree_frame_sink_id, metadata_version,
+                             std::move(frame));
 }
 
 void SynchronousCompositorProxyMojo::SendBeginFrameResponse(

content/renderer/android/synchronous_compositor_proxy_mojo.h

@@ -30,7 +30,8 @@ class SynchronousCompositorProxyMojo : public SynchronousCompositorProxy {
       const content::SyncCompositorCommonRendererParams&) final;
   void SendDemandDrawHwAsyncReply(
       const content::SyncCompositorCommonRendererParams&,
-      uint32_t,
+      uint32_t layer_tree_frame_sink_id,
+      uint32_t metadata_version,
       base::Optional<viz::CompositorFrame>) final;
 
  private: