Add a feedback callback to mirroring service

This replaces VideoFrameFeedback member in VideoFrame, which was used
before to pass resource utilization to the capturer.

The follow-up CL would decouple VideoFrameFeedback from media::VideoFrame.

Bug: chromium:1134073
Change-Id: I55697a98c0c04d150dae70c53ddd9348f36c4038
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2495022
Commit-Queue: Ilya Nikolaevskiy <ilnik@chromium.org>
Auto-Submit: Ilya Nikolaevskiy <ilnik@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825775}

components/mirroring/service/rtp_stream_unittest.cc

@@ -92,7 +92,7 @@ TEST_F(RtpStreamTest, VideoStreaming) {
   auto video_sender = std::make_unique<media::cast::VideoSender>(
       cast_environment_, media::cast::GetDefaultVideoSenderConfig(),
       base::DoNothing(), base::DoNothing(), base::DoNothing(), &transport_,
-      base::DoNothing());
+      base::DoNothing(), base::DoNothing());
   VideoRtpStream video_stream(std::move(video_sender), client_.GetWeakPtr());
   {
     base::RunLoop run_loop;

components/mirroring/service/session.cc

@@ -771,6 +771,8 @@ void Session::OnAnswer(const std::vector<FrameSenderConfig>& audio_configs,
                               weak_factory_.GetWeakPtr()),
           cast_transport_.get(),
           base::BindRepeating(&Session::SetTargetPlayoutDelay,
+                              weak_factory_.GetWeakPtr()),
+          base::BindRepeating(&Session::ProcessFeedback,
                               weak_factory_.GetWeakPtr()));
       video_stream_ = std::make_unique<VideoRtpStream>(
           std::move(video_sender), weak_factory_.GetWeakPtr());
@@ -834,6 +836,12 @@ void Session::SetTargetPlayoutDelay(base::TimeDelta playout_delay) {
     video_stream_->SetTargetPlayoutDelay(playout_delay);
 }
 
+void Session::ProcessFeedback(const media::VideoFrameFeedback& feedback) {
+  if (video_capture_client_) {
+    video_capture_client_->ProcessFeedback(feedback);
+  }
+}
+
 // TODO(issuetracker.google.com/159352836): Refactor to use libcast's
 // OFFER message format.
 void Session::CreateAndSendOffer() {

components/mirroring/service/session.h

@@ -20,6 +20,7 @@
 #include "components/mirroring/service/rtp_stream.h"
 #include "components/mirroring/service/wifi_status_monitor.h"
 #include "gpu/config/gpu_info.h"
+#include "media/base/video_frame_feedback.h"
 #include "media/cast/cast_environment.h"
 #include "media/cast/net/cast_transport_defines.h"
 #include "media/mojo/mojom/video_encode_accelerator.mojom.h"
@@ -141,6 +142,9 @@ class COMPONENT_EXPORT(MIRRORING_SERVICE) Session final
   // Callback by media::cast::VideoSender to set a new target playout delay.
   void SetTargetPlayoutDelay(base::TimeDelta playout_delay);
 
+  // Callback by media::cast::VideoSender to report resource utilization.
+  void ProcessFeedback(const media::VideoFrameFeedback& feedback);
+
   media::VideoEncodeAccelerator::SupportedProfiles GetSupportedVeaProfiles();
 
   // Create and send OFFER message.

components/mirroring/service/video_capture_client.cc

@@ -235,7 +235,7 @@ void VideoCaptureClient::OnBufferReady(int32_t buffer_id,
   }
   frame->AddDestructionObserver(
       base::BindOnce(&VideoCaptureClient::DidFinishConsumingFrame,
-                     frame->feedback(), std::move(buffer_finished_callback)));
+                     std::move(buffer_finished_callback)));
 
   frame->set_metadata(info->metadata);
   if (info->color_space.has_value())
@@ -258,8 +258,7 @@ void VideoCaptureClient::OnBufferDestroyed(int32_t buffer_id) {
 
 void VideoCaptureClient::OnClientBufferFinished(
     int buffer_id,
-    base::ReadOnlySharedMemoryMapping mapping,
-    media::VideoFrameFeedback feedback) {
+    base::ReadOnlySharedMemoryMapping mapping) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DVLOG(3) << __func__ << ": buffer_id=" << buffer_id;
 
@@ -269,17 +268,23 @@ void VideoCaptureClient::OnClientBufferFinished(
     return;
   }
 
-  video_capture_host_->ReleaseBuffer(DeviceId(), buffer_id, feedback);
+  video_capture_host_->ReleaseBuffer(DeviceId(), buffer_id, feedback_);
+  feedback_ = media::VideoFrameFeedback();
 }
 
 // static
 void VideoCaptureClient::DidFinishConsumingFrame(
-    const media::VideoFrameFeedback* feedback,
     BufferFinishedCallback callback) {
   // Note: This function may be called on any thread by the VideoFrame
-  // destructor.  |feedback| is still valid for read-access at this point.
+  // destructor.
   DCHECK(!callback.is_null());
-  std::move(callback).Run(*feedback);
+  std::move(callback).Run();
+}
+
+void VideoCaptureClient::ProcessFeedback(
+    const media::VideoFrameFeedback& feedback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  feedback_ = feedback;
 }
 
 }  // namespace mirroring

