From d96b275cd686f0fad637cc9233ba8c34084077eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 13 Dec 2018 11:23:27 +0100 Subject: [PATCH] Refactor EncodeParameters usage, remove unused rtt/loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:10126 Change-Id: Ib93f5e65b25540576c026197f72a5902cf43fc16 Reviewed-on: https://webrtc-review.googlesource.com/c/114281 Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Erik SprÃ¥ng Cr-Commit-Position: refs/heads/master@{#26001} --- modules/video_coding/BUILD.gn | 1 + modules/video_coding/generic_encoder.cc | 1 - modules/video_coding/generic_encoder.h | 18 +++++++-- modules/video_coding/video_coding_impl.cc | 4 +- modules/video_coding/video_coding_impl.h | 6 ++- modules/video_coding/video_sender.cc | 40 ++++++++++--------- modules/video_coding/video_sender_unittest.cc | 20 +++++----- video/video_stream_encoder.cc | 3 +- 8 files changed, 54 insertions(+), 39 deletions(-) diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index 2ec46e600a..196880dcbd 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -173,6 +173,7 @@ rtc_static_library("video_coding") { "..:module_api_public", "../..:webrtc_common", "../../api:fec_controller_api", + "../../api/units:data_rate", "../../api/video:builtin_video_bitrate_allocator_factory", "../../api/video:encoded_frame", "../../api/video:video_bitrate_allocator", diff --git a/modules/video_coding/generic_encoder.cc b/modules/video_coding/generic_encoder.cc index 807ccbec51..1f24159e74 100644 --- a/modules/video_coding/generic_encoder.cc +++ b/modules/video_coding/generic_encoder.cc @@ -46,7 +46,6 @@ VCMGenericEncoder::VCMGenericEncoder( : encoder_(encoder), vcm_encoded_frame_callback_(encoded_frame_callback), internal_source_(internal_source), - encoder_params_({VideoBitrateAllocation(), 0, 0, 0}), streams_or_svc_num_(0), codec_type_(VideoCodecType::kVideoCodecGeneric) {} diff --git a/modules/video_coding/generic_encoder.h b/modules/video_coding/generic_encoder.h index 2f841b34dd..1f4ade201e 100644 --- a/modules/video_coding/generic_encoder.h +++ b/modules/video_coding/generic_encoder.h @@ -15,9 +15,9 @@ #include #include +#include "api/units/data_rate.h" #include "modules/video_coding/include/video_codec_interface.h" #include "modules/video_coding/include/video_coding_defines.h" - #include "rtc_base/criticalsection.h" #include "rtc_base/race_checker.h" @@ -28,9 +28,21 @@ class MediaOptimization; } // namespace media_optimization struct EncoderParameters { + EncoderParameters() : total_bitrate(DataRate::Zero()), input_frame_rate(0) {} + EncoderParameters(DataRate total_bitrate, + const VideoBitrateAllocation& allocation, + uint32_t framerate) + : total_bitrate(total_bitrate), + target_bitrate(allocation), + input_frame_rate(framerate) {} + + // Total bitrate allocated for this encoder. + DataRate total_bitrate; + // The bitrate allocation, across spatial and/or temporal layers. Note that + // the sum of these might be less than |total_bitrate| if the allocator has + // capped the bitrate for some configuration. VideoBitrateAllocation target_bitrate; - uint8_t loss_rate; - int64_t rtt; + // The input frame rate to the encoder in fps, measured in the video sender. uint32_t input_frame_rate; }; diff --git a/modules/video_coding/video_coding_impl.cc b/modules/video_coding/video_coding_impl.cc index 39eccb56da..dffebd707d 100644 --- a/modules/video_coding/video_coding_impl.cc +++ b/modules/video_coding/video_coding_impl.cc @@ -121,8 +121,8 @@ class VideoCodingModuleImpl : public VideoCodingModule { int32_t SetChannelParameters(uint32_t target_bitrate, // bits/s. uint8_t lossRate, int64_t rtt) override { - return sender_.SetChannelParameters(target_bitrate, lossRate, rtt, - rate_allocator_.get(), nullptr); + return sender_.SetChannelParameters(target_bitrate, rate_allocator_.get(), + nullptr); } int32_t SetVideoProtection(VCMVideoProtection videoProtection, diff --git a/modules/video_coding/video_coding_impl.h b/modules/video_coding/video_coding_impl.h index e27a2fe760..daf21d8b9e 100644 --- a/modules/video_coding/video_coding_impl.h +++ b/modules/video_coding/video_coding_impl.h @@ -79,8 +79,6 @@ class VideoSender { // cause an immediate call to VideoEncoder::SetRateAllocation(). int32_t SetChannelParameters( uint32_t target_bitrate_bps, - uint8_t loss_rate, - int64_t rtt, VideoBitrateAllocator* bitrate_allocator, VideoBitrateAllocationObserver* bitrate_updated_callback); @@ -107,6 +105,10 @@ class VideoSender { uint32_t target_bitrate_bps); void SetEncoderParameters(EncoderParameters params, bool has_internal_source) RTC_EXCLUSIVE_LOCKS_REQUIRED(encoder_crit_); + VideoBitrateAllocation GetAllocation( + uint32_t bitrate_bps, + uint32_t framerate_fps, + VideoBitrateAllocator* bitrate_allocator) const; rtc::CriticalSection encoder_crit_; VCMGenericEncoder* _encoder; diff --git a/modules/video_coding/video_sender.cc b/modules/video_coding/video_sender.cc index f8e21066e9..da59fe99e8 100644 --- a/modules/video_coding/video_sender.cc +++ b/modules/video_coding/video_sender.cc @@ -55,7 +55,6 @@ VideoSender::VideoSender(Clock* clock, frame_dropper_requested_(true), force_disable_frame_dropper_(false), current_codec_(), - encoder_params_({VideoBitrateAllocation(), 0, 0, 0}), encoder_has_internal_source_(false), next_frame_types_(1, kVideoFrameDelta) { // Allow VideoSender to be created on one thread but used on another, post @@ -167,22 +166,31 @@ EncoderParameters VideoSender::UpdateEncoderParameters( if (input_frame_rate == 0) input_frame_rate = current_codec_.maxFramerate; + EncoderParameters new_encoder_params = { + DataRate::bps(target_bitrate_bps), + GetAllocation(video_target_rate_bps, input_frame_rate, bitrate_allocator), + input_frame_rate}; + return new_encoder_params; +} + +VideoBitrateAllocation VideoSender::GetAllocation( + uint32_t bitrate_bps, + uint32_t framerate_fps, + VideoBitrateAllocator* bitrate_allocator) const { VideoBitrateAllocation bitrate_allocation; // Only call allocators if bitrate > 0 (ie, not suspended), otherwise they // might cap the bitrate to the min bitrate configured. - if (target_bitrate_bps > 0) { + if (bitrate_bps > 0) { if (bitrate_allocator) { - bitrate_allocation = bitrate_allocator->GetAllocation( - video_target_rate_bps, input_frame_rate); + bitrate_allocation = + bitrate_allocator->GetAllocation(bitrate_bps, framerate_fps); } else { DefaultVideoBitrateAllocator default_allocator(current_codec_); - bitrate_allocation = default_allocator.GetAllocation( - video_target_rate_bps, input_frame_rate); + bitrate_allocation = + default_allocator.GetAllocation(bitrate_bps, framerate_fps); } } - EncoderParameters new_encoder_params = {bitrate_allocation, params.loss_rate, - params.rtt, input_frame_rate}; - return new_encoder_params; + return bitrate_allocation; } void VideoSender::UpdateChannelParameters( @@ -193,22 +201,19 @@ void VideoSender::UpdateChannelParameters( rtc::CritScope cs(¶ms_crit_); encoder_params_ = UpdateEncoderParameters(encoder_params_, bitrate_allocator, - encoder_params_.target_bitrate.get_sum_bps()); + encoder_params_.total_bitrate.bps()); target_rate = encoder_params_.target_bitrate; } - if (bitrate_updated_callback && target_rate.get_sum_bps() > 0) + if (bitrate_updated_callback && target_rate.get_sum_bps() > 0) { bitrate_updated_callback->OnBitrateAllocationUpdated(target_rate); + } } int32_t VideoSender::SetChannelParameters( uint32_t target_bitrate_bps, - uint8_t loss_rate, - int64_t rtt, VideoBitrateAllocator* bitrate_allocator, VideoBitrateAllocationObserver* bitrate_updated_callback) { EncoderParameters encoder_params; - encoder_params.loss_rate = loss_rate; - encoder_params.rtt = rtt; encoder_params = UpdateEncoderParameters(encoder_params, bitrate_allocator, target_bitrate_bps); if (bitrate_updated_callback && target_bitrate_bps > 0) { @@ -286,11 +291,10 @@ int32_t VideoSender::AddVideoFrame( _mediaOpt.EnableFrameDropper(frame_dropping_enabled); if (_mediaOpt.DropFrame()) { - RTC_LOG(LS_VERBOSE) << "Drop Frame " + RTC_LOG(LS_VERBOSE) << "Drop Frame: " << "target bitrate " << encoder_params.target_bitrate.get_sum_bps() - << " loss rate " << encoder_params.loss_rate << " rtt " - << encoder_params.rtt << " input frame rate " + << ", input frame rate " << encoder_params.input_frame_rate; post_encode_callback_->OnDroppedFrame( EncodedImageCallback::DropReason::kDroppedByMediaOptimizations); diff --git a/modules/video_coding/video_sender_unittest.cc b/modules/video_coding/video_sender_unittest.cc index c2ff0f6207..8e727df8f0 100644 --- a/modules/video_coding/video_sender_unittest.cc +++ b/modules/video_coding/video_sender_unittest.cc @@ -314,14 +314,14 @@ TEST_F(TestVideoSenderWithMockEncoder, TestSetRate) { SetRateAllocation(new_rate_allocation, settings_.maxFramerate)) .Times(1) .WillOnce(Return(0)); - sender_->SetChannelParameters(new_bitrate_kbps * 1000, 0, 200, - rate_allocator_.get(), nullptr); + sender_->SetChannelParameters(new_bitrate_kbps * 1000, rate_allocator_.get(), + nullptr); AddFrame(); clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); // Expect no call to encoder_.SetRates if the new bitrate is zero. EXPECT_CALL(encoder_, SetRateAllocation(_, _)).Times(0); - sender_->SetChannelParameters(0, 0, 200, rate_allocator_.get(), nullptr); + sender_->SetChannelParameters(0, rate_allocator_.get(), nullptr); AddFrame(); } @@ -358,21 +358,19 @@ TEST_F(TestVideoSenderWithMockEncoder, TestEncoderParametersForInternalSource) { EXPECT_CALL(encoder_, SetRateAllocation(new_rate_allocation, _)) .Times(1) .WillOnce(Return(0)); - sender_->SetChannelParameters(new_bitrate_kbps * 1000, 0, 200, - rate_allocator_.get(), nullptr); + sender_->SetChannelParameters(new_bitrate_kbps * 1000, rate_allocator_.get(), + nullptr); } TEST_F(TestVideoSenderWithMockEncoder, NoRedundantSetChannelParameterOrSetRatesCalls) { - const uint8_t kLossRate = 4; - const uint8_t kRtt = 200; const int64_t kRateStatsWindowMs = 2000; const uint32_t kInputFps = 20; int64_t start_time = clock_.TimeInMilliseconds(); // Expect initial call to SetChannelParameters. Rates are initialized through // InitEncode and expects no additional call before the framerate (or bitrate) // updates. - sender_->SetChannelParameters(settings_.startBitrate * 1000, kLossRate, kRtt, + sender_->SetChannelParameters(settings_.startBitrate * 1000, rate_allocator_.get(), nullptr); while (clock_.TimeInMilliseconds() < start_time + kRateStatsWindowMs) { AddFrame(); @@ -387,8 +385,8 @@ TEST_F(TestVideoSenderWithMockEncoder, EXPECT_CALL(encoder_, SetRateAllocation(new_rate_allocation, kInputFps)) .Times(1) .WillOnce(Return(0)); - sender_->SetChannelParameters(new_bitrate_bps, kLossRate, kRtt, - rate_allocator_.get(), nullptr); + sender_->SetChannelParameters(new_bitrate_bps, rate_allocator_.get(), + nullptr); AddFrame(); } @@ -440,7 +438,7 @@ class TestVideoSenderWithVp8 : public TestVideoSender { // buffer since it will fail to calculate the framerate. if (i != 0) { EXPECT_EQ(VCM_OK, sender_->SetChannelParameters( - available_bitrate_kbps_ * 1000, 0, 200, + available_bitrate_kbps_ * 1000, rate_allocator_.get(), nullptr)); } } diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 8cabc032b9..235e72bda1 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -1003,8 +1003,7 @@ void VideoStreamEncoder::OnBitrateUpdated(uint32_t bitrate_bps, has_seen_first_significant_bwe_change_ = true; } - video_sender_.SetChannelParameters(bitrate_bps, fraction_lost, - round_trip_time_ms, rate_allocator_.get(), + video_sender_.SetChannelParameters(bitrate_bps, rate_allocator_.get(), bitrate_observer_); encoder_start_bitrate_bps_ =