From 5056af06787b4f7e656bb24f4b01d24239d8f59a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 2 Sep 2019 15:53:11 +0200 Subject: [PATCH] Make sure link allocation is at least as large as bitrate sum. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The VideoBitrateAllocator subclasses may actually allocate more than the target, in order to satisfy the min bitrate constraint. In this case, make sure the bandwidth allocation we signal to the encoder is at least this large. Bug: chromium:995462 Change-Id: I08b89a7c54392330d773e13c1b0a3eff42f81672 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/151125 Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#29040} --- video/video_stream_encoder.cc | 7 +++ video/video_stream_encoder_unittest.cc | 61 +++++++++++++++++++++----- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 0b558206e0..703f470b31 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -1145,6 +1145,13 @@ VideoStreamEncoder::UpdateBitrateAllocationAndNotifyObserver( EncoderRateSettings new_rate_settings = rate_settings; new_rate_settings.bitrate = new_allocation; + // VideoBitrateAllocator subclasses may allocate a bitrate higher than the + // target in order to sustain the min bitrate of the video codec. In this + // case, make sure the bandwidth allocation is at least equal the allocation + // as that is part of the document contract for that field. + new_rate_settings.bandwidth_allocation = + std::max(new_rate_settings.bandwidth_allocation, + DataRate::bps(new_rate_settings.bitrate.get_sum_bps())); if (bitrate_adjuster_) { VideoBitrateAllocation adjusted_allocation = diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 2ed4ad9b28..3b80f8544b 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -766,10 +766,11 @@ class VideoStreamEncoderTest : public ::testing::Test { expect_null_frame_ = true; } - absl::optional GetAndResetLastBitrateAllocation() { - auto allocation = last_bitrate_allocation_; - last_bitrate_allocation_.reset(); - return allocation; + absl::optional + GetAndResetLastRateControlSettings() { + auto settings = last_rate_control_settings_; + last_rate_control_settings_.reset(); + return settings; } int GetNumEncoderInitializations() const { @@ -855,7 +856,7 @@ class VideoStreamEncoderTest : public ::testing::Test { } } last_framerate_ = static_cast(parameters.framerate_fps + 0.5); - last_bitrate_allocation_ = parameters.bitrate; + last_rate_control_settings_ = parameters; RateControlParameters adjusted_paramters = parameters; adjusted_paramters.bitrate = adjusted_rate_allocation; FakeEncoder::SetRates(adjusted_paramters); @@ -884,7 +885,8 @@ class VideoStreamEncoderTest : public ::testing::Test { bool force_init_encode_failed_ RTC_GUARDED_BY(local_crit_sect_) = false; double rate_factor_ RTC_GUARDED_BY(local_crit_sect_) = 1.0; uint32_t last_framerate_ RTC_GUARDED_BY(local_crit_sect_) = 0; - absl::optional last_bitrate_allocation_; + absl::optional + last_rate_control_settings_; VideoFrame::UpdateRect last_update_rect_ RTC_GUARDED_BY(local_crit_sect_) = {0, 0, 0, 0}; std::vector last_frame_types_; @@ -3062,10 +3064,10 @@ TEST_F(VideoStreamEncoderTest, CallsBitrateObserver) { video_source_.IncomingCapturedFrame( CreateFrame(rtc::TimeMillis(), codec_width_, codec_height_)); WaitForEncodedFrame(rtc::TimeMillis()); - absl::optional bitrate_allocation = - fake_encoder_.GetAndResetLastBitrateAllocation(); + VideoBitrateAllocation bitrate_allocation = + fake_encoder_.GetAndResetLastRateControlSettings()->bitrate; // Check that encoder has been updated too, not just allocation observer. - EXPECT_EQ(bitrate_allocation->get_sum_bps(), kLowTargetBitrateBps); + EXPECT_EQ(bitrate_allocation.get_sum_bps(), kLowTargetBitrateBps); // TODO(srte): The use of millisecs here looks like an error, but the tests // fails using seconds, this should be investigated. fake_clock_.AdvanceTime(TimeDelta::ms(1) / kDefaultFps); @@ -3092,7 +3094,7 @@ TEST_F(VideoStreamEncoderTest, CallsBitrateObserver) { } // Since rates are unchanged, encoder should not be reconfigured. - EXPECT_FALSE(fake_encoder_.GetAndResetLastBitrateAllocation().has_value()); + EXPECT_FALSE(fake_encoder_.GetAndResetLastRateControlSettings().has_value()); video_stream_encoder_->Stop(); } @@ -4667,4 +4669,43 @@ TEST_F(VideoStreamEncoderTest, CopiesVideoFrameMetadataAfterDownscale) { video_stream_encoder_->Stop(); } +TEST_F(VideoStreamEncoderTest, BandwidthAllocationLowerBound) { + const int kFrameWidth = 320; + const int kFrameHeight = 180; + + // Initial rate. + video_stream_encoder_->OnBitrateUpdated( + /*target_bitrate=*/DataRate::kbps(300), + /*link_allocation=*/DataRate::kbps(300), + /*fraction_lost=*/0, + /*rtt_ms=*/0); + + // Insert a first video frame so that encoder gets configured. + int64_t timestamp_ms = fake_clock_.TimeNanos() / rtc::kNumNanosecsPerMillisec; + VideoFrame frame = CreateFrame(timestamp_ms, kFrameWidth, kFrameHeight); + frame.set_rotation(kVideoRotation_270); + video_source_.IncomingCapturedFrame(frame); + WaitForEncodedFrame(timestamp_ms); + + // Set a target rate below the minimum allowed by the codec settings. + VideoCodec codec_config = fake_encoder_.codec_config(); + DataRate min_rate = DataRate::kbps(codec_config.minBitrate); + DataRate target_rate = min_rate - DataRate::kbps(1); + video_stream_encoder_->OnBitrateUpdated( + /*target_bitrate=*/target_rate, + /*link_allocation=*/target_rate, + /*fraction_lost=*/0, + /*rtt_ms=*/0); + video_stream_encoder_->WaitUntilTaskQueueIsIdle(); + + // Target bitrate and bandwidth allocation should both be capped at min_rate. + auto rate_settings = fake_encoder_.GetAndResetLastRateControlSettings(); + ASSERT_TRUE(rate_settings.has_value()); + DataRate allocation_sum = DataRate::bps(rate_settings->bitrate.get_sum_bps()); + EXPECT_EQ(min_rate, allocation_sum); + EXPECT_EQ(rate_settings->bandwidth_allocation, min_rate); + + video_stream_encoder_->Stop(); +} + } // namespace webrtc