components/mirroring/service/video_capture_client.h

@@ -48,6 +48,9 @@ class COMPONENT_EXPORT(MIRRORING_SERVICE) VideoCaptureClient
 
   void Resume(FrameDeliverCallback deliver_callback);
 
+  // Feedback callback.
+  void ProcessFeedback(const media::VideoFrameFeedback& feedback);
+
   // Requests to receive a refreshed captured video frame. Do nothing if the
   // capturing device is not started or the capturing is paused.
   void RequestRefreshFrame();
@@ -61,16 +64,13 @@ class COMPONENT_EXPORT(MIRRORING_SERVICE) VideoCaptureClient
   void OnBufferDestroyed(int32_t buffer_id) override;
 
  private:
-  using BufferFinishedCallback =
-      base::OnceCallback<void(media::VideoFrameFeedback)>;
+  using BufferFinishedCallback = base::OnceCallback<void()>;
   // Called by the VideoFrame destructor.
-  static void DidFinishConsumingFrame(const media::VideoFrameFeedback* feedback,
-                                      BufferFinishedCallback callback);
+  static void DidFinishConsumingFrame(BufferFinishedCallback callback);
 
   // Reports the utilization, unmaps the shared memory, and returns the buffer.
   void OnClientBufferFinished(int buffer_id,
-                              base::ReadOnlySharedMemoryMapping mapping,
-                              media::VideoFrameFeedback feedback);
+                              base::ReadOnlySharedMemoryMapping mapping);
 
   const media::VideoCaptureParams params_;
   const mojo::Remote<media::mojom::VideoCaptureHost> video_capture_host_;
@@ -104,6 +104,9 @@ class COMPONENT_EXPORT(MIRRORING_SERVICE) VideoCaptureClient
   // |buffer_id| is the key to this map.
   MappingMap mapped_buffers_;
 
+  // Latest received feedback.
+  media::VideoFrameFeedback feedback_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   base::WeakPtrFactory<VideoCaptureClient> weak_factory_{this};

components/mirroring/service/video_capture_client_unittest.cc

@@ -61,7 +61,7 @@ class VideoCaptureClientTest : public ::testing::Test,
 
   MOCK_METHOD1(OnFrameReceived, void(const gfx::Size&));
   void OnFrameReady(scoped_refptr<media::VideoFrame> video_frame) {
-    *video_frame->feedback() = kFeedback;
+    client_->ProcessFeedback(kFeedback);
     OnFrameReceived(video_frame->coded_size());
   }
 

