[Viz, capture] Fix corruption bug: Invalidate marked buffer on reuse and...

[Viz, capture] Fix corruption bug: Invalidate marked buffer on reuse and remark frames on size change

The marked buffer must be invalidated, because otherwise it might be used as if it had a correct latest
version of the image, while it was overwritten by a smaller image.

The second part of the CL is an optimization: If the capture size was reduced due to pool overuse, but
content is static, previously it was impossible to cache the result and unnecessary CopyOutputRequests
were constantly made.

The drawback of the change is that if captured size is changed very rapidly, more copyOutputRequests
would be made than before. In all other cases it is optimization.

Bug: chromium:1090301
Change-Id: Ia3e6f3dd8901110e5dc07ef1398fb8478717ef63
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225766
Commit-Queue: Ilya Nikolaevskiy <ilnik@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Auto-Submit: Ilya Nikolaevskiy <ilnik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774757}

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

@@ -782,8 +782,11 @@ void FrameSinkVideoCapturerImpl::DidCopyFrame(
         frame.get(), gfx::Rect(content_rect.origin(),
                                AdjustSizeForPixelFormat(result->size())));
 
-    if (content_version > content_version_in_marked_frame_) {
+    if (content_version > content_version_in_marked_frame_ ||
+        (content_version == content_version_in_marked_frame_ &&
+         frame->coded_size() != marked_frame_size_)) {
       frame_pool_.MarkFrame(*frame);
+      marked_frame_size_ = frame->coded_size();
       content_version_in_marked_frame_ = content_version;
     }
   }

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

@@ -299,6 +299,8 @@ class VIZ_SERVICE_EXPORT FrameSinkVideoCapturerImpl final
 
   int64_t content_version_in_marked_frame_ = -1;
 
+  gfx::Size marked_frame_size_;
+
   // A queue of captured frames pending delivery. This queue is used to re-order
   // frames, if they should happen to be captured out-of-order.
   struct CapturedFrame {

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

@@ -89,10 +89,11 @@ const struct SizeSet {
   // The location of the letterboxed content within each VideoFrame. All pixels
   // outside of this region should be black.
   gfx::Rect expected_content_rect;
-} kSizeSets[3] = {
+} kSizeSets[4] = {
     {gfx::Size(100, 100), gfx::Size(32, 18), gfx::Rect(6, 0, 18, 18)},
     {gfx::Size(64, 18), gfx::Size(32, 18), gfx::Rect(0, 4, 32, 8)},
-    {gfx::Size(64, 18), gfx::Size(64, 18), gfx::Rect(0, 0, 64, 18)}};
+    {gfx::Size(64, 18), gfx::Size(64, 18), gfx::Rect(0, 0, 64, 18)},
+    {gfx::Size(100, 100), gfx::Size(16, 8), gfx::Rect(0, 0, 8, 8)}};
 
 constexpr float kDefaultDeviceScaleFactor = 1.f;
 constexpr float kDefaultPageScaleFactor = 1.f;
@@ -450,6 +451,14 @@ class FrameSinkVideoCapturerTest : public testing::Test {
                                         size_set_.capture_size, false);
   }
 
