FrameSinkVideoCapturer: Ensure refresh frame requests.

Due to a minor oversight, the FrameSinkVideoCapturer will occasionally
drop refresh frame requests. This is because the VideoCaptureOracle can
reject the request for a frame capture, based on its own sampling
heuristics. This change adds a simple mechanism where a "retry timer" is
started if this should occur. The "retry" is canceled if other frame
captures are triggered in the meantime. Essentially, this guarantees
that the consumer will eventually receive a new video frame after making
a refresh request for one.

Bug: 785072
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ic3dd0d5ca1f8cb48193123e714ed8cf89076f953
Reviewed-on: https://chromium-review.googlesource.com/838320Reviewed-by: Xiangjun Zhang <xjz@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527093}

components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc

@@ -4,7 +4,10 @@
 
 #include "components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
+#include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 #include "components/viz/common/frame_sinks/copy_output_request.h"
 #include "components/viz/common/frame_sinks/copy_output_result.h"
@@ -43,10 +46,15 @@ constexpr media::VideoPixelFormat
 // static
 constexpr media::ColorSpace FrameSinkVideoCapturerImpl::kDefaultColorSpace;
 
+// static
+constexpr base::TimeDelta
+    FrameSinkVideoCapturerImpl::kRefreshFrameRetryInterval;
+
 FrameSinkVideoCapturerImpl::FrameSinkVideoCapturerImpl(
     FrameSinkVideoCapturerManager* frame_sink_manager)
     : frame_sink_manager_(frame_sink_manager),
       copy_request_source_(base::UnguessableToken::Create()),
+      clock_(base::DefaultTickClock::GetInstance()),
       oracle_(true /* enable_auto_throttling */),
       frame_pool_(kDesignLimitMaxFrames),
       feedback_weak_factory_(&oracle_),