media/cast/cast_sender_impl.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -131,13 +132,15 @@ void CastSenderImpl::InitializeVideo(
 
   VLOG(1) << "CastSenderImpl@" << this << "::InitializeVideo()";
 
+  // No feedback callback, since it's ignored for CastSender.
   video_sender_ = std::make_unique<VideoSender>(
       cast_environment_, video_config,
       base::BindRepeating(&CastSenderImpl::OnVideoStatusChange,
                           weak_factory_.GetWeakPtr(), status_change_cb),
       create_vea_cb, create_video_encode_mem_cb, transport_sender_,
       base::BindRepeating(&CastSenderImpl::SetTargetPlayoutDelay,
-                          weak_factory_.GetWeakPtr()));
+                          weak_factory_.GetWeakPtr()),
+      media::VideoCaptureFeedbackCB());
   if (audio_sender_) {
     DCHECK(audio_sender_->GetTargetPlayoutDelay() ==
            video_sender_->GetTargetPlayoutDelay());

media/cast/sender/video_sender.cc

@@ -93,7 +93,8 @@ VideoSender::VideoSender(
     const CreateVideoEncodeAcceleratorCallback& create_vea_cb,
     const CreateVideoEncodeMemoryCallback& create_video_encode_mem_cb,
     CastTransport* const transport_sender,
-    PlayoutDelayChangeCB playout_delay_change_cb)
+    PlayoutDelayChangeCB playout_delay_change_cb,
+    media::VideoCaptureFeedbackCB feedback_callback)
     : FrameSender(
           cast_environment,
           transport_sender,
@@ -108,6 +109,7 @@ VideoSender::VideoSender(
       frames_in_encoder_(0),
       last_bitrate_(0),
       playout_delay_change_cb_(std::move(playout_delay_change_cb)),
+      feedback_cb_(feedback_callback),
       low_latency_mode_(false),
       last_reported_encoder_utilization_(-1.0),
       last_reported_lossy_utilization_(-1.0) {
@@ -326,10 +328,13 @@ void VideoSender::OnEncodedVideoFrame(
     // Key frames are artificially capped to 1.0 because their actual
     // utilization is atypical compared to the other frames in the stream, and
     // this can misguide the producer of the input video frames.
-    video_frame->feedback()->resource_utilization =
+    VideoFrameFeedback feedback;
+    feedback.resource_utilization =
         encoded_frame->dependency == EncodedFrame::KEY
             ? std::min(1.0, attenuated_utilization)
             : attenuated_utilization;
+    if (feedback_cb_)
+      feedback_cb_.Run(feedback);
   }
 
   SendEncodedFrame(encoder_bitrate, std::move(encoded_frame));

media/cast/sender/video_sender.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
+#include "media/base/video_frame_feedback.h"
 #include "media/cast/cast_config.h"
 #include "media/cast/cast_sender.h"
 #include "media/cast/common/rtp_time.h"
@@ -45,7 +46,8 @@ class VideoSender : public FrameSender {
               const CreateVideoEncodeAcceleratorCallback& create_vea_cb,
               const CreateVideoEncodeMemoryCallback& create_video_encode_mem_cb,
               CastTransport* const transport_sender,
-              PlayoutDelayChangeCB playout_delay_change_cb);
+              PlayoutDelayChangeCB playout_delay_change_cb,
+              media::VideoCaptureFeedbackCB feedback_callback);
 
   ~VideoSender() override;
 
@@ -93,6 +95,8 @@ class VideoSender : public FrameSender {
 
   PlayoutDelayChangeCB playout_delay_change_cb_;
 
+  media::VideoCaptureFeedbackCB feedback_cb_;
+
   // Indicates we are operating in a mode where the target playout latency is
   // low for best user experience. When operating in low latency mode, we
   // prefer dropping frames over increasing target playout time.

media/cast/sender/video_sender_unittest.cc

@@ -120,9 +120,21 @@ class PeerVideoSender : public VideoSender {
                     create_vea_cb,
                     create_video_encode_mem_cb,
                     transport_sender,
-                    base::BindRepeating(&IgnorePlayoutDelayChanges)) {}
+                    base::BindRepeating(&IgnorePlayoutDelayChanges),
+                    base::BindRepeating(&PeerVideoSender::ProcessFeedback,
+                                        base::Unretained(this))) {}
+
   using VideoSender::OnReceivedCastFeedback;
   using VideoSender::OnReceivedPli;
+
+  void ProcessFeedback(const media::VideoFrameFeedback& feedback) {
+    feedback_ = feedback;
+  }
+
+  VideoFrameFeedback GetFeedback() { return feedback_; }
+
+ private:
+  VideoFrameFeedback feedback_;
 };
 
 class TransportClient : public CastTransport::Client {
@@ -567,13 +579,12 @@ TEST_F(VideoSenderTest, CheckVideoFrameFactoryIsNull) {
   EXPECT_EQ(nullptr, video_sender_->CreateVideoFrameFactory().get());
 }
 
-TEST_F(VideoSenderTest, PopulatesResourceUtilizationInFrameFeedback) {
+TEST_F(VideoSenderTest, ReportsResourceUtilizationInCallback) {
   InitEncoder(false, true);
   ASSERT_EQ(STATUS_INITIALIZED, operational_status_);
 
   for (int i = 0; i < 3; ++i) {
     scoped_refptr<media::VideoFrame> video_frame = GetNewVideoFrame();
-    EXPECT_LE(video_frame->feedback()->resource_utilization, 0.0);
 
     const base::TimeTicks reference_time = testing_clock_.NowTicks();
     video_sender_->InsertRawVideoFrame(video_frame, reference_time);
@@ -586,7 +597,7 @@ TEST_F(VideoSenderTest, PopulatesResourceUtilizationInFrameFeedback) {
     // Check that the resource_utilization value is set and non-negative.  Don't
     // check for specific values because they are dependent on real-world CPU
     // encode time, which can vary across test runs.
-    double utilization = video_frame->feedback()->resource_utilization;
+    double utilization = video_sender_->GetFeedback().resource_utilization;
     EXPECT_LE(0.0, utilization);
     if (i == 0)
       EXPECT_GE(1.0, utilization);  // Key frames never exceed 1.0.