Revert of De-dupe copy requests for tab capture in DelegatedFrameHost....

Revert of De-dupe copy requests for tab capture in DelegatedFrameHost. (patchset #7 id:160001 of https://codereview.chromium.org/986823002/)

Reason for revert:
Weird frame flickering effect in capture on some systems.  https://crbug.com/469345

Original issue's description:
> De-dupe copy requests for tab capture in DelegatedFrameHost.
>
> Tab capture requests copies/readbacks in response to every delegated
> frame swap from the renderer.  These are executed in a later step, when
> the browser is composited.  Unfortunately, the browser may not be
> composited for every render frame.  This means multiple copies/readbacks
> may become enqueued, each of which is expensive, and will turn out to be
> completely redundant as the results will be identical.  This change
> detects this scenario, and de-dupes by "aborting" the older frame
> subscriber callback.
>
> BUG=464475
>
> Committed: https://crrev.com/ddee0845cc298cbba6f9d8d6ff7fb9e06073f0e8
> Cr-Commit-Position: refs/heads/master@{#319998}

TBR=danakj@chromium.org,ccameron@chromium.org,hubbe@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=464475

Review URL: https://codereview.chromium.org/1010803013

Cr-Commit-Position: refs/heads/master@{#321828}

content/browser/compositor/delegated_frame_host.cc

@@ -64,7 +64,6 @@ 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);
 }
@@ -268,38 +267,13 @@ void DelegatedFrameHost::DidReceiveFrameFromRenderer(
 
   scoped_refptr<media::VideoFrame> frame;
   RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback callback;
-  if (!frame_subscriber()->ShouldCaptureFrame(damage_rect, present_time,
-                                              &frame, &callback))
-    return;
-
-  if (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 frame subscriber delivery callback is to be aborted and the current
-    // one will be run when the existing copy request completes.  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.
-    DCHECK(!frame_subscriber_callbacks_.empty());
-    frame_subscriber_callbacks_.back().Run(false);
-    frame_subscriber_callbacks_.back() = base::Bind(callback, present_time);
-    return;
+  if (frame_subscriber()->ShouldCaptureFrame(damage_rect, present_time,
+                                             &frame, &callback)) {
+    CopyFromCompositingSurfaceToVideoFrame(
+        gfx::Rect(current_frame_size_in_dip_),
+        frame,
+        base::Bind(callback, present_time));
   }
-
-  // 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) {
-  DCHECK(!frame_subscriber_callbacks_.empty());
-  frame_subscriber_callbacks_.front().Run(success);
-  frame_subscriber_callbacks_.pop_front();
 }
 
 void DelegatedFrameHost::SwapDelegatedFrame(
@@ -880,7 +854,6 @@ 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(
@@ -930,11 +903,6 @@ 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);
 

content/browser/compositor/delegated_frame_host.h

@@ -136,8 +136,6 @@ class CONTENT_EXPORT DelegatedFrameHost
                            SkippedDelegatedFrames);
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraTest,
                            DiscardDelegatedFramesWithLocking);
-  FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraCopyRequestTest,
-                           DedupeFrameSubscriberRequests);
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraCopyRequestTest,
                            DestroyedAfterCopyRequest);
 
@@ -230,14 +228,8 @@ class CONTENT_EXPORT DelegatedFrameHost
   // cc::SurfaceFactoryClient implementation.
   void ReturnResources(const cc::ReturnedResourceArray& resources) override;
 
-  // Called to consult the current |frame_subscriber_|, to determine and maybe
-  // initiate a copy-into-video-frame request.
   void DidReceiveFrameFromRenderer(const gfx::Rect& damage_rect);
 
-  // Called when the next copy has completed for the |frame_subscriber_|, to run
-  // the next callback in the |frame_subscriber_callbacks_| queue.
-  void DeliverResultForFrameSubscriber(bool success);
-
   DelegatedFrameHostClient* const client_;
   ui::Compositor* compositor_;
 
