From b6a45dda4cddb612d8e89b67ea1cd630da2f1bbf Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Wed, 18 Sep 2019 13:46:06 +0000 Subject: [PATCH] Revert "Fix minor regression caused by a8336d3" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 809198edfff416fce8d75b574a43afab5e67b1cd. Reason for revert: Performance regressions that need to be addressed. Original change's description: > Fix minor regression caused by a8336d3 > > 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} TBR=sprang@webrtc.org,eshr@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:10126 Change-Id: I133cbe5d8cb894ed944ae8a2d0f63a78bbed72ee Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/153484 Commit-Queue: Erik Språng Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#29221} --- 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, 27 insertions(+), 121 deletions(-) diff --git a/api/video_codecs/video_encoder.cc b/api/video_codecs/video_encoder.cc index 3a848f39ed..d3f16a0053 100644 --- a/api/video_codecs/video_encoder.cc +++ b/api/video_codecs/video_encoder.cc @@ -116,17 +116,6 @@ 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 766ea75712..0ee5521b95 100644 --- a/api/video_codecs/video_encoder.h +++ b/api/video_codecs/video_encoder.h @@ -239,9 +239,6 @@ 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 e1ca55722d..200b4293ef 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -441,7 +441,7 @@ class VideoStreamEncoder::VideoSourceProxy { }; VideoStreamEncoder::EncoderRateSettings::EncoderRateSettings() - : rate_control(), + : VideoEncoder::RateControlParameters(), encoder_target(DataRate::Zero()), stable_encoder_target(DataRate::Zero()) {} @@ -451,13 +451,16 @@ VideoStreamEncoder::EncoderRateSettings::EncoderRateSettings( DataRate bandwidth_allocation, DataRate encoder_target, DataRate stable_encoder_target) - : rate_control(bitrate, framerate_fps, bandwidth_allocation), + : VideoEncoder::RateControlParameters(bitrate, + framerate_fps, + bandwidth_allocation), encoder_target(encoder_target), stable_encoder_target(stable_encoder_target) {} bool VideoStreamEncoder::EncoderRateSettings::operator==( const EncoderRateSettings& rhs) const { - return rate_control == rhs.rate_control && + return bitrate == rhs.bitrate && framerate_fps == rhs.framerate_fps && + bandwidth_allocation == rhs.bandwidth_allocation && encoder_target == rhs.encoder_target && stable_encoder_target == rhs.stable_encoder_target; } @@ -931,8 +934,7 @@ 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_->rate_control.framerate_fps = - GetInputFramerateFps(); + last_encoder_rate_settings_->framerate_fps = GetInputFramerateFps(); SetEncoderRates( UpdateBitrateAllocationAndNotifyObserver(*last_encoder_rate_settings_)); } @@ -1133,7 +1135,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.rate_control.framerate_fps)); + rate_settings.framerate_fps)); } if (bitrate_observer_ && new_allocation.get_sum_bps() > 0) { @@ -1154,27 +1156,27 @@ VideoStreamEncoder::UpdateBitrateAllocationAndNotifyObserver( } EncoderRateSettings new_rate_settings = rate_settings; - new_rate_settings.rate_control.bitrate = new_allocation; + 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.rate_control.bandwidth_allocation = std::max( - new_rate_settings.rate_control.bandwidth_allocation, - DataRate::bps(new_rate_settings.rate_control.bitrate.get_sum_bps())); + 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 = - bitrate_adjuster_->AdjustRateAllocation(new_rate_settings.rate_control); + bitrate_adjuster_->AdjustRateAllocation(new_rate_settings); RTC_LOG(LS_VERBOSE) << "Adjusting allocation, fps = " - << rate_settings.rate_control.framerate_fps << ", from " + << rate_settings.framerate_fps << ", from " << new_allocation.ToString() << ", to " << adjusted_allocation.ToString(); - new_rate_settings.rate_control.bitrate = adjusted_allocation; + new_rate_settings.bitrate = adjusted_allocation; } encoder_stats_observer_->OnBitrateAllocationUpdated( - send_codec_, new_rate_settings.rate_control.bitrate); + send_codec_, new_rate_settings.bitrate); return new_rate_settings; } @@ -1191,11 +1193,10 @@ uint32_t VideoStreamEncoder::GetInputFramerateFps() { void VideoStreamEncoder::SetEncoderRates( const EncoderRateSettings& rate_settings) { - 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) { + 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) { last_encoder_rate_settings_ = rate_settings; } @@ -1211,16 +1212,15 @@ void VideoStreamEncoder::SetEncoderRates( // bitrate. // TODO(perkj): Make sure all known encoder implementations handle zero // target bitrate and remove this check. - if (!HasInternalSource() && - rate_settings.rate_control.bitrate.get_sum_bps() == 0) { + if (!HasInternalSource() && rate_settings.bitrate.get_sum_bps() == 0) { return; } - if (rate_control_changed) { - encoder_->SetRates(rate_settings.rate_control); + if (settings_changes) { + encoder_->SetRates(rate_settings); frame_encode_metadata_writer_.OnSetRates( - rate_settings.rate_control.bitrate, - static_cast(rate_settings.rate_control.framerate_fps + 0.5)); + rate_settings.bitrate, + static_cast(rate_settings.framerate_fps + 0.5)); } } @@ -1269,8 +1269,7 @@ 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.rate_control.framerate_fps = - static_cast(framerate_fps); + new_rate_settings.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 ba9f519475..f2268678d6 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 { + struct EncoderRateSettings : public VideoEncoder::RateControlParameters { EncoderRateSettings(); EncoderRateSettings(const VideoBitrateAllocation& bitrate, double framerate_fps, @@ -131,7 +131,6 @@ 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 6eba930405..6f19edcbb1 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -782,11 +782,6 @@ 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 { @@ -853,7 +848,6 @@ 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) { @@ -907,7 +901,6 @@ 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 { @@ -4882,75 +4875,4 @@ 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