From 809198edfff416fce8d75b574a43afab5e67b1cd Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Tue, 17 Sep 2019 13:00:04 +0200 Subject: [PATCH] Fix minor regression caused by a8336d3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VideoEncoder::SetRates was being called unnessesarily when the fields appended to RateControlParameters were changed. Since SetRates only cares about RateControlParameters, it should have only been called if the RateControlParameters themselves were actually changed. Bug: webrtc:10126 Change-Id: Ic47d67e642a3043307fec950e5fba970d9f95167 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/152829 Reviewed-by: Erik Språng Commit-Queue: Evan Shrubsole Cr-Commit-Position: refs/heads/master@{#29208} --- api/video_codecs/video_encoder.cc | 11 ++++ api/video_codecs/video_encoder.h | 3 + video/video_stream_encoder.cc | 53 ++++++++--------- video/video_stream_encoder.h | 3 +- video/video_stream_encoder_unittest.cc | 78 ++++++++++++++++++++++++++ 5 files changed, 121 insertions(+), 27 deletions(-) diff --git a/api/video_codecs/video_encoder.cc b/api/video_codecs/video_encoder.cc index d3f16a0053..3a848f39ed 100644 --- a/api/video_codecs/video_encoder.cc +++ b/api/video_codecs/video_encoder.cc @@ -116,6 +116,17 @@ VideoEncoder::RateControlParameters::RateControlParameters( framerate_fps(framerate_fps), bandwidth_allocation(bandwidth_allocation) {} +bool VideoEncoder::RateControlParameters::operator==( + const VideoEncoder::RateControlParameters& rhs) const { + return std::tie(bitrate, framerate_fps, bandwidth_allocation) == + std::tie(rhs.bitrate, rhs.framerate_fps, rhs.bandwidth_allocation); +} + +bool VideoEncoder::RateControlParameters::operator!=( + const VideoEncoder::RateControlParameters& rhs) const { + return !(rhs == *this); +} + VideoEncoder::RateControlParameters::~RateControlParameters() = default; void VideoEncoder::SetFecControllerOverride( diff --git a/api/video_codecs/video_encoder.h b/api/video_codecs/video_encoder.h index 0ee5521b95..766ea75712 100644 --- a/api/video_codecs/video_encoder.h +++ b/api/video_codecs/video_encoder.h @@ -239,6 +239,9 @@ class RTC_EXPORT VideoEncoder { // |bitrate.get_sum_bps()|, but may be higher if the application is not // network constrained. DataRate bandwidth_allocation; + + bool operator==(const RateControlParameters& rhs) const; + bool operator!=(const RateControlParameters& rhs) const; }; struct LossNotification { diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 8b576e8c75..9257f93ad3 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -441,7 +441,7 @@ class VideoStreamEncoder::VideoSourceProxy { }; VideoStreamEncoder::EncoderRateSettings::EncoderRateSettings() - : VideoEncoder::RateControlParameters(), + : rate_control(), encoder_target(DataRate::Zero()), stable_encoder_target(DataRate::Zero()) {} @@ -451,16 +451,13 @@ VideoStreamEncoder::EncoderRateSettings::EncoderRateSettings( DataRate bandwidth_allocation, DataRate encoder_target, DataRate stable_encoder_target) - : VideoEncoder::RateControlParameters(bitrate, - framerate_fps, - bandwidth_allocation), + : rate_control(bitrate, framerate_fps, bandwidth_allocation), encoder_target(encoder_target), stable_encoder_target(stable_encoder_target) {} bool VideoStreamEncoder::EncoderRateSettings::operator==( const EncoderRateSettings& rhs) const { - return bitrate == rhs.bitrate && framerate_fps == rhs.framerate_fps && - bandwidth_allocation == rhs.bandwidth_allocation && + return rate_control == rhs.rate_control && encoder_target == rhs.encoder_target && stable_encoder_target == rhs.stable_encoder_target; } @@ -948,7 +945,8 @@ 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(); + last_encoder_rate_settings_->rate_control.framerate_fps = + GetInputFramerateFps(); SetEncoderRates( UpdateBitrateAllocationAndNotifyObserver(*last_encoder_rate_settings_)); } @@ -1149,7 +1147,7 @@ VideoStreamEncoder::UpdateBitrateAllocationAndNotifyObserver( if (rate_allocator_ && rate_settings.encoder_target > DataRate::Zero()) { new_allocation = rate_allocator_->Allocate(VideoBitrateAllocationParameters( rate_settings.encoder_target, rate_settings.stable_encoder_target, - rate_settings.framerate_fps)); + rate_settings.rate_control.framerate_fps)); } if (bitrate_observer_ && new_allocation.get_sum_bps() > 0) { @@ -1170,27 +1168,27 @@ VideoStreamEncoder::UpdateBitrateAllocationAndNotifyObserver( } EncoderRateSettings new_rate_settings = rate_settings; - new_rate_settings.bitrate = new_allocation; + new_rate_settings.rate_control.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())); + new_rate_settings.rate_control.bandwidth_allocation = std::max( + new_rate_settings.rate_control.bandwidth_allocation, + DataRate::bps(new_rate_settings.rate_control.bitrate.get_sum_bps())); if (bitrate_adjuster_) { VideoBitrateAllocation adjusted_allocation = - bitrate_adjuster_->AdjustRateAllocation(new_rate_settings); + bitrate_adjuster_->AdjustRateAllocation(new_rate_settings.rate_control); RTC_LOG(LS_VERBOSE) << "Adjusting allocation, fps = " - << rate_settings.framerate_fps << ", from " + << rate_settings.rate_control.framerate_fps << ", from " << new_allocation.ToString() << ", to " << adjusted_allocation.ToString(); - new_rate_settings.bitrate = adjusted_allocation; + new_rate_settings.rate_control.bitrate = adjusted_allocation; } encoder_stats_observer_->OnBitrateAllocationUpdated( - send_codec_, new_rate_settings.bitrate); + send_codec_, new_rate_settings.rate_control.bitrate); return new_rate_settings; } @@ -1207,10 +1205,11 @@ uint32_t VideoStreamEncoder::GetInputFramerateFps() { void VideoStreamEncoder::SetEncoderRates( const EncoderRateSettings& rate_settings) { - RTC_DCHECK_GT(rate_settings.framerate_fps, 0.0); - const bool settings_changes = !last_encoder_rate_settings_ || - rate_settings != *last_encoder_rate_settings_; - if (settings_changes) { + RTC_DCHECK_GT(rate_settings.rate_control.framerate_fps, 0.0); + bool rate_control_changed = + (!last_encoder_rate_settings_.has_value() || + last_encoder_rate_settings_->rate_control != rate_settings.rate_control); + if (last_encoder_rate_settings_ != rate_settings) { last_encoder_rate_settings_ = rate_settings; } @@ -1226,15 +1225,16 @@ void VideoStreamEncoder::SetEncoderRates( // bitrate. // TODO(perkj): Make sure all known encoder implementations handle zero // target bitrate and remove this check. - if (!HasInternalSource() && rate_settings.bitrate.get_sum_bps() == 0) { + if (!HasInternalSource() && + rate_settings.rate_control.bitrate.get_sum_bps() == 0) { return; } - if (settings_changes) { - encoder_->SetRates(rate_settings); + if (rate_control_changed) { + encoder_->SetRates(rate_settings.rate_control); frame_encode_metadata_writer_.OnSetRates( - rate_settings.bitrate, - static_cast(rate_settings.framerate_fps + 0.5)); + rate_settings.rate_control.bitrate, + static_cast(rate_settings.rate_control.framerate_fps + 0.5)); } } @@ -1283,7 +1283,8 @@ void VideoStreamEncoder::MaybeEncodeVideoFrame(const VideoFrame& video_frame, // |last_encoder_rate_setings_|, triggering the call to SetRate() on the // encoder. EncoderRateSettings new_rate_settings = *last_encoder_rate_settings_; - new_rate_settings.framerate_fps = static_cast(framerate_fps); + new_rate_settings.rate_control.framerate_fps = + static_cast(framerate_fps); SetEncoderRates( UpdateBitrateAllocationAndNotifyObserver(new_rate_settings)); } diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index 4db92f5fa6..46df362ebd 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -121,7 +121,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, int pixel_count() const { return width * height; } }; - struct EncoderRateSettings : public VideoEncoder::RateControlParameters { + struct EncoderRateSettings { EncoderRateSettings(); EncoderRateSettings(const VideoBitrateAllocation& bitrate, double framerate_fps, @@ -131,6 +131,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, bool operator==(const EncoderRateSettings& rhs) const; bool operator!=(const EncoderRateSettings& rhs) const; + VideoEncoder::RateControlParameters rate_control; // This is the scalar target bitrate before the VideoBitrateAllocator, i.e. // the |target_bitrate| argument of the OnBitrateUpdated() method. This is // needed because the bitrate allocator may truncate the total bitrate and a diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 6f19edcbb1..6eba930405 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -782,6 +782,11 @@ class VideoStreamEncoderTest : public ::testing::Test { return num_encoder_initializations_; } + int GetNumSetRates() const { + rtc::CritScope lock(&local_crit_sect_); + return num_set_rates_; + } + private: int32_t Encode(const VideoFrame& input_image, const std::vector* frame_types) override { @@ -848,6 +853,7 @@ class VideoStreamEncoderTest : public ::testing::Test { void SetRates(const RateControlParameters& parameters) { rtc::CritScope lock(&local_crit_sect_); + num_set_rates_++; VideoBitrateAllocation adjusted_rate_allocation; for (size_t si = 0; si < kMaxSpatialLayers; ++si) { for (size_t ti = 0; ti < kMaxTemporalStreams; ++ti) { @@ -901,6 +907,7 @@ class VideoStreamEncoderTest : public ::testing::Test { int num_encoder_initializations_ RTC_GUARDED_BY(local_crit_sect_) = 0; std::vector resolution_bitrate_limits_ RTC_GUARDED_BY(local_crit_sect_); + int num_set_rates_ RTC_GUARDED_BY(local_crit_sect_) = 0; }; class TestSink : public VideoStreamEncoder::EncoderSink { @@ -4875,4 +4882,75 @@ TEST_F(VideoStreamEncoderTest, ResolutionEncoderSwitch) { video_stream_encoder_->Stop(); } +TEST_F(VideoStreamEncoderTest, + AllocationPropegratedToEncoderWhenTargetRateChanged) { + const int kFrameWidth = 320; + const int kFrameHeight = 180; + + // Set initial rate. + auto rate = DataRate::kbps(100); + video_stream_encoder_->OnBitrateUpdated( + /*target_bitrate=*/rate, + /*stable_target_bitrate=*/rate, + /*link_allocation=*/rate, + /*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); + EXPECT_EQ(1, fake_encoder_.GetNumSetRates()); + + // Change of target bitrate propagates to the encoder. + auto new_stable_rate = rate - DataRate::kbps(5); + video_stream_encoder_->OnBitrateUpdated( + /*target_bitrate=*/new_stable_rate, + /*stable_target_bitrate=*/new_stable_rate, + /*link_allocation=*/rate, + /*fraction_lost=*/0, + /*rtt_ms=*/0); + video_stream_encoder_->WaitUntilTaskQueueIsIdle(); + EXPECT_EQ(2, fake_encoder_.GetNumSetRates()); + video_stream_encoder_->Stop(); +} + +TEST_F(VideoStreamEncoderTest, + AllocationNotPropegratedToEncoderWhenTargetRateUnchanged) { + const int kFrameWidth = 320; + const int kFrameHeight = 180; + + // Set initial rate. + auto rate = DataRate::kbps(100); + video_stream_encoder_->OnBitrateUpdated( + /*target_bitrate=*/rate, + /*stable_target_bitrate=*/rate, + /*link_allocation=*/rate, + /*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); + EXPECT_EQ(1, fake_encoder_.GetNumSetRates()); + + // Set a higher target rate without changing the link_allocation. Should not + // reset encoder's rate. + auto new_stable_rate = rate - DataRate::kbps(5); + video_stream_encoder_->OnBitrateUpdated( + /*target_bitrate=*/rate, + /*stable_target_bitrate=*/new_stable_rate, + /*link_allocation=*/rate, + /*fraction_lost=*/0, + /*rtt_ms=*/0); + video_stream_encoder_->WaitUntilTaskQueueIsIdle(); + EXPECT_EQ(1, fake_encoder_.GetNumSetRates()); + video_stream_encoder_->Stop(); +} + } // namespace webrtc