Invalidate encoder rates on VideoStreamEncoder::ReconfigureEncoder
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 <eshr@google.com>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29301}
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index 200b429..b22f326 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -934,9 +934,13 @@
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 a572506..369d163 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/video/video_stream_encoder_unittest.cc
@@ -4900,6 +4900,51 @@
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<int>(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<int>(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));