@@ -195,6 +203,8 @@ void FrameSinkVideoCapturerImpl::Start(
 void FrameSinkVideoCapturerImpl::Stop() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  refresh_frame_retry_timer_.Stop();
+
   // Cancel any captures in-flight and any captured frames pending delivery.
   capture_weak_factory_.InvalidateWeakPtrs();
   oracle_.CancelAllCaptures();
@@ -255,10 +265,31 @@ void FrameSinkVideoCapturerImpl::MaybeCaptureFrame(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Consult the oracle to determine whether this frame should be captured.
-  if (!oracle_.ObserveEventAndDecideCapture(event, damage_rect, event_time)) {
+  if (oracle_.ObserveEventAndDecideCapture(event, damage_rect, event_time)) {
+    // Regardless of the type of |event|, there is no longer a need for the
+    // refresh frame retry timer to fire. The following is a no-op, if the timer
+    // was not running.
+    refresh_frame_retry_timer_.Stop();
+  } else {
     TRACE_EVENT_INSTANT1("gpu.capture", "FpsRateLimited",
                          TRACE_EVENT_SCOPE_THREAD, "trigger",
                          VideoCaptureOracle::EventAsString(event));
+
+    // If the oracle rejected a "refresh frame" request, schedule a later retry.
+    if (event == VideoCaptureOracle::kPassiveRefreshRequest ||
+        event == VideoCaptureOracle::kActiveRefreshRequest) {
+      refresh_frame_retry_timer_.Start(
+          FROM_HERE,
+          std::max(kRefreshFrameRetryInterval, oracle_.min_capture_period()),
+          base::BindRepeating(
+              [](FrameSinkVideoCapturerImpl* self,
+                 VideoCaptureOracle::Event event) {
+                self->MaybeCaptureFrame(event,
+                                        gfx::Rect(self->oracle_.source_size()),
+                                        self->clock_->NowTicks());
+              },
+              this, event));
+    }
     return;
   }
 

components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.h

@@ -14,8 +14,9 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
-#include "base/time/default_tick_clock.h"
+#include "base/time/tick_clock.h"
 #include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "base/unguessable_token.h"
 #include "components/viz/common/frame_sinks/begin_frame_args.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
@@ -105,6 +106,10 @@ class VIZ_SERVICE_EXPORT FrameSinkVideoCapturerImpl final
   // exceeding 60% of the design limit is considered "red line" operation.
   static constexpr float kTargetPipelineUtilization = 0.6f;
 
+  // The amount of time to wait before retrying a refresh frame request.
+  static constexpr base::TimeDelta kRefreshFrameRetryInterval =
+      base::TimeDelta::FromMicroseconds(base::Time::kMicrosecondsPerSecond / 4);
+
  private:
   friend class FrameSinkVideoCapturerTest;
 
@@ -158,8 +163,7 @@ class VIZ_SERVICE_EXPORT FrameSinkVideoCapturerImpl final
 
   // Use the default base::TimeTicks clock; but allow unit tests to provide a
   // replacement.
-  base::DefaultTickClock default_tick_clock_;
-  base::TickClock* clock_ = &default_tick_clock_;
+  base::TickClock* clock_;
 
   // Current image format.
   media::VideoPixelFormat pixel_format_ = kDefaultPixelFormat;
@@ -197,6 +201,11 @@ class VIZ_SERVICE_EXPORT FrameSinkVideoCapturerImpl final
   int64_t next_capture_frame_number_ = 0;
   int64_t next_delivery_frame_number_ = 0;
 
+  // When the oracle rejects a "refresh frame" request, this timer is set to
+  // auto-retry the refresh at a later point. This ensures refresh frame
+  // requests eventually result in a frame being delivered to the consumer.
+  base::OneShotTimer refresh_frame_retry_timer_;
+
   // Provides a pool of VideoFrames that can be efficiently delivered across
   // processes. The size of this pool is used to limit the maximum number of
   // frames in-flight at any one time.

components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/callback.h"
 #include "base/optional.h"
 #include "base/test/simple_test_tick_clock.h"
+#include "base/test/test_simple_task_runner.h"
 #include "base/time/time.h"
 #include "components/viz/common/frame_sinks/begin_frame_args.h"
 #include "components/viz/common/frame_sinks/copy_output_request.h"
@@ -227,7 +228,9 @@ MATCHER_P(IsLetterboxedFrame, color, "") {
 
 class FrameSinkVideoCapturerTest : public testing::Test {
  public:
-  FrameSinkVideoCapturerTest() : capturer_(&frame_sink_manager_) {}
+  FrameSinkVideoCapturerTest()
+      : retry_timer_task_runner_(new base::TestSimpleTaskRunner()),
+        capturer_(&frame_sink_manager_) {}
 
   void SetUp() override {
     // Override the capturer's TickClock with the one controlled by the tests.
@@ -235,6 +238,10 @@ class FrameSinkVideoCapturerTest : public testing::Test {
     clock_.SetNowTicks(start_time_);
     capturer_.clock_ = &clock_;
 
+    // Point the retry timer at this test's manually-controlled task runner.
+    capturer_.refresh_frame_retry_timer_.SetTaskRunner(
+        retry_timer_task_runner_);
+
     // Before setting the format, ensure the defaults are in-place. Then, for
     // these tests, set a specific format and color space.
     ASSERT_EQ(FrameSinkVideoCapturerImpl::kDefaultPixelFormat,
@@ -255,6 +262,8 @@ class FrameSinkVideoCapturerTest : public testing::Test {
     capturer_.SetResolutionConstraints(kCaptureSize, kCaptureSize, false);
   }
 
+  void TearDown() override { retry_timer_task_runner_->ClearPendingTasks(); }
+
   void AdvanceClockToNextVsync() {
     const auto num_vsyncs_elapsed =
         (clock_.NowTicks() - start_time_) / kVsyncInterval;
@@ -279,9 +288,20 @@ class FrameSinkVideoCapturerTest : public testing::Test {
     capturer_.OnFrameDamaged(ack, kSourceSize, gfx::Rect(kSourceSize));
   }
 
+  bool IsRefreshRetryTimerRunning() {
+    return capturer_.refresh_frame_retry_timer_.IsRunning();
+  }
+
+  void FireRefreshRetryTimer() {
+    ASSERT_TRUE(IsRefreshRetryTimerRunning());
+    ASSERT_TRUE(retry_timer_task_runner_->HasPendingTask());
+    retry_timer_task_runner_->RunPendingTasks();
+  }
+
  protected:
   base::TimeTicks start_time_;
   base::SimpleTestTickClock clock_;
+  scoped_refptr<base::TestSimpleTaskRunner> retry_timer_task_runner_;
   MockFrameSinkManager frame_sink_manager_;
   FakeCapturableFrameSink frame_sink_;
   FrameSinkVideoCapturerImpl capturer_;
@@ -362,6 +382,10 @@ TEST_F(FrameSinkVideoCapturerTest, SendsBlackFrameOnStartWithoutATarget) {
   capturer_.Start(std::move(consumer));
   // A copy request was not necessary.
   EXPECT_EQ(0, frame_sink_.num_copy_results());
+  // The initial black frame is the initial refresh frame. Since that was
+  // supposed to have been sent, the timer should not be running to retry
+  // later.
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
   capturer_.Stop();
 }
 
@@ -393,6 +417,7 @@ TEST_F(FrameSinkVideoCapturerTest, CapturesCompositedFrames) {
   // frame. Simulate a copy result and expect to see the refresh frame delivered
   // to the consumer.
   ASSERT_EQ(num_refresh_frames, frame_sink_.num_copy_results());
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
   frame_sink_.SendCopyOutputResult(0);
   ASSERT_EQ(num_refresh_frames, consumer->num_frames_received());
   EXPECT_THAT(consumer->TakeFrame(0),
@@ -701,13 +726,11 @@ TEST_F(FrameSinkVideoCapturerTest, CancelsInFlightCapturesOnStop) {
 
   // Note: Because the clock hasn't advanced while switching consumers, the
   // capturer won't send a refresh frame. This is because the VideoCaptureOracle
-  // thinks the frame rate would be too fast.
-  //
-  // TODO(crbug.com/785072): Revisit this because the capturer should always
-  // guarantee an initial video frame is sent to the consumer within a
-  // reasonable time period. In practice, it does; but if things happen too
-  // quickly, it might not.
+  // thinks the frame rate would be too fast. However, the refresh frame retry
+  // timer should be running. It will be canceled when the next composite-
+  // triggered capture occurs (in the loop below).
   num_refresh_frames = 0;
+  EXPECT_TRUE(IsRefreshRetryTimerRunning());
 
   // From here, any new copy requests should be executed with video frames
   // delivered to the consumer containing |color2|.
@@ -717,6 +740,7 @@ TEST_F(FrameSinkVideoCapturerTest, CancelsInFlightCapturesOnStop) {
     NotifyFrameDamaged(num_copy_requests);
     ++num_copy_requests;
     ASSERT_EQ(num_copy_requests, frame_sink_.num_copy_results());
+    ASSERT_FALSE(IsRefreshRetryTimerRunning());
     frame_sink_.SendCopyOutputResult(frame_sink_.num_copy_results() - 1);
     ++num_completed_captures;
     ASSERT_EQ(num_completed_captures, consumer2->num_frames_received());
@@ -727,4 +751,67 @@ TEST_F(FrameSinkVideoCapturerTest, CancelsInFlightCapturesOnStop) {
   capturer_.Stop();
 }
 
+// Tests that refresh requests ultimately result in a frame being delivered to
+// the consumer.
+TEST_F(FrameSinkVideoCapturerTest, EventuallySendsARefreshFrame) {
+  frame_sink_.SetCopyOutputColor(YUVColor{0x80, 0x80, 0x80});
+  EXPECT_CALL(frame_sink_manager_, FindCapturableFrameSink(kFrameSinkId))
+      .WillRepeatedly(Return(&frame_sink_));
+
+  capturer_.ChangeTarget(kFrameSinkId);
+
+  MockConsumer* consumer;
+  {
+    auto mock_consumer = std::make_unique<MockConsumer>();
+    consumer = mock_consumer.get();
+    capturer_.Start(std::move(mock_consumer));
+  }
+  const int num_refresh_frames = 2;  // Initial, plus later refresh.
+  const int num_update_frames = 3;
+  EXPECT_CALL(*consumer, OnFrameCapturedMock(_, _, _))
+      .Times(num_refresh_frames + num_update_frames);
+  EXPECT_CALL(*consumer, OnTargetLost(_)).Times(0);
+  EXPECT_CALL(*consumer, OnStopped()).Times(1);
+
+  // To start, the capturer will make a copy request for the initial refresh
+  // frame. Simulate a copy result and expect to see the refresh frame delivered
+  // to the consumer. The capturer should not have started the retry timer for
+  // this initial refresh frame.
+  ASSERT_EQ(1, frame_sink_.num_copy_results());
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
+  frame_sink_.SendCopyOutputResult(0);
+  ASSERT_EQ(1, consumer->num_frames_received());
+  consumer->SendDoneNotification(0);
+
+  // Drive the capturer pipeline for a series of frame composites.
+  int num_frames = 1 + num_update_frames;
+  for (int i = 1; i < num_frames; ++i) {
+    AdvanceClockToNextVsync();
+    NotifyBeginFrame(i);
+    NotifyFrameDamaged(i);
+    ASSERT_EQ(i + 1, frame_sink_.num_copy_results());
+    frame_sink_.SendCopyOutputResult(i);
+    ASSERT_EQ(i + 1, consumer->num_frames_received());
+    consumer->SendDoneNotification(i);
+  }
+
+  // Without advancing the clock, request a refresh frame. The oracle will
+  // reject the request, and the retry timer will be started.
+  capturer_.RequestRefreshFrame();
+  ASSERT_EQ(num_frames, frame_sink_.num_copy_results());
+  EXPECT_TRUE(IsRefreshRetryTimerRunning());
+
+  // Simulate the elapse of time and the firing of the refresh retry timer. The
+  // oracle should allow this later retry request and deliver a refresh frame.
+  clock_.Advance(FrameSinkVideoCapturerImpl::kRefreshFrameRetryInterval);
+  FireRefreshRetryTimer();
+  ++num_frames;
+  ASSERT_EQ(num_frames, frame_sink_.num_copy_results());
+  frame_sink_.SendCopyOutputResult(num_frames - 1);
+  ASSERT_EQ(num_frames, consumer->num_frames_received());
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
+
+  capturer_.Stop();
+}
+
 }  // namespace viz