From dd44334bc9f9deb73891547bcb1fe53f737c3c6f Mon Sep 17 00:00:00 2001 From: Per K Date: Tue, 21 May 2024 16:31:47 +0000 Subject: [PATCH] Ensure BitrateAllocator is updated with stream max bitrate after codec change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:341803760 Change-Id: I4453cf98fa98068aa94b3e091f03304d5cd4e6dc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/351142 Reviewed-by: Erik Språng Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#42361} --- video/video_send_stream_impl.cc | 13 +++--- video/video_send_stream_impl_unittest.cc | 57 ++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index 682183ea09..b42c577887 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -806,13 +806,12 @@ void VideoSendStreamImpl::OnEncoderConfigurationChanged( ? experimental_min_bitrate->bps() : std::max(streams[0].min_bitrate_bps, GetDefaultMinVideoBitrateBps(codec_type)); - - encoder_max_bitrate_bps_ = 0; double stream_bitrate_priority_sum = 0; + uint32_t encoder_max_bitrate_bps = 0; for (const auto& stream : streams) { // We don't want to allocate more bitrate than needed to inactive streams. if (stream.active) { - encoder_max_bitrate_bps_ += stream.max_bitrate_bps; + encoder_max_bitrate_bps += stream.max_bitrate_bps; } if (stream.bitrate_priority) { RTC_DCHECK_GT(*stream.bitrate_priority, 0); @@ -821,9 +820,11 @@ void VideoSendStreamImpl::OnEncoderConfigurationChanged( } RTC_DCHECK_GT(stream_bitrate_priority_sum, 0); encoder_bitrate_priority_ = stream_bitrate_priority_sum; - encoder_max_bitrate_bps_ = - std::max(static_cast(encoder_min_bitrate_bps_), - encoder_max_bitrate_bps_); + if (encoder_max_bitrate_bps > 0) { + encoder_max_bitrate_bps_ = + std::max(static_cast(encoder_min_bitrate_bps_), + encoder_max_bitrate_bps); + } // TODO(bugs.webrtc.org/10266): Query the VideoBitrateAllocator instead. max_padding_bitrate_ = CalculateMaxPadBitrateBps( diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index 548a6f677b..29ac08e065 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc @@ -73,12 +73,15 @@ namespace { using ::testing::_; using ::testing::AllOf; using ::testing::AnyNumber; +using ::testing::Eq; using ::testing::Field; using ::testing::Invoke; using ::testing::Mock; using ::testing::NiceMock; using ::testing::Return; using ::testing::SaveArg; +using ::testing::Sequence; +using ::testing::SizeIs; constexpr int64_t kDefaultInitialBitrateBps = 333000; const double kDefaultBitratePriority = 0.5; @@ -180,6 +183,7 @@ class VideoSendStreamImplTest : public ::testing::Test { encoder_config.content_type = content_type; encoder_config.simulcast_layers.push_back(VideoStream()); encoder_config.simulcast_layers.back().active = true; + encoder_config.simulcast_layers.back().bitrate_priority = 1.0; return encoder_config; } @@ -274,6 +278,59 @@ TEST_F(VideoSendStreamImplTest, vss_impl->Stop(); } +TEST_F(VideoSendStreamImplTest, + MaxBitrateCorrectIfActiveEncodingUpdatedAfterCreation) { + VideoEncoderConfig one_active_encoding = TestVideoEncoderConfig(); + ASSERT_THAT(one_active_encoding.simulcast_layers, SizeIs(1)); + one_active_encoding.max_bitrate_bps = 10'000'000; + one_active_encoding.simulcast_layers[0].max_bitrate_bps = 2'000'000; + VideoEncoderConfig no_active_encodings = one_active_encoding.Copy(); + no_active_encodings.simulcast_layers[0].active = false; + auto vss_impl = CreateVideoSendStreamImpl(no_active_encodings.Copy()); + + encoder_queue_->PostTask([&] { + static_cast(vss_impl.get()) + ->OnEncoderConfigurationChanged( + no_active_encodings.simulcast_layers, false, + VideoEncoderConfig::ContentType::kRealtimeVideo, + /*min_transmit_bitrate_bps*/ 30000); + }); + time_controller_.AdvanceTime(TimeDelta::Zero()); + + Sequence s; + // Expect codec max bitrate as max needed bitrate before the encoder has + // notifed about the actual send streams. + EXPECT_CALL(bitrate_allocator_, + AddObserver(vss_impl.get(), + Field(&MediaStreamAllocationConfig::max_bitrate_bps, + Eq(one_active_encoding.max_bitrate_bps)))) + .InSequence(s); + + // Expect the sum of active encodings as max needed bitrate after + // ->OnEncoderConfigurationChanged. + EXPECT_CALL( + bitrate_allocator_, + AddObserver( + vss_impl.get(), + Field(&MediaStreamAllocationConfig::max_bitrate_bps, + Eq(one_active_encoding.simulcast_layers[0].max_bitrate_bps)))) + .InSequence(s); + vss_impl->Start(); + // Enable encoding of a stream. + vss_impl->ReconfigureVideoEncoder(one_active_encoding.Copy()); + encoder_queue_->PostTask([&] { + static_cast(vss_impl.get()) + ->OnEncoderConfigurationChanged( + one_active_encoding.simulcast_layers, false, + VideoEncoderConfig::ContentType::kRealtimeVideo, + /*min_transmit_bitrate_bps*/ 30000); + }); + time_controller_.AdvanceTime(TimeDelta::Zero()); + + EXPECT_CALL(bitrate_allocator_, RemoveObserver(vss_impl.get())).InSequence(s); + vss_impl->Stop(); +} + TEST_F(VideoSendStreamImplTest, DoNotRegistersAsBitrateObserverOnStrayEncodedImage) { auto vss_impl = CreateVideoSendStreamImpl(TestVideoEncoderConfig());