Stop using VideoBitrateAllocationObserver in VideoStreamEncoder.

VideoBitrateAllocation is instead reported through the EncoderSink.
Enable VideoBitrateAllocation reporting from WebRtcVideoChannel::AddSendStream in preparation for
using the extension RtpVideoLayersAllocationExtension instead of RTCP XR.

Bug: webrtc:12000
Change-Id: I5ea8e4f237a1c4e84a89cbfd97ac4353d4c2984f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/186940
Commit-Queue: Per Kjellander <[email protected]>
Reviewed-by: Erik Språng <[email protected]>
Cr-Commit-Position: refs/heads/master@{#32347}

api/video/video_stream_encoder_interface.h

@@ -49,6 +49,9 @@ class VideoStreamEncoderInterface : public rtc::VideoSinkInterface<VideoFrame> {
         bool is_svc,
         VideoEncoderConfig::ContentType content_type,
         int min_transmit_bitrate_bps) = 0;
+
+    virtual void OnBitrateAllocationUpdated(
+        const VideoBitrateAllocation& allocation) = 0;
   };
 
   // If the resource is overusing, the VideoStreamEncoder will try to reduce
@@ -110,11 +113,6 @@ class VideoStreamEncoderInterface : public rtc::VideoSinkInterface<VideoFrame> {
                                 int64_t round_trip_time_ms,
                                 double cwnd_reduce_ratio) = 0;
 