@@ -316,17 +308,6 @@ class CONTENT_EXPORT DelegatedFrameHost
   scoped_ptr<RenderWidgetHostViewFrameSubscriber> frame_subscriber_;
   std::vector<scoped_refptr<OwnedMailbox> > idle_frame_subscriber_textures_;
 
-  // Set to true if a frame was received from the renderer and a copy request
-  // was made for the frame subscriber, but drawing has not yet started.  This
-  // is used to prevent more than one copy request being executed per draw.
-  bool frame_subscriber_copy_request_pending_;
-
-  // Stores the delivery callbacks, in order, that will be executed as each of
-  // the frame subscriber's copy-into-video-frame requests completes.  The size
-  // of this queue is always equal to the number of oustanding, de-duped copy
-  // requests.
-  std::deque<base::Callback<void(bool)>> frame_subscriber_callbacks_;
-
   // Callback used to pass the output request to the layer or to a function
   // specified by a test.
   base::Callback<void(scoped_ptr<cc::CopyOutputRequest>)>

content/browser/media/capture/video_capture_oracle.cc

@@ -119,9 +119,7 @@ bool VideoCaptureOracle::CompleteCapture(int frame_number,
                                          base::TimeTicks* frame_timestamp) {
   // Drop frame if previous frame number is higher.
   if (last_delivered_frame_number_ > frame_number) {
-    LOG(WARNING) << "Out of order frame delivery detected (have #"
-                 << frame_number << ", last was #"
-                 << last_delivered_frame_number_ << ").  Dropping frame.";
+    LOG(WARNING) << "Out of order frame delivery detected.  Dropping frame.";
     return false;
   }
   last_delivered_frame_number_ = frame_number;

content/browser/renderer_host/render_widget_host_view_aura.h

@@ -388,8 +388,6 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraTest, SoftwareDPIChange);
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraTest,
                            UpdateCursorIfOverSelf);
-  FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraCopyRequestTest,
-                           DedupeFrameSubscriberRequests);
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraCopyRequestTest,
                            DestroyedAfterCopyRequest);
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraTest,

content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

@@ -6,7 +6,6 @@
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
-#include "base/memory/scoped_vector.h"
 #include "base/memory/shared_memory.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -2059,155 +2058,22 @@ class RenderWidgetHostViewAuraCopyRequestTest
   RenderWidgetHostViewAuraCopyRequestTest()
       : callback_count_(0), result_(false) {}
 
-  void CallbackMethod(bool result) {
+  void CallbackMethod(const base::Closure& quit_closure, bool result) {
     result_ = result;
     callback_count_++;
-    quit_closure_.Run();
-  }
-
-  void RunLoopUntilCallback() {
-    base::RunLoop run_loop;
-    quit_closure_ = run_loop.QuitClosure();
-    run_loop.Run();
+    quit_closure.Run();
   }
 
   int callback_count_;
   bool result_;
 
  private:
-  base::Closure quit_closure_;
-
   DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewAuraCopyRequestTest);
 };
 
