Plumb frame_feedback_id through VideoCapture mojom interfaces

The FrameSinkVideoCaptureDevice, unlike other implementers of the VideoCaptureDevice mojom interface, actually makes use of the frame_feedback_id to send frame utilization data back in some scenarios. However, prior to this change, that id gets lost along the way during Capture on Lacros. This adds the necessary plumbing to always pass the frame_feedback_id, such that consumers can decide if they care about the id or not. Fixed: 1305434 Change-Id: I1de979bc19b752b7744181a4606e6b28c7f57e94 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3527436 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Mark Foltz <mfoltz@chromium.org> Reviewed-by: Will Cassella <cassew@chromium.org> Reviewed-by: Markus Handell <handellm@google.com> Commit-Queue: Alexander Cooper <alcooper@chromium.org> Cr-Commit-Position: refs/heads/main@{#990100}
chromium · Apr 7, 2022 · 4da5318 · 4da5318
1 parent f42f824
commit 4da5318
Show file tree

Hide file tree

Showing 32 changed files with 120 additions and 101 deletions.
diff --git a/components/mirroring/browser/single_client_video_capture_host.cc b/components/mirroring/browser/single_client_video_capture_host.cc
@@ -308,7 +308,7 @@ void SingleClientVideoCaptureHost::OnDeviceLaunchAborted() {
 
 void SingleClientVideoCaptureHost::OnFinishedConsumingBuffer(
     int buffer_context_id,
-    const media::VideoCaptureFeedback& feedback) {
+    media::VideoCaptureFeedback feedback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(observer_);
   const auto buffer_context_iter = buffer_context_map_.find(buffer_context_id);
@@ -320,8 +320,8 @@ void SingleClientVideoCaptureHost::OnFinishedConsumingBuffer(
   VideoFrameConsumerFeedbackObserver* feedback_observer =
       launched_device_.get();
   if (feedback_observer && !feedback.Empty()) {
-    feedback_observer->OnUtilizationReport(buffer_context_iter->second.first,
-                                           feedback);
+    feedback.frame_id = buffer_context_iter->second.first;
+    feedback_observer->OnUtilizationReport(feedback);
   }
   buffer_context_map_.erase(buffer_context_iter);
   const auto retired_iter = retired_buffers_.find(buffer_context_id);

diff --git a/components/mirroring/browser/single_client_video_capture_host.h b/components/mirroring/browser/single_client_video_capture_host.h
@@ -103,7 +103,7 @@ class SingleClientVideoCaptureHost final
  private:
   // Reports the |consumer_resource_utilization| and removes the buffer context.
   void OnFinishedConsumingBuffer(int buffer_context_id,
-                                 const media::VideoCaptureFeedback& feedback);
+                                 media::VideoCaptureFeedback feedback);
 
   const std::string device_id_;
   const blink::mojom::MediaStreamType type_;

diff --git a/components/mirroring/browser/single_client_video_capture_host_unittest.cc b/components/mirroring/browser/single_client_video_capture_host_unittest.cc
@@ -52,7 +52,7 @@ class MockVideoCaptureDevice final
                     uint32_t,
                     base::OnceCallback<void(media::mojom::CropRequestResult)>));
   MOCK_METHOD0(RequestRefreshFrame, void());
-  MOCK_METHOD2(OnUtilizationReport, void(int, media::VideoCaptureFeedback));
+  MOCK_METHOD1(OnUtilizationReport, void(media::VideoCaptureFeedback));
 };
 
 class FakeDeviceLauncher final : public content::VideoCaptureDeviceLauncher {
@@ -276,10 +276,9 @@ class SingleClientVideoCaptureHostTest : public ::testing::Test {
   }
 
   void FinishConsumingBuffer(int buffer_context_id,
-                             int feedback_id,
                              const media::VideoCaptureFeedback& feedback) {
     base::RunLoop run_loop;
-    EXPECT_CALL(*launched_device_, OnUtilizationReport(feedback_id, feedback))
+    EXPECT_CALL(*launched_device_, OnUtilizationReport(feedback))
         .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
     consumer_->FinishConsumingBuffer(buffer_context_id, feedback);
     run_loop.Run();
@@ -319,8 +318,11 @@ class SingleClientVideoCaptureHostTest : public ::testing::Test {
 TEST_F(SingleClientVideoCaptureHostTest, Basic) {
   CustomSetUp();
   CreateBuffer(1, 0);
-  FrameReadyInBuffer(1, 0, 5);
-  FinishConsumingBuffer(0, 5, media::VideoCaptureFeedback(1.0));
+  const int feedback_id = 5;
+  FrameReadyInBuffer(1, 0, feedback_id);
+  auto feedback = media::VideoCaptureFeedback(1.0);
+  feedback.frame_id = feedback_id;
+  FinishConsumingBuffer(0, feedback);
   RetireBuffer(1, 0);
 }
 
@@ -331,7 +333,8 @@ TEST_F(SingleClientVideoCaptureHostTest, InvalidParams) {
 TEST_F(SingleClientVideoCaptureHostTest, ReuseBufferId) {
   CustomSetUp();
   CreateBuffer(0, 0);
-  FrameReadyInBuffer(0, 0, 3);
+  const int feedback_id = 3;
+  FrameReadyInBuffer(0, 0, feedback_id);
   // Retire buffer 0. The consumer is not expected to receive OnBufferDestroyed
   // since the buffer is not returned yet.
   {
@@ -341,19 +344,24 @@ TEST_F(SingleClientVideoCaptureHostTest, ReuseBufferId) {
   }
 
   // Re-use buffer 0.
+  const int feedback_id_2 = 7;
   CreateBuffer(0, 1);
-  FrameReadyInBuffer(0, 1, 7);
+  FrameReadyInBuffer(0, 1, feedback_id_2);
 
   // Finish consuming frame in the retired buffer 0.
-  FinishConsumingBuffer(0, 3, media::VideoCaptureFeedback(1.0));
+  auto feedback = media::VideoCaptureFeedback(1.0);
+  feedback.frame_id = feedback_id;
+  FinishConsumingBuffer(0, feedback);
   // The retired buffer is expected to be destroyed since the consumer finished
   // consuming the frame in that buffer.
   base::RunLoop run_loop;
   EXPECT_CALL(*consumer_, OnBufferDestroyedCall(0))
       .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
   run_loop.Run();
 
-  FinishConsumingBuffer(1, 7, media::VideoCaptureFeedback(0.5));
+  auto feedback_2 = media::VideoCaptureFeedback(0.5);
+  feedback_2.frame_id = feedback_id_2;
+  FinishConsumingBuffer(1, feedback_2);
   RetireBuffer(0, 1);
 }
 

diff --git a/content/browser/media/capture/aura_window_to_mojo_device_adapter.cc b/content/browser/media/capture/aura_window_to_mojo_device_adapter.cc
@@ -75,8 +75,7 @@ void AuraWindowToMojoDeviceAdapter::TakePhoto(TakePhotoCallback callback) {
 
 void AuraWindowToMojoDeviceAdapter::ProcessFeedback(
     const media::VideoCaptureFeedback& feedback) {
-  // Feedback ID is not propagated by mojo interface.
-  device_->OnUtilizationReport(/*frame_feedback_id=*/0, feedback);
+  device_->OnUtilizationReport(feedback);
 }
 
 void AuraWindowToMojoDeviceAdapter::RequestRefreshFrame() {

diff --git a/content/browser/media/capture/frame_sink_video_capture_device.cc b/content/browser/media/capture/frame_sink_video_capture_device.cc
@@ -192,11 +192,13 @@ void FrameSinkVideoCaptureDevice::StopAndDeAllocate() {
 }
 
 void FrameSinkVideoCaptureDevice::OnUtilizationReport(
-    int frame_feedback_id,
     media::VideoCaptureFeedback feedback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  const auto index = static_cast<size_t>(frame_feedback_id);
+  if (!feedback.frame_id.has_value())
+    return;
+
+  const auto index = static_cast<size_t>(feedback.frame_id.value());
   DCHECK_LT(index, frame_callbacks_.size());
 
   // In most cases, we expect that the mojo InterfacePtr in |frame_callbacks_|

diff --git a/content/browser/media/capture/frame_sink_video_capture_device.h b/content/browser/media/capture/frame_sink_video_capture_device.h
@@ -86,8 +86,7 @@ class CONTENT_EXPORT FrameSinkVideoCaptureDevice
             base::OnceCallback<void(media::mojom::CropRequestResult)> callback)
       override;
   void StopAndDeAllocate() final;
-  void OnUtilizationReport(int frame_feedback_id,
-                           media::VideoCaptureFeedback feedback) final;
+  void OnUtilizationReport(media::VideoCaptureFeedback feedback) final;
 
   // FrameSinkVideoConsumer implementation.
   void OnFrameCaptured(

diff --git a/content/browser/media/capture/frame_sink_video_capture_device_unittest.cc b/content/browser/media/capture/frame_sink_video_capture_device_unittest.cc
@@ -518,16 +518,15 @@ TEST_F(FrameSinkVideoCaptureDeviceTest, CapturesAndDeliversFrames) {
         MockFrameSinkVideoConsumerFrameCallbacks& callbacks =
             callbackses[frame_number - first_frame_number];
 
-        const media::VideoCaptureFeedback fake_feedback =
-            media::VideoCaptureFeedback(static_cast<double>(frame_number) /
-                                        kNumFramesToDeliver);
+        media::VideoCaptureFeedback fake_feedback = media::VideoCaptureFeedback(
+            static_cast<double>(frame_number) / kNumFramesToDeliver);
+        fake_feedback.frame_id = receiver->TakeFeedbackId(buffer_id);
+
         EXPECT_CALL(callbacks, ProvideFeedback(fake_feedback));
         EXPECT_CALL(callbacks, Done());
         EXPECT_CALL(*receiver, OnBufferRetired(buffer_id));
 
-        const int feedback_id = receiver->TakeFeedbackId(buffer_id);
-        POST_DEVICE_METHOD_CALL(OnUtilizationReport, feedback_id,
-                                fake_feedback);
+        POST_DEVICE_METHOD_CALL(OnUtilizationReport, fake_feedback);
         receiver->ReleaseAccessPermission(buffer_id);
         WAIT_FOR_DEVICE_TASKS();
       }

diff --git a/content/browser/media/capture/screen_capture_device_android.cc b/content/browser/media/capture/screen_capture_device_android.cc
@@ -69,11 +69,13 @@ void ScreenCaptureDeviceAndroid::RequestRefreshFrame() {
 }
 
 void ScreenCaptureDeviceAndroid::OnUtilizationReport(
-    int frame_feedback_id,
     media::VideoCaptureFeedback feedback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(oracle_proxy_);
-  oracle_proxy_->OnConsumerReportingUtilization(frame_feedback_id, feedback);
+  DCHECK(feedback.frame_id.has_value());
+
+  oracle_proxy_->OnConsumerReportingUtilization(feedback.frame_id.value(),
+                                                feedback);
 }
 
 }  // namespace content
diff --git a/content/browser/media/capture/screen_capture_device_android.h b/content/browser/media/capture/screen_capture_device_android.h
@@ -33,8 +33,7 @@ class CONTENT_EXPORT ScreenCaptureDeviceAndroid
                         std::unique_ptr<Client> client) override;
   void StopAndDeAllocate() override;
   void RequestRefreshFrame() override;
-  void OnUtilizationReport(int frame_feedback_id,
-                           media::VideoCaptureFeedback feedback) override;
+  void OnUtilizationReport(media::VideoCaptureFeedback feedback) override;
 
  private:
   SEQUENCE_CHECKER(sequence_checker_);

diff --git a/content/browser/media/capture/video_capture_device_proxy_lacros.cc b/content/browser/media/capture/video_capture_device_proxy_lacros.cc
@@ -213,7 +213,6 @@ void VideoCaptureDeviceProxyLacros::TakePhoto(TakePhotoCallback callback) {
 }
 
 void VideoCaptureDeviceProxyLacros::OnUtilizationReport(
-    int frame_feedback_id,
     media::VideoCaptureFeedback feedback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 

diff --git a/content/browser/media/capture/video_capture_device_proxy_lacros.h b/content/browser/media/capture/video_capture_device_proxy_lacros.h
@@ -80,8 +80,7 @@ class CONTENT_EXPORT VideoCaptureDeviceProxyLacros
   void SetPhotoOptions(media::mojom::PhotoSettingsPtr settings,
                        SetPhotoOptionsCallback callback) final;
   void TakePhoto(TakePhotoCallback callback) final;
-  void OnUtilizationReport(int frame_feedback_id,
-                           media::VideoCaptureFeedback feedback) final;
+  void OnUtilizationReport(media::VideoCaptureFeedback feedback) final;
 
  private:
   // Helper that logs the given error |message| to the |receiver_| and then

diff --git a/content/browser/renderer_host/media/fake_video_capture_device_launcher.cc b/content/browser/renderer_host/media/fake_video_capture_device_launcher.cc
@@ -54,9 +54,8 @@ class FakeLaunchedVideoCaptureDevice
                                       base::OnceClosure done_cb) override {
     // Do nothing.
   }
-  void OnUtilizationReport(int frame_feedback_id,
-                           media::VideoCaptureFeedback feedback) override {
-    device_->OnUtilizationReport(frame_feedback_id, feedback);
+  void OnUtilizationReport(media::VideoCaptureFeedback feedback) override {
+    device_->OnUtilizationReport(feedback);
   }
 
  private:

diff --git a/content/browser/renderer_host/media/in_process_launched_video_capture_device.cc b/content/browser/renderer_host/media/in_process_launched_video_capture_device.cc
@@ -157,16 +157,14 @@ void InProcessLaunchedVideoCaptureDevice::SetDesktopCaptureWindowIdAsync(
 }
 
 void InProcessLaunchedVideoCaptureDevice::OnUtilizationReport(
-    int frame_feedback_id,
     media::VideoCaptureFeedback feedback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Unretained() is safe to use here because |device| would be null if it
   // was scheduled for shutdown and destruction, and because this task is
   // guaranteed to run before the task that destroys the |device|.
   device_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&media::VideoCaptureDevice::OnUtilizationReport,
-                                base::Unretained(device_.get()),
-                                frame_feedback_id, feedback));
+                                base::Unretained(device_.get()), feedback));
 }
 
 void InProcessLaunchedVideoCaptureDevice::

diff --git a/content/browser/renderer_host/media/in_process_launched_video_capture_device.h b/content/browser/renderer_host/media/in_process_launched_video_capture_device.h
@@ -40,8 +40,7 @@ class InProcessLaunchedVideoCaptureDevice : public LaunchedVideoCaptureDevice {
   void SetDesktopCaptureWindowIdAsync(gfx::NativeViewId window_id,
                                       base::OnceClosure done_cb) override;
 
-  void OnUtilizationReport(int frame_feedback_id,
-                           media::VideoCaptureFeedback feedback) override;
+  void OnUtilizationReport(media::VideoCaptureFeedback feedback) override;
 
  private:
   void SetDesktopCaptureWindowIdOnDeviceThread(

diff --git a/content/browser/renderer_host/media/mock_video_capture_provider.h b/content/browser/renderer_host/media/mock_video_capture_provider.h
@@ -77,8 +77,7 @@ class MockLaunchedVideoCaptureDevice : public LaunchedVideoCaptureDevice {
   MOCK_METHOD0(RequestRefreshFrame, void());
   MOCK_METHOD2(DoSetDesktopCaptureWindowId,
                void(gfx::NativeViewId window_id, base::OnceClosure* done_cb));
-  MOCK_METHOD2(OnUtilizationReport,
-               void(int frame_feedback_id, media::VideoCaptureFeedback));
+  MOCK_METHOD1(OnUtilizationReport, void(media::VideoCaptureFeedback));
 
   void GetPhotoState(
       media::VideoCaptureDevice::GetPhotoStateCallback callback) override {

diff --git a/content/browser/renderer_host/media/service_launched_video_capture_device.cc b/content/browser/renderer_host/media/service_launched_video_capture_device.cc
@@ -103,9 +103,9 @@ void ServiceLaunchedVideoCaptureDevice::SetDesktopCaptureWindowIdAsync(
 }
 
 void ServiceLaunchedVideoCaptureDevice::OnUtilizationReport(
-    int frame_feedback_id,
     media::VideoCaptureFeedback feedback) {
   DCHECK(sequence_checker_.CalledOnValidSequence());
+
   if (feedback != last_feedback_) {
     subscription_->ProcessFeedback(feedback);
     last_feedback_ = feedback;

diff --git a/content/browser/renderer_host/media/service_launched_video_capture_device.h b/content/browser/renderer_host/media/service_launched_video_capture_device.h
@@ -45,8 +45,7 @@ class ServiceLaunchedVideoCaptureDevice : public LaunchedVideoCaptureDevice {
   void SetDesktopCaptureWindowIdAsync(gfx::NativeViewId window_id,
                                       base::OnceClosure done_cb) override;
 
-  void OnUtilizationReport(int frame_feedback_id,
-                           media::VideoCaptureFeedback feedback) override;
+  void OnUtilizationReport(media::VideoCaptureFeedback feedback) override;
 
  private:
   void OnLostConnectionToSourceOrSubscription();

diff --git a/content/browser/renderer_host/media/service_video_capture_device_launcher.h b/content/browser/renderer_host/media/service_video_capture_device_launcher.h
@@ -39,8 +39,6 @@ class CONTENT_EXPORT ServiceVideoCaptureDeviceLauncher
                          base::OnceClosure done_cb) override;
   void AbortLaunch() override;
 
-  void OnUtilizationReport(int frame_feedback_id, double utilization);
-
  private:
   enum class State {
     READY_TO_LAUNCH,

diff --git a/content/browser/renderer_host/media/video_capture_controller.cc b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -218,8 +218,10 @@ void VideoCaptureController::BufferContext::DecreaseConsumerCount() {
   if (consumer_hold_count_ == 0) {
     if (consumer_feedback_observer_ != nullptr &&
         !combined_consumer_feedback_.Empty()) {
+      // We set this now since frame_feedback_id_ may be updated at anytime.
+      combined_consumer_feedback_.frame_id = frame_feedback_id_;
       consumer_feedback_observer_->OnUtilizationReport(
-          frame_feedback_id_, combined_consumer_feedback_);
+          combined_consumer_feedback_);
     }
     buffer_read_permission_.reset();
     combined_consumer_feedback_ = media::VideoCaptureFeedback();

diff --git a/content/browser/renderer_host/media/video_capture_controller_unittest.cc b/content/browser/renderer_host/media/video_capture_controller_unittest.cc
@@ -448,11 +448,10 @@ TEST_P(VideoCaptureControllerTest, NormalCaptureMultipleClients) {
   client_b_->feedback_.resource_utilization = -1.0;
   // Expect VideoCaptureController to call the load observer with a
   // resource utilization of 0.5 (the largest of all reported values).
-  const media::VideoCaptureFeedback kExpectedFeedback =
+  media::VideoCaptureFeedback kExpectedFeedback =
       media::VideoCaptureFeedback(0.5);
-  EXPECT_CALL(
-      *mock_launched_device_,
-      OnUtilizationReport(arbitrary_frame_feedback_id, kExpectedFeedback));
+  kExpectedFeedback.frame_id = arbitrary_frame_feedback_id;
+  EXPECT_CALL(*mock_launched_device_, OnUtilizationReport(kExpectedFeedback));
 
   device_client_->OnIncomingCapturedBuffer(std::move(buffer), device_format,
                                            arbitrary_reference_time_,
@@ -477,13 +476,16 @@ TEST_P(VideoCaptureControllerTest, NormalCaptureMultipleClients) {
   memset(buffer2_access->data(), buffer_no++, buffer2_access->mapped_size());
 
   client_a_->feedback_ = media::VideoCaptureFeedback(0.5, 60, 1000);
+  client_a_->feedback_.frame_id = arbitrary_frame_feedback_id_2;
   client_b_->feedback_ = media::VideoCaptureFeedback(3.14, 30);
+  client_b_->feedback_.frame_id = arbitrary_frame_feedback_id_2;
+
   // Expect VideoCaptureController to call the load observer with a
   // resource utilization of 3.14 (the largest of all reported values) and
   // sink constraints being the minimum of all reported values.
-  EXPECT_CALL(*mock_launched_device_,
-              OnUtilizationReport(arbitrary_frame_feedback_id_2,
-                                  media::VideoCaptureFeedback(3.14, 30, 1000)));
+  auto expected_feedback_2 = media::VideoCaptureFeedback(3.14, 30, 1000);
+  expected_feedback_2.frame_id = arbitrary_frame_feedback_id_2;
+  EXPECT_CALL(*mock_launched_device_, OnUtilizationReport(expected_feedback_2));
 
   device_client_->OnIncomingCapturedBuffer(std::move(buffer2), device_format,
                                            arbitrary_reference_time_,
@@ -845,9 +847,10 @@ TEST_F(VideoCaptureControllerTest, FrameFeedbackIsReportedForSequenceOfFrames) {
   for (int frame_index = 0; frame_index < kTestFrameSequenceLength;
        frame_index++) {
     const int stub_frame_feedback_id = frame_index;
-    const media::VideoCaptureFeedback stub_consumer_feedback =
+    media::VideoCaptureFeedback stub_consumer_feedback =
         media::VideoCaptureFeedback(static_cast<float>(frame_index) /
                                     kTestFrameSequenceLength);
+    stub_consumer_feedback.frame_id = stub_frame_feedback_id;
 
     client_a_->feedback_ = stub_consumer_feedback;
 
@@ -856,9 +859,8 @@ TEST_F(VideoCaptureControllerTest, FrameFeedbackIsReportedForSequenceOfFrames) {
                     ControllerIDAndSize(route_id, arbitrary_format.frame_size),
                     std::vector<ControllerIDAndSize>()))
         .Times(1);
-    EXPECT_CALL(
-        *mock_launched_device_,
-        OnUtilizationReport(stub_frame_feedback_id, stub_consumer_feedback))
+    EXPECT_CALL(*mock_launched_device_,
+                OnUtilizationReport(stub_consumer_feedback))
         .Times(1);
 
     // Device prepares and pushes a frame.