-  // Register observer for the bitrate allocation between the temporal
-  // and spatial layers.
-  virtual void SetBitrateAllocationObserver(
-      VideoBitrateAllocationObserver* bitrate_observer) = 0;
-
   // Set a FecControllerOverride, through which the encoder may override
   // decisions made by FecController.
   virtual void SetFecControllerOverride(

api/video/video_stream_encoder_settings.h

@@ -39,6 +39,12 @@ class EncoderSwitchRequestCallback {
 };
 
 struct VideoStreamEncoderSettings {
+  enum class BitrateAllocationCallbackType {
+    kVideoBitrateAllocation,
+    kVideoBitrateAllocationWhenScreenSharing,
+    kVideoLayersAllocation
+  };
+
   explicit VideoStreamEncoderSettings(
       const VideoEncoder::Capabilities& capabilities)
       : capabilities(capabilities) {}
@@ -59,6 +65,11 @@ struct VideoStreamEncoderSettings {
   // Negotiated capabilities which the VideoEncoder may expect the other
   // side to use.
   VideoEncoder::Capabilities capabilities;
+
+  // TODO(bugs.webrtc.org/12000): Reporting of VideoBitrateAllocation is beeing
+  // deprecated. Instead VideoLayersAllocation should be reported.
+  BitrateAllocationCallbackType allocation_cb_type =
+      BitrateAllocationCallbackType::kVideoBitrateAllocationWhenScreenSharing;
 };
 
 }  // namespace webrtc

media/engine/webrtc_video_engine.cc

@@ -1297,6 +1297,15 @@ bool WebRtcVideoChannel::AddSendStream(const StreamParams& sp) {
       video_config_.periodic_alr_bandwidth_probing;
   config.encoder_settings.experiment_cpu_load_estimator =
       video_config_.experiment_cpu_load_estimator;
+  // TODO(bugs.webrtc.org/12000): Enable allocation callback type
+  // VideoLayersAllocation if RtpVideoLayersAllocationExtension has been
+  // negotiated in `send_rtp_extensions_`.
+  config.encoder_settings.allocation_cb_type =
+      IsEnabled(call_->trials(), "WebRTC-Target-Bitrate-Rtcp")
+          ? webrtc::VideoStreamEncoderSettings::BitrateAllocationCallbackType::
+                kVideoBitrateAllocation
+          : webrtc::VideoStreamEncoderSettings::BitrateAllocationCallbackType::
+                kVideoBitrateAllocationWhenScreenSharing;
   config.encoder_settings.encoder_factory = encoder_factory_;
   config.encoder_settings.bitrate_allocator_factory =
       bitrate_allocator_factory_;

video/end_to_end_tests/extended_reports_tests.cc

@@ -62,11 +62,13 @@ class RtcpXrObserver : public test::EndToEndTest {
   RtcpXrObserver(bool enable_rrtr,
                  bool expect_target_bitrate,
                  bool enable_zero_target_bitrate,
+                 bool enable_target_bitrate,
                  VideoEncoderConfig::ContentType content_type)
       : EndToEndTest(test::CallTest::kDefaultTimeoutMs),
         enable_rrtr_(enable_rrtr),
         expect_target_bitrate_(expect_target_bitrate),
         enable_zero_target_bitrate_(enable_zero_target_bitrate),
+        enable_target_bitrate_(enable_target_bitrate),
         content_type_(content_type),
         sent_rtcp_sr_(0),
         sent_rtcp_rr_(0),
@@ -175,6 +177,12 @@ class RtcpXrObserver : public test::EndToEndTest {
       VideoSendStream::Config* send_config,
       std::vector<VideoReceiveStream::Config>* receive_configs,
       VideoEncoderConfig* encoder_config) override {
+    if (enable_target_bitrate_) {
+      send_config->encoder_settings.allocation_cb_type =
+          VideoStreamEncoderSettings::BitrateAllocationCallbackType::
+              kVideoBitrateAllocation;
+    }
+
     if (enable_zero_target_bitrate_) {
       // Configure VP8 to be able to use simulcast.
       send_config->rtp.payload_name = "VP8";
@@ -202,6 +210,7 @@ class RtcpXrObserver : public test::EndToEndTest {
   const bool enable_rrtr_;
   const bool expect_target_bitrate_;
   const bool enable_zero_target_bitrate_;
+  const bool enable_target_bitrate_;
   const VideoEncoderConfig::ContentType content_type_;
   int sent_rtcp_sr_;
   int sent_rtcp_rr_ RTC_GUARDED_BY(&mutex_);
@@ -217,6 +226,7 @@ TEST_F(ExtendedReportsEndToEndTest,
        TestExtendedReportsWithRrtrWithoutTargetBitrate) {
   RtcpXrObserver test(/*enable_rrtr=*/true, /*expect_target_bitrate=*/false,
                       /*enable_zero_target_bitrate=*/false,
+                      /*enable_target_bitrate=*/false,
                       VideoEncoderConfig::ContentType::kRealtimeVideo);
   RunBaseTest(&test);
 }
@@ -225,6 +235,7 @@ TEST_F(ExtendedReportsEndToEndTest,
        TestExtendedReportsWithoutRrtrWithoutTargetBitrate) {
   RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/false,
                       /*enable_zero_target_bitrate=*/false,
+                      /*enable_target_bitrate=*/false,
                       VideoEncoderConfig::ContentType::kRealtimeVideo);
   RunBaseTest(&test);
 }
@@ -233,6 +244,7 @@ TEST_F(ExtendedReportsEndToEndTest,
        TestExtendedReportsWithRrtrWithTargetBitrate) {
   RtcpXrObserver test(/*enable_rrtr=*/true, /*expect_target_bitrate=*/true,
                       /*enable_zero_target_bitrate=*/false,
+                      /*enable_target_bitrate=*/false,
                       VideoEncoderConfig::ContentType::kScreen);
   RunBaseTest(&test);
 }
@@ -241,15 +253,16 @@ TEST_F(ExtendedReportsEndToEndTest,
        TestExtendedReportsWithoutRrtrWithTargetBitrate) {
   RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/true,
                       /*enable_zero_target_bitrate=*/false,
+                      /*enable_target_bitrate=*/false,
                       VideoEncoderConfig::ContentType::kScreen);
   RunBaseTest(&test);
 }
 
 TEST_F(ExtendedReportsEndToEndTest,
-       TestExtendedReportsWithoutRrtrWithTargetBitrateFromFieldTrial) {
-  test::ScopedFieldTrials field_trials("WebRTC-Target-Bitrate-Rtcp/Enabled/");
+       TestExtendedReportsWithoutRrtrWithTargetBitrateExplicitlySet) {
   RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/true,
                       /*enable_zero_target_bitrate=*/false,
+                      /*enable_target_bitrate=*/true,
                       VideoEncoderConfig::ContentType::kRealtimeVideo);
   RunBaseTest(&test);
 }
@@ -258,6 +271,7 @@ TEST_F(ExtendedReportsEndToEndTest,
        TestExtendedReportsCanSignalZeroTargetBitrate) {
   RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/true,
                       /*enable_zero_target_bitrate=*/true,
+                      /*enable_target_bitrate=*/false,
                       VideoEncoderConfig::ContentType::kScreen);
   RunBaseTest(&test);
 }

video/test/mock_video_stream_encoder.h

@@ -44,10 +44,6 @@ class MockVideoStreamEncoder : public VideoStreamEncoderInterface {
               (DataRate, DataRate, DataRate, uint8_t, int64_t, double),
               (override));
   MOCK_METHOD(void, OnFrame, (const VideoFrame&), (override));
-  MOCK_METHOD(void,
-              SetBitrateAllocationObserver,
-              (VideoBitrateAllocationObserver*),
-              (override));
   MOCK_METHOD(void,
               SetFecControllerOverride,
               (FecControllerOverride*),

video/video_send_stream.cc

@@ -29,8 +29,6 @@ namespace webrtc {
 
 namespace {
 
-constexpr char kTargetBitrateRtcpFieldTrial[] = "WebRTC-Target-Bitrate-Rtcp";
-
 size_t CalculateMaxHeaderSize(const RtpConfig& config) {
   size_t header_size = kRtpHeaderSize;
   size_t extensions_size = 0;
@@ -113,13 +111,6 @@ VideoSendStream::VideoSendStream(
   // it was created on.
   thread_sync_event_.Wait(rtc::Event::kForever);
   send_stream_->RegisterProcessThread(module_process_thread);
-  // TODO(sprang): Enable this also for regular video calls by default, if it
-  // works well.
-  if (encoder_config.content_type == VideoEncoderConfig::ContentType::kScreen ||
-      field_trial::IsEnabled(kTargetBitrateRtcpFieldTrial)) {
-    video_stream_encoder_->SetBitrateAllocationObserver(send_stream_.get());
-  }
-
   ReconfigureVideoEncoder(std::move(encoder_config));
 }
 

video/video_send_stream_impl.h

@@ -67,8 +67,7 @@ struct PacingConfig {
 // An encoder may deliver frames through the EncodedImageCallback on an
 // arbitrary thread.
 class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
-                            public VideoStreamEncoderInterface::EncoderSink,
-                            public VideoBitrateAllocationObserver {
+                            public VideoStreamEncoderInterface::EncoderSink {
  public:
   VideoSendStreamImpl(
       Clock* clock,
@@ -113,12 +112,16 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
   // Implements BitrateAllocatorObserver.
   uint32_t OnBitrateUpdated(BitrateAllocationUpdate update) override;
 
+  // Implements VideoStreamEncoderInterface::EncoderSink
   void OnEncoderConfigurationChanged(
       std::vector<VideoStream> streams,
       bool is_svc,
       VideoEncoderConfig::ContentType content_type,
       int min_transmit_bitrate_bps) override;
 
+  void OnBitrateAllocationUpdated(
+      const VideoBitrateAllocation& allocation) override;
+
   // Implements EncodedImageCallback. The implementation routes encoded frames
   // to the |payload_router_| and |config.pre_encode_callback| if set.
   // Called on an arbitrary encoder callback thread.
@@ -129,10 +132,6 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
   // Implements EncodedImageCallback.
   void OnDroppedFrame(EncodedImageCallback::DropReason reason) override;
 
-  // Implements VideoBitrateAllocationObserver.
-  void OnBitrateAllocationUpdated(
-      const VideoBitrateAllocation& allocation) override;
-
   // Starts monitoring and sends a keyframe.
   void StartupVideoSendStream();
   // Removes the bitrate observer, stops monitoring and notifies the video

video/video_send_stream_impl_unittest.cc

@@ -435,9 +435,10 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) {
         auto vss_impl = CreateVideoSendStreamImpl(
             kDefaultInitialBitrateBps, kDefaultBitratePriority,
             VideoEncoderConfig::ContentType::kScreen);
+        VideoStreamEncoderInterface::EncoderSink* const sink =
+            static_cast<VideoStreamEncoderInterface::EncoderSink*>(
+                vss_impl.get());
         vss_impl->Start();
-        VideoBitrateAllocationObserver* const observer =
-            static_cast<VideoBitrateAllocationObserver*>(vss_impl.get());
 
         // Populate a test instance of video bitrate allocation.
         VideoBitrateAllocation alloc;
@@ -449,7 +450,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) {
         // Encoder starts out paused, don't forward allocation.
         EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
             .Times(0);
-        observer->OnBitrateAllocationUpdated(alloc);
+        sink->OnBitrateAllocationUpdated(alloc);
 
         // Unpause encoder, allocation should be passed through.
         const uint32_t kBitrateBps = 100000;
@@ -460,7 +461,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) {
             ->OnBitrateUpdated(CreateAllocation(kBitrateBps));
         EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
             .Times(1);
-        observer->OnBitrateAllocationUpdated(alloc);
+        sink->OnBitrateAllocationUpdated(alloc);
 
         // Pause encoder again, and block allocations.
         EXPECT_CALL(rtp_video_sender_, GetPayloadBitrateBps())
@@ -470,7 +471,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) {
             ->OnBitrateUpdated(CreateAllocation(0));
         EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
             .Times(0);
-        observer->OnBitrateAllocationUpdated(alloc);
+        sink->OnBitrateAllocationUpdated(alloc);
 
         vss_impl->Stop();
       },
@@ -491,8 +492,9 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) {
             .WillOnce(Return(kBitrateBps));
         static_cast<BitrateAllocatorObserver*>(vss_impl.get())
             ->OnBitrateUpdated(CreateAllocation(kBitrateBps));
-        VideoBitrateAllocationObserver* const observer =
-            static_cast<VideoBitrateAllocationObserver*>(vss_impl.get());
+        VideoStreamEncoderInterface::EncoderSink* const sink =
+            static_cast<VideoStreamEncoderInterface::EncoderSink*>(
+                vss_impl.get());
 
         // Populate a test instance of video bitrate allocation.
         VideoBitrateAllocation alloc;
@@ -504,7 +506,7 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) {
         // Initial value.
         EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
             .Times(1);
-        observer->OnBitrateAllocationUpdated(alloc);
+        sink->OnBitrateAllocationUpdated(alloc);
 
         VideoBitrateAllocation updated_alloc = alloc;
         // Needs 10% increase in bitrate to trigger immediate forward.
@@ -514,22 +516,22 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) {
         // Too small increase, don't forward.
         updated_alloc.SetBitrate(0, 0, base_layer_min_update_bitrate_bps - 1);
         EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(_)).Times(0);
-        observer->OnBitrateAllocationUpdated(updated_alloc);
+        sink->OnBitrateAllocationUpdated(updated_alloc);
 
         // Large enough increase, do forward.
         updated_alloc.SetBitrate(0, 0, base_layer_min_update_bitrate_bps);
         EXPECT_CALL(rtp_video_sender_,
                     OnBitrateAllocationUpdated(updated_alloc))
             .Times(1);
-        observer->OnBitrateAllocationUpdated(updated_alloc);
+        sink->OnBitrateAllocationUpdated(updated_alloc);
 
         // This is now a decrease compared to last forward allocation, forward
         // immediately.
         updated_alloc.SetBitrate(0, 0, base_layer_min_update_bitrate_bps - 1);
         EXPECT_CALL(rtp_video_sender_,
                     OnBitrateAllocationUpdated(updated_alloc))
             .Times(1);
-        observer->OnBitrateAllocationUpdated(updated_alloc);
+        sink->OnBitrateAllocationUpdated(updated_alloc);
 
         vss_impl->Stop();
       },
@@ -550,8 +552,9 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) {
             .WillOnce(Return(kBitrateBps));
         static_cast<BitrateAllocatorObserver*>(vss_impl.get())
             ->OnBitrateUpdated(CreateAllocation(kBitrateBps));
-        VideoBitrateAllocationObserver* const observer =
-            static_cast<VideoBitrateAllocationObserver*>(vss_impl.get());
+        VideoStreamEncoderInterface::EncoderSink* const sink =
+            static_cast<VideoStreamEncoderInterface::EncoderSink*>(
+                vss_impl.get());
 
         // Populate a test instance of video bitrate allocation.
         VideoBitrateAllocation alloc;
@@ -563,7 +566,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) {
         // Initial value.
         EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
             .Times(1);
-        observer->OnBitrateAllocationUpdated(alloc);
+        sink->OnBitrateAllocationUpdated(alloc);
 
         // Move some bitrate from one layer to a new one, but keep sum the same.
         // Since layout has changed, immediately trigger forward.
@@ -574,7 +577,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) {
         EXPECT_CALL(rtp_video_sender_,
                     OnBitrateAllocationUpdated(updated_alloc))
             .Times(1);
-        observer->OnBitrateAllocationUpdated(updated_alloc);
+        sink->OnBitrateAllocationUpdated(updated_alloc);
 
         vss_impl->Stop();
       },
@@ -595,8 +598,9 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) {
             .WillRepeatedly(Return(kBitrateBps));
         static_cast<BitrateAllocatorObserver*>(vss_impl.get())
             ->OnBitrateUpdated(CreateAllocation(kBitrateBps));
-        VideoBitrateAllocationObserver* const observer =
-            static_cast<VideoBitrateAllocationObserver*>(vss_impl.get());
+        VideoStreamEncoderInterface::EncoderSink* const sink =
+            static_cast<VideoStreamEncoderInterface::EncoderSink*>(
+                vss_impl.get());
 
         // Populate a test instance of video bitrate allocation.
         VideoBitrateAllocation alloc;
@@ -618,14 +622,14 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) {
           // Initial value.
           EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
               .Times(1);
-          observer->OnBitrateAllocationUpdated(alloc);
+          sink->OnBitrateAllocationUpdated(alloc);
         }
 
         {
           // Sending same allocation again, this one should be throttled.
           EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
               .Times(0);
-          observer->OnBitrateAllocationUpdated(alloc);
+          sink->OnBitrateAllocationUpdated(alloc);
         }
 
         clock_.AdvanceTimeMicroseconds(kMaxVbaThrottleTimeMs * 1000);
@@ -634,14 +638,14 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) {
           // Sending similar allocation again after timeout, should forward.
           EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
               .Times(1);
-          observer->OnBitrateAllocationUpdated(alloc);
+          sink->OnBitrateAllocationUpdated(alloc);
         }
 
         {
           // Sending similar allocation again without timeout, throttle.
           EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc))
               .Times(0);
-          observer->OnBitrateAllocationUpdated(alloc);
+          sink->OnBitrateAllocationUpdated(alloc);
         }
 
         {