De-dupe copy requests for tab capture in DelegatedFrameHost.

content/browser/compositor/delegated_frame_host.cc

Index: content/browser/compositor/delegated_frame_host.cc
diff --git a/content/browser/compositor/delegated_frame_host.cc b/content/browser/compositor/delegated_frame_host.cc
index e6cb16150673df0c4db6cc80b28610e4154792a2..2b4110352d8a5ed18e5edeeab33d3a7084cae71a 100644
--- a/content/browser/compositor/delegated_frame_host.cc
+++ b/content/browser/compositor/delegated_frame_host.cc
@@ -64,6 +64,7 @@ DelegatedFrameHost::DelegatedFrameHost(DelegatedFrameHostClient* client)
       skipped_frames_(false),
       current_scale_factor_(1.f),
       can_lock_compositor_(YES_CAN_LOCK),
+      frame_subscriber_copy_request_pending_(false),
       delegated_frame_evictor_(new DelegatedFrameEvictor(this)) {
   ImageTransportFactory::GetInstance()->AddObserver(this);
 }
@@ -267,13 +268,40 @@ void DelegatedFrameHost::DidReceiveFrameFromRenderer(
 
   scoped_refptr<media::VideoFrame> frame;
   RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback callback;
-  if (frame_subscriber()->ShouldCaptureFrame(damage_rect, present_time,
-                                             &frame, &callback)) {
-    CopyFromCompositingSurfaceToVideoFrame(
-        gfx::Rect(current_frame_size_in_dip_),
-        frame,
-        base::Bind(callback, present_time));
+  if (!frame_subscriber()->ShouldCaptureFrame(damage_rect, present_time,
+                                              &frame, &callback))
+    return;
+
+  if (!frame_subscriber_callbacks_.empty() &&

[hubbe, 2015/03/10 17:06:53]

The empty() call should not be required, as it should not be possible for the
queue to be empty if frame_subscriber_copy_request_pending_ is true. Perhaps
make it a DCHECK instead?

[miu, 2015/03/10 20:54:07]

Done.

+      frame_subscriber_copy_request_pending_) {
+    // A copy request was made for the previous frame from the renderer, but
+    // drawing never started (which executes the request).  Therefore, the
+    // prior request is to be aborted.  This de-duping check must be done after
+    // the call to ShouldCaptureFrame() above, since the frame subscriber makes
+    // decisions based on the renderer's intentions, and not the performance of
+    // the browser compositor.
+    frame_subscriber_callbacks_.back().Run(false);
+    frame_subscriber_callbacks_.back() = base::Bind(callback, present_time);
+    return;
+  }
+
+  // Start a new copy request.
+  frame_subscriber_copy_request_pending_ = true;
+  frame_subscriber_callbacks_.push_back(base::Bind(callback, present_time));
+  CopyFromCompositingSurfaceToVideoFrame(
+      gfx::Rect(current_frame_size_in_dip_),
+      frame,
+      base::Bind(&DelegatedFrameHost::DeliverResultForFrameSubscriber,
+                 AsWeakPtr()));
+}
+
+void DelegatedFrameHost::DeliverResultForFrameSubscriber(bool success) {
+  if (frame_subscriber_callbacks_.empty()) {
+    NOTREACHED();

[danakj, 2015/03/10 17:17:17]

Don't do "NOTREACHED(); return;" please. Either DCHECK() and don't handle the
case (it can't happen right?) or don't dcheck and handle it because it can
happen. I think here it can't happen, so just DCHECK(!empty()).

[miu, 2015/03/10 20:54:07]

Done.

+    return;
   }
+  frame_subscriber_callbacks_.front().Run(success);
+  frame_subscriber_callbacks_.pop_front();
 }
 
 void DelegatedFrameHost::SwapDelegatedFrame(
@@ -854,6 +882,7 @@ void DelegatedFrameHost::OnCompositingDidCommit(
 void DelegatedFrameHost::OnCompositingStarted(
     ui::Compositor* compositor, base::TimeTicks start_time) {
   last_draw_ended_ = start_time;
+  frame_subscriber_copy_request_pending_ = false;
 }
 
 void DelegatedFrameHost::OnCompositingEnded(
@@ -903,6 +932,11 @@ void DelegatedFrameHost::OnLostResources() {
 // DelegatedFrameHost, private:
 
 DelegatedFrameHost::~DelegatedFrameHost() {
+  while (!frame_subscriber_callbacks_.empty()) {
+    frame_subscriber_callbacks_.front().Run(false);
+    frame_subscriber_callbacks_.pop_front();
+  }
+
   DCHECK(!compositor_);
   ImageTransportFactory::GetInstance()->RemoveObserver(this);