From e32ae4f8fb8f8e73211ea1f1589094b106c9c8a4 Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Wed, 25 Sep 2019 12:50:23 +0200 Subject: [PATCH] Invalidate encoder rates on VideoStreamEncoder::ReconfigureEncoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without invalidation the codec configuration may not get updated. This is prone to happen when the bandwidth is below the min or above the max in which case the last_encoder_rate_settings_ may have not changed. This led to a regression in b6a45dd, where last_encoder_rate_settings_ would not change the allocation or frame rate, but the frame rate would have been set by the video adapter. Thus the frame rates were set incorrectly, leading to lower values in the regression tests. I have re-run this scenario against some of the metric drops and the regression appears to be fixed. Bug: webrtc:10126 Change-Id: I0fa6c9b71e7aff5dd80e53119db109d97eed98b6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/154343 Commit-Queue: Evan Shrubsole Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#29301} --- video/video_stream_encoder.cc | 10 ++++-- video/video_stream_encoder_unittest.cc | 45 ++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 200b4293ef..b22f326d6a 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -934,9 +934,13 @@ void VideoStreamEncoder::ReconfigureEncoder() { if (rate_allocator_ && last_encoder_rate_settings_) { // We have a new rate allocator instance and already configured target // bitrate. Update the rate allocation and notify observers. - last_encoder_rate_settings_->framerate_fps = GetInputFramerateFps(); - SetEncoderRates( - UpdateBitrateAllocationAndNotifyObserver(*last_encoder_rate_settings_)); + // We must invalidate the last_encoder_rate_settings_ to ensure + // the changes get propagated to all listeners. + EncoderRateSettings rate_settings = *last_encoder_rate_settings_; + last_encoder_rate_settings_.reset(); + rate_settings.framerate_fps = GetInputFramerateFps(); + + SetEncoderRates(UpdateBitrateAllocationAndNotifyObserver(rate_settings)); } encoder_stats_observer_->OnEncoderReconfigured(encoder_config_, streams); diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index a572506875..369d163c3a 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -4900,6 +4900,51 @@ TEST_F(VideoStreamEncoderTest, BandwidthAllocationLowerBound) { video_stream_encoder_->Stop(); } +TEST_F(VideoStreamEncoderTest, EncoderRatesPropegatedOnReconfigure) { + video_stream_encoder_->OnBitrateUpdated( + DataRate::bps(kTargetBitrateBps), DataRate::bps(kTargetBitrateBps), + DataRate::bps(kTargetBitrateBps), 0, 0); + // Capture a frame and wait for it to synchronize with the encoder thread. + int64_t timestamp_ms = fake_clock_.TimeNanos() / rtc::kNumNanosecsPerMillisec; + video_source_.IncomingCapturedFrame(CreateFrame(timestamp_ms, nullptr)); + WaitForEncodedFrame(1); + + auto prev_rate_settings = fake_encoder_.GetAndResetLastRateControlSettings(); + ASSERT_TRUE(prev_rate_settings.has_value()); + EXPECT_EQ(static_cast(prev_rate_settings->framerate_fps), + kDefaultFramerate); + + // Send 1s of video to ensure the framerate is stable at kDefaultFramerate. + for (int i = 0; i < 2 * kDefaultFramerate; i++) { + timestamp_ms += 1000 / kDefaultFramerate; + video_source_.IncomingCapturedFrame(CreateFrame(timestamp_ms, nullptr)); + WaitForEncodedFrame(timestamp_ms); + } + EXPECT_EQ(static_cast(fake_encoder_.GetLastFramerate()), + kDefaultFramerate); + // Capture larger frame to trigger a reconfigure. + codec_height_ *= 2; + codec_width_ *= 2; + timestamp_ms += 1000 / kDefaultFramerate; + video_source_.IncomingCapturedFrame(CreateFrame(timestamp_ms, nullptr)); + WaitForEncodedFrame(timestamp_ms); + + EXPECT_EQ(2, sink_.number_of_reconfigurations()); + auto current_rate_settings = + fake_encoder_.GetAndResetLastRateControlSettings(); + // Ensure we have actually reconfigured twice + // The rate settings should have been set again even though + // they haven't changed. + ASSERT_TRUE(current_rate_settings.has_value()); + EXPECT_EQ(prev_rate_settings->bitrate, current_rate_settings->bitrate); + EXPECT_EQ(prev_rate_settings->framerate_fps, + current_rate_settings->framerate_fps); + EXPECT_EQ(prev_rate_settings->bandwidth_allocation, + current_rate_settings->bandwidth_allocation); + + video_stream_encoder_->Stop(); +} + struct MockEncoderSwitchRequestCallback : public EncoderSwitchRequestCallback { MOCK_METHOD0(RequestEncoderFallback, void()); MOCK_METHOD1(RequestEncoderSwitch, void(const Config& conf));