-// Tests that only one copy/readback request will be executed per one browser
-// composite operation, even when multiple render frame swaps occur in between
-// browser composites, and even if the frame subscriber desires more frames than
-// the number of browser composites.
-TEST_F(RenderWidgetHostViewAuraCopyRequestTest, DedupeFrameSubscriberRequests) {
-  gfx::Rect view_rect(100, 100);
-  scoped_ptr<cc::CopyOutputRequest> request;
-
-  view_->InitAsChild(NULL);
-  view_->GetDelegatedFrameHost()->SetRequestCopyOfOutputCallbackForTesting(
-      base::Bind(&FakeRenderWidgetHostViewAura::InterceptCopyOfOutput,
-                 base::Unretained(view_)));
-  aura::client::ParentWindowWithContext(
-      view_->GetNativeView(),
-      parent_view_->GetNativeView()->GetRootWindow(),
-      gfx::Rect());
-  view_->SetSize(view_rect.size());
-  view_->Show();
-
-  view_->BeginFrameSubscription(make_scoped_ptr(new FakeFrameSubscriber(
-      view_rect.size(),
-      base::Bind(&RenderWidgetHostViewAuraCopyRequestTest::CallbackMethod,
-                 base::Unretained(this)))).Pass());
-  int expected_callback_count = 0;
-  ASSERT_EQ(expected_callback_count, callback_count_);
-  ASSERT_FALSE(view_->last_copy_request_);
-
-  // Normal case: A browser composite executes for each render frame swap.
-  for (int i = 0; i < 3; ++i) {
-    // Renderer provides another frame.
-    view_->OnSwapCompositorFrame(
-        1, MakeDelegatedFrame(1.f, view_rect.size(), gfx::Rect(view_rect)));
-    ASSERT_TRUE(view_->last_copy_request_);
-    request = view_->last_copy_request_.Pass();
-
-    // Browser composites with the frame, executing the copy request, and then
-    // the result is delivered.
-    view_->GetDelegatedFrameHost()->OnCompositingStarted(
-        nullptr, base::TimeTicks::Now());
-    request->SendTextureResult(view_rect.size(),
-                               request->texture_mailbox(),
-                               scoped_ptr<cc::SingleReleaseCallback>());
-    view_->GetDelegatedFrameHost()->OnCompositingEnded(nullptr);
-    RunLoopUntilCallback();
-
-    // The callback should be run with success status.
-    ++expected_callback_count;
-    ASSERT_EQ(expected_callback_count, callback_count_);
-    EXPECT_TRUE(result_);
-  }
-
-  // De-duping cases: One browser composite executes per varied number of render
-  // frame swaps.
-  for (int i = 0; i < 3; ++i) {
-    const int num_swaps = 1 + i % 3;
-
-    // The renderer provides |num_swaps| frames.
-    cc::CopyOutputRequest* the_only_request = nullptr;
-    for (int j = 0; j < num_swaps; ++j) {
-      view_->OnSwapCompositorFrame(
-          1, MakeDelegatedFrame(1.f, view_rect.size(), gfx::Rect(view_rect)));
-      ASSERT_TRUE(view_->last_copy_request_);
-      if (the_only_request)
-        ASSERT_EQ(the_only_request, view_->last_copy_request_.get());
-      else
-        the_only_request = view_->last_copy_request_.get();
-      if (j > 0) {
-        ++expected_callback_count;
-        ASSERT_EQ(expected_callback_count, callback_count_);
-        EXPECT_FALSE(result_);  // The prior copy request was aborted.
-      }
-      if (j == (num_swaps - 1))
-        request = view_->last_copy_request_.Pass();
-    }
-
-    // Browser composites with the frame, executing the de-duped copy request,
-    // and then the result is delivered.
-    view_->GetDelegatedFrameHost()->OnCompositingStarted(
-        nullptr, base::TimeTicks::Now());
-    request->SendTextureResult(view_rect.size(),
-                               request->texture_mailbox(),
-                               scoped_ptr<cc::SingleReleaseCallback>());
-    view_->GetDelegatedFrameHost()->OnCompositingEnded(nullptr);
-    RunLoopUntilCallback();
-
-    // The final callback should be run with success status.
-    ++expected_callback_count;
-    ASSERT_EQ(expected_callback_count, callback_count_);
-    EXPECT_TRUE(result_);
-  }
-
-  // Multiple de-duped copy requests in-flight.
-  for (int i = 0; i < 3; ++i) {
-    const int num_in_flight = 1 + i % 3;
-    ScopedVector<cc::CopyOutputRequest> requests;
-
-    for (int j = 0; j < num_in_flight; ++j) {
-      // Renderer provides another frame.
-      view_->OnSwapCompositorFrame(
-          1, MakeDelegatedFrame(1.f, view_rect.size(), gfx::Rect(view_rect)));
-      ASSERT_TRUE(view_->last_copy_request_);
-      requests.push_back(view_->last_copy_request_.Pass());
-
-      // Browser composites with the frame, but the response to the copy request
-      // is delayed.
-      view_->GetDelegatedFrameHost()->OnCompositingStarted(
-          nullptr, base::TimeTicks::Now());
-      view_->GetDelegatedFrameHost()->OnCompositingEnded(nullptr);
-      ASSERT_EQ(expected_callback_count, callback_count_);
-    }
-
-    // Deliver each response, and expect success.
-    for (cc::CopyOutputRequest* r : requests) {
-      r->SendTextureResult(view_rect.size(),
-                           request->texture_mailbox(),
-                           scoped_ptr<cc::SingleReleaseCallback>());
-      RunLoopUntilCallback();
-      ++expected_callback_count;
-      ASSERT_EQ(expected_callback_count, callback_count_);
-      EXPECT_TRUE(result_);
-    }
-  }
-
-  // Destroy the RenderWidgetHostViewAura and ImageTransportFactory.
-  TearDownEnvironment();
-}
-
 TEST_F(RenderWidgetHostViewAuraCopyRequestTest, DestroyedAfterCopyRequest) {
+  base::RunLoop run_loop;
+
   gfx::Rect view_rect(100, 100);
   scoped_ptr<cc::CopyOutputRequest> request;
 
@@ -2225,7 +2091,8 @@ TEST_F(RenderWidgetHostViewAuraCopyRequestTest, DestroyedAfterCopyRequest) {
   scoped_ptr<FakeFrameSubscriber> frame_subscriber(new FakeFrameSubscriber(
       view_rect.size(),
       base::Bind(&RenderWidgetHostViewAuraCopyRequestTest::CallbackMethod,
-                 base::Unretained(this))));
+                 base::Unretained(this),
+                 run_loop.QuitClosure())));
 
   EXPECT_EQ(0, callback_count_);
   EXPECT_FALSE(view_->last_copy_request_);
