Disable padding for paused encoders even on reconfiguration

Bug: None
Change-Id: If5bdcd5197f82abc9d39ecacc24e58d5b92d6780
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132324
Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27561}

video/video_send_stream_impl.cc

@@ -222,6 +222,7 @@ VideoSendStreamImpl::VideoSendStreamImpl(
       call_stats_(call_stats),
       transport_(transport),
       bitrate_allocator_(bitrate_allocator),
+      disable_padding_(true),
       max_padding_bitrate_(0),
       encoder_min_bitrate_bps_(0),
       encoder_target_rate_bps_(0),
@@ -398,6 +399,7 @@ void VideoSendStreamImpl::StartupVideoSendStream() {
               SignalEncoderTimedOut();
             }
             timed_out_ = true;
+            disable_padding_ = true;
           } else if (timed_out_) {
             SignalEncoderActive();
             timed_out_ = false;
@@ -490,15 +492,17 @@ void VideoSendStreamImpl::OnBitrateAllocationUpdated(
 
 void VideoSendStreamImpl::SignalEncoderActive() {
   RTC_DCHECK_RUN_ON(worker_queue_);
+  if (rtp_video_sender_->IsActive()) {
     RTC_LOG(LS_INFO) << "SignalEncoderActive, Encoder is active.";
     bitrate_allocator_->AddObserver(this, GetAllocationConfig());
+  }
 }
 
 MediaStreamAllocationConfig VideoSendStreamImpl::GetAllocationConfig() const {
   return MediaStreamAllocationConfig{
       static_cast<uint32_t>(encoder_min_bitrate_bps_),
       encoder_max_bitrate_bps_,
-      static_cast<uint32_t>(max_padding_bitrate_),
+      static_cast<uint32_t>(disable_padding_ ? 0 : max_padding_bitrate_),
       /* priority_bitrate */ 0,
       !config_->suspend_below_min_bitrate,
       config_->track_id,
@@ -584,6 +588,20 @@ EncodedImageCallback::Result VideoSendStreamImpl::OnEncodedImage(
   // Indicate that there still is activity going on.
   activity_ = true;
 
+  auto enable_padding_task = [this]() {
+    if (disable_padding_) {
+      RTC_DCHECK_RUN_ON(worker_queue_);
+      disable_padding_ = false;
+      // To ensure that padding bitrate is propagated to the bitrate allocator.
+      SignalEncoderActive();
+    }
+  };
+  if (!worker_queue_->IsCurrent()) {
+    worker_queue_->PostTask(enable_padding_task);
+  } else {
+    enable_padding_task();
+  }
+
   EncodedImageCallback::Result result(EncodedImageCallback::Result::OK);
   if (media_transport_) {
     int64_t frame_id;

video/video_send_stream_impl.h

@@ -163,6 +163,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver,
 
   rtc::CriticalSection ivf_writers_crit_;
 
+  bool disable_padding_;
   int max_padding_bitrate_;
   int encoder_min_bitrate_bps_;
   uint32_t encoder_max_bitrate_bps_;

video/video_send_stream_impl_unittest.cc

@@ -213,16 +213,18 @@ TEST_F(VideoSendStreamImplTest, UpdatesObserverOnConfigurationChange) {
     config_.rtp.ssrcs.emplace_back(2);
 
     EXPECT_CALL(bitrate_allocator_, AddObserver(vss_impl.get(), _))
-        .WillOnce(Invoke(
+        .WillRepeatedly(Invoke(
             [&](BitrateAllocatorObserver*, MediaStreamAllocationConfig config) {
               EXPECT_EQ(config.min_bitrate_bps,
                         static_cast<uint32_t>(min_transmit_bitrate_bps));
               EXPECT_EQ(config.max_bitrate_bps,
                         static_cast<uint32_t>(qvga_stream.max_bitrate_bps +
                                               vga_stream.max_bitrate_bps));
+              if (config.pad_up_bitrate_bps != 0) {
                 EXPECT_EQ(config.pad_up_bitrate_bps,
                           static_cast<uint32_t>(qvga_stream.target_bitrate_bps +
                                                 vga_stream.min_bitrate_bps));
+              }
               EXPECT_EQ(config.enforce_min_bitrate, !kSuspend);
             }));
 
@@ -279,15 +281,17 @@ TEST_F(VideoSendStreamImplTest, UpdatesObserverOnConfigurationChangeWithAlr) {
     config_.rtp.ssrcs.emplace_back(2);
 
     EXPECT_CALL(bitrate_allocator_, AddObserver(vss_impl.get(), _))
-        .WillOnce(Invoke(
+        .WillRepeatedly(Invoke(
             [&](BitrateAllocatorObserver*, MediaStreamAllocationConfig config) {
               EXPECT_EQ(config.min_bitrate_bps,
                         static_cast<uint32_t>(low_stream.min_bitrate_bps));
               EXPECT_EQ(config.max_bitrate_bps,
                         static_cast<uint32_t>(low_stream.max_bitrate_bps +
                                               high_stream.max_bitrate_bps));
+              if (config.pad_up_bitrate_bps != 0) {
                 EXPECT_EQ(config.pad_up_bitrate_bps,
                           static_cast<uint32_t>(min_transmit_bitrate_bps));
+              }
               EXPECT_EQ(config.enforce_min_bitrate, !kSuspend);
             }));
 
@@ -335,16 +339,19 @@ TEST_F(VideoSendStreamImplTest,
     config_.rtp.ssrcs.emplace_back(2);
 
     EXPECT_CALL(bitrate_allocator_, AddObserver(vss_impl.get(), _))
-        .WillOnce(Invoke([&](BitrateAllocatorObserver*,
+        .WillRepeatedly(Invoke([&](BitrateAllocatorObserver*,
                                    MediaStreamAllocationConfig config) {
           EXPECT_EQ(config.min_bitrate_bps,
                     static_cast<uint32_t>(low_stream.min_bitrate_bps));
           EXPECT_EQ(config.max_bitrate_bps,
                     static_cast<uint32_t>(low_stream.max_bitrate_bps +
                                           high_stream.max_bitrate_bps));
-          EXPECT_EQ(config.pad_up_bitrate_bps,
+          if (config.pad_up_bitrate_bps != 0) {
+            EXPECT_EQ(
+                config.pad_up_bitrate_bps,
                 static_cast<uint32_t>(low_stream.target_bitrate_bps +
                                       1.25 * high_stream.min_bitrate_bps));
+          }
         }));
 
     static_cast<VideoStreamEncoderInterface::EncoderSink*>(vss_impl.get())
@@ -726,5 +733,96 @@ TEST_F(VideoSendStreamImplTest, CallsVideoStreamEncoderOnBitrateUpdate) {
   });
 }
 
+TEST_F(VideoSendStreamImplTest, DisablesPaddingOnPausedEncoder) {
+  int padding_bitrate = 0;
+  std::unique_ptr<VideoSendStreamImpl> vss_impl;
+
+  test_queue_.SendTask([&] {
+    vss_impl = CreateVideoSendStreamImpl(
+        kDefaultInitialBitrateBps, kDefaultBitratePriority,
+        VideoEncoderConfig::ContentType::kRealtimeVideo);
+
+    // Capture padding bitrate for testing.
+    EXPECT_CALL(bitrate_allocator_, AddObserver(vss_impl.get(), _))
+        .WillRepeatedly(Invoke(
+            [&](BitrateAllocatorObserver*, MediaStreamAllocationConfig config) {
+              padding_bitrate = config.pad_up_bitrate_bps;
+            }));
+    // If observer is removed, no padding will be sent.
+    EXPECT_CALL(bitrate_allocator_, RemoveObserver(vss_impl.get()))
+        .WillRepeatedly(
+            Invoke([&](BitrateAllocatorObserver*) { padding_bitrate = 0; }));
+
+    EXPECT_CALL(rtp_video_sender_, OnEncodedImage(_, _, _))
+        .WillRepeatedly(Return(
+            EncodedImageCallback::Result(EncodedImageCallback::Result::OK)));
+
+    config_.track_id = "test";
+    const bool kSuspend = false;
+    config_.suspend_below_min_bitrate = kSuspend;
+    config_.rtp.extensions.emplace_back(
+        RtpExtension::kTransportSequenceNumberUri, 1);
+    VideoStream qvga_stream;
+    qvga_stream.width = 320;
+    qvga_stream.height = 180;
+    qvga_stream.max_framerate = 30;
+    qvga_stream.min_bitrate_bps = 30000;
+    qvga_stream.target_bitrate_bps = 150000;
+    qvga_stream.max_bitrate_bps = 200000;
+    qvga_stream.max_qp = 56;
+    qvga_stream.bitrate_priority = 1;
+
+    int min_transmit_bitrate_bps = 30000;
+
+    config_.rtp.ssrcs.emplace_back(1);
+
+    vss_impl->Start();
+
+    // Starts without padding.
+    EXPECT_EQ(0, padding_bitrate);
+
+    // Reconfigure e.g. due to a fake frame.
+    static_cast<VideoStreamEncoderInterface::EncoderSink*>(vss_impl.get())
+        ->OnEncoderConfigurationChanged(
+            std::vector<VideoStream>{qvga_stream},
+            VideoEncoderConfig::ContentType::kRealtimeVideo,
+            min_transmit_bitrate_bps);
+    // Still no padding because no actual frames were passed, only
+    // reconfiguration happened.
+    EXPECT_EQ(0, padding_bitrate);
+
+    // Unpause encoder.
+    const uint32_t kBitrateBps = 100000;
+    EXPECT_CALL(rtp_video_sender_, GetPayloadBitrateBps())
+        .Times(1)
+        .WillOnce(Return(kBitrateBps));
+    static_cast<BitrateAllocatorObserver*>(vss_impl.get())
+        ->OnBitrateUpdated(CreateAllocation(kBitrateBps));
+
+    // A frame is encoded.
+    EncodedImage encoded_image;
+    CodecSpecificInfo codec_specific;
+    static_cast<EncodedImageCallback*>(vss_impl.get())
+        ->OnEncodedImage(encoded_image, &codec_specific, nullptr);
+    // Only after actual frame is encoded are we enabling the padding.
+    EXPECT_GT(padding_bitrate, 0);
+  });
+
+  rtc::Event done;
+  test_queue_.PostDelayedTask(
+      [&] {
+        // No padding supposed to be sent for paused observer
+        EXPECT_EQ(0, padding_bitrate);
+        testing::Mock::VerifyAndClearExpectations(&bitrate_allocator_);
+        vss_impl->Stop();
+        vss_impl.reset();
+        done.Set();
+      },
+      5000);
+
+  // Pause the test suite so that the last delayed task executes.
+  ASSERT_TRUE(done.Wait(10000));
+}
+
 }  // namespace internal
 }  // namespace webrtc