+  void ForceOracleSize(const SizeSet& size_set) {
+    size_set_ = size_set;
+    oracle_->set_forced_capture_size(size_set.capture_size);
+    frame_sink_.set_size_set(size_set);
+    // No call to capturer_, because this method simulates size change requested
+    // by the oracle, internal to the capturer.
+  }
+
   const SizeSet& size_set() { return size_set_; }
 
   void NotifyFrameDamaged(
@@ -962,6 +971,104 @@ TEST_F(FrameSinkVideoCapturerTest, EventuallySendsARefreshFrame) {
   StopCapture();
 }
 
+// Tests that full capture happens on capture resolution change due to oracle,
+// but only once and resurrected frames are used after that.
+TEST_F(FrameSinkVideoCapturerTest,
+       RessurectsFramesForChangingCaptureResolution) {
+  frame_sink_.SetCopyOutputColor(YUVColor{0x80, 0x80, 0x80});
+  EXPECT_CALL(frame_sink_manager_, FindCapturableFrameSink(kFrameSinkId))
+      .WillRepeatedly(Return(&frame_sink_));
+
+  capturer_->ChangeTarget(kFrameSinkId);
+
+  MockConsumer consumer;
+  constexpr int num_refresh_frames = 3;  // Initial, plus two refreshes after
+                                         // downscale and upscale.
+  constexpr int num_update_frames = 3;
+
+  int expected_frames_count = 0;
+  int expected_copy_results = 0;
+
+  EXPECT_CALL(consumer, OnFrameCapturedMock())
+      .Times(num_refresh_frames + num_update_frames);
+  EXPECT_CALL(consumer, OnStopped()).Times(1);
+  StartCapture(&consumer);
+
+  // 1. The first frame captured automatically once the capture stats.
+  // It will be marked as the latest content in the buffer.
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
+  ++expected_copy_results;
+  ++expected_frames_count;
+  frame_sink_.SendCopyOutputResult(expected_copy_results - 1);
+  ASSERT_EQ(expected_copy_results, frame_sink_.num_copy_results());
+  EXPECT_EQ(expected_frames_count, consumer.num_frames_received());
+  consumer.SendDoneNotification(expected_copy_results - 1);
+
+  // 2. Another frame is captured, but there was no updates since the previous
+  // frame, therefore the marked frame should be resurrected, without making an
+  // actual request.
+  capturer_->RequestRefreshFrame();
+  AdvanceClockForRefreshTimer();
+  ++expected_frames_count;
+  EXPECT_EQ(expected_copy_results, frame_sink_.num_copy_results());
+  EXPECT_EQ(expected_frames_count, consumer.num_frames_received());
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
+  // If we do not advance the clock, the oracle will reject capture due to
+  // frame rate limits.
+  AdvanceClockToNextVsync();
+
+  // 3. Simulate a change in the oracle imposed capture size (e.g. due to
+  // overuse). This frame is of a different size than the cached frame and will
+  // be captured with a CopyOutputRequest.
+  ForceOracleSize(kSizeSets[3]);
+  capturer_->RequestRefreshFrame();
+  AdvanceClockForRefreshTimer();
+  ++expected_copy_results;
+  ++expected_frames_count;
+  frame_sink_.SendCopyOutputResult(expected_copy_results - 1);
+  ASSERT_EQ(expected_copy_results, frame_sink_.num_copy_results());
+  EXPECT_EQ(expected_frames_count, consumer.num_frames_received());
+  consumer.SendDoneNotification(expected_copy_results - 1);
+
+  // 4. Another frame is captured, but there was no updates since the previous
+  // frame, therefore the marked frame should be resurrected, without making an
+  // actual request.
+  capturer_->RequestRefreshFrame();
+  AdvanceClockForRefreshTimer();
+  ++expected_frames_count;
+  EXPECT_EQ(expected_copy_results, frame_sink_.num_copy_results());
+  EXPECT_EQ(expected_frames_count, consumer.num_frames_received());
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
+  // If we do not advance the clock, the oracle will reject capture due to
+  // frame rate limits.
+  AdvanceClockToNextVsync();
+
+  // 5. Simulate a change in the oracle imposed capture size (e.g. due to
+  // overuse). This frame is of a different size than the cached frame and will
+  // be captured with a CopyOutputRequest.
+  ForceOracleSize(kSizeSets[0]);
+  capturer_->RequestRefreshFrame();
+  AdvanceClockForRefreshTimer();
+  ++expected_copy_results;
+  ++expected_frames_count;
+  frame_sink_.SendCopyOutputResult(expected_copy_results - 1);
+  ASSERT_EQ(expected_copy_results, frame_sink_.num_copy_results());
+  EXPECT_EQ(expected_frames_count, consumer.num_frames_received());
+  consumer.SendDoneNotification(expected_copy_results - 1);
+
+  // 6. Another frame is captured, but there was no updates since the previous
+  // frame, therefore the marked frame should be resurrected, without making an
+  // actual request.
+  capturer_->RequestRefreshFrame();
+  AdvanceClockForRefreshTimer();
+  ++expected_frames_count;
+  EXPECT_EQ(expected_copy_results, frame_sink_.num_copy_results());
+  EXPECT_EQ(expected_frames_count, consumer.num_frames_received());
+  EXPECT_FALSE(IsRefreshRetryTimerRunning());
+
+  StopCapture();
+}
+
 // Tests that CompositorFrameMetadata variables (|device_scale_factor|,
 // |page_scale_factor| and |root_scroll_offset|) are sent along with each frame,
 // and refreshes cause variables of the cached CompositorFrameMetadata

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

@@ -38,6 +38,10 @@ scoped_refptr<VideoFrame> InterprocessFramePool::ReserveVideoFrame(
     if (it->mapping.size() < bytes_required) {
       continue;
     }
+    // The buffer will possibly be rewritten, so the marked frame may be lost.
+    if (it->mapping.memory() == marked_frame_buffer_) {
+      ClearFrameMarking();
+    }
     PooledBuffer taken = std::move(*it);
     available_buffers_.erase(it.base() - 1);
     return WrapBuffer(std::move(taken), format, size);

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

@@ -293,6 +293,38 @@ TEST(InterprocessFramePoolTest, FrameMarkingIsLostWhenBufferIsReallocated) {
   ASSERT_FALSE(pool.HasMarkedFrameWithSize(kBiggerSize));
 }
 
+TEST(InterprocessFramePoolTest, FrameMarkingIsLostWhenBufferIsReused) {
+  InterprocessFramePool pool(2);
+
+  // Reserve enough frames to hit capacity.
+  scoped_refptr<media::VideoFrame> frame1 =
+      pool.ReserveVideoFrame(kFormat, kSize);
+  scoped_refptr<media::VideoFrame> frame2 =
+      pool.ReserveVideoFrame(kFormat, kSize);
+  ASSERT_TRUE(frame1);
+  EXPECT_TRUE(frame2);
+
+  // Mark one of them
+  pool.MarkFrame(*frame1);
+  EXPECT_TRUE(pool.HasMarkedFrameWithSize(kSize));
+
+  // Release all frames
+  frame1 = nullptr;
+  frame2 = nullptr;
+
+  // Reserve all frames again but this time request a smaller size.
+  // This should lead to all buffers being reused and the marking being
+  // lost.
+  gfx::Size kSmallerSize(kSize.width() - 2, kSize.height() - 2);
+  frame1 = pool.ReserveVideoFrame(kFormat, kSmallerSize);
+  frame2 = pool.ReserveVideoFrame(kFormat, kSmallerSize);
+  EXPECT_TRUE(frame1);
+  EXPECT_TRUE(frame2);
+
+  EXPECT_FALSE(pool.HasMarkedFrameWithSize(kSize));
+  EXPECT_FALSE(pool.HasMarkedFrameWithSize(kSmallerSize));
+}
+
 TEST(InterprocessFramePoolTest, ReportsCorrectUtilization) {
   InterprocessFramePool pool(2);
   ASSERT_EQ(0.0f, pool.GetUtilization());