@@ -2239,17 +2106,14 @@ TEST_F(RenderWidgetHostViewAuraCopyRequestTest, DestroyedAfterCopyRequest) {
   EXPECT_TRUE(view_->last_copy_request_->has_texture_mailbox());
   request = view_->last_copy_request_.Pass();
 
-  // Notify DelegatedFrameHost that the graphics commands were issued by calling
-  // OnCompositingStarted().  This clears the "frame subscriber de-duping" flag.
-  view_->GetDelegatedFrameHost()->OnCompositingStarted(nullptr,
-                                                       base::TimeTicks::Now());
   // Send back the mailbox included in the request. There's no release callback
   // since the mailbox came from the RWHVA originally.
   request->SendTextureResult(view_rect.size(),
                              request->texture_mailbox(),
                              scoped_ptr<cc::SingleReleaseCallback>());
-  view_->GetDelegatedFrameHost()->OnCompositingEnded(nullptr);
-  RunLoopUntilCallback();
+
+  // This runs until the callback happens.
+  run_loop.Run();
 
   // The callback should succeed.
   EXPECT_EQ(1, callback_count_);
@@ -2264,16 +2128,16 @@ TEST_F(RenderWidgetHostViewAuraCopyRequestTest, DestroyedAfterCopyRequest) {
   // Destroy the RenderWidgetHostViewAura and ImageTransportFactory.
   TearDownEnvironment();
 
-  // The DelegatedFrameHost should have run all remaining callbacks from its
-  // destructor.
-  EXPECT_EQ(2, callback_count_);
-  EXPECT_FALSE(result_);
-
-  // Send the result after-the-fact.  It goes nowhere since DelegatedFrameHost
-  // has been destroyed.
+  // Send back the mailbox included in the request. There's no release callback
+  // since the mailbox came from the RWHVA originally.
   request->SendTextureResult(view_rect.size(),
                              request->texture_mailbox(),
                              scoped_ptr<cc::SingleReleaseCallback>());
+
+  // Because the copy request callback may be holding state within it, that
+  // state must handle the RWHVA and ImageTransportFactory going away before the
+  // callback is called. This test passes if it does not crash as a result of
+  // these things being destroyed.
   EXPECT_EQ(2, callback_count_);
   EXPECT_FALSE(result_);
 }