From 69ccb331310fff13a8b061c294d82b20f2b82bc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Thu, 29 Oct 2015 16:30:23 +0100 Subject: [PATCH] Remove redudant encoder rate calls. Moves EncoderParameters update checks into GenericEncoder before calling SetRates/SetChannelParameters as applicable. Also removes CodecConfigParameters as a bonus. BUG= R=stefan@webrtc.org TBR=mflodman@webrtc.org Review URL: https://codereview.webrtc.org/1426953003 . Cr-Commit-Position: refs/heads/master@{#10452} --- .../video_coding/codecs/i420/include/i420.h | 4 - .../mock/mock_video_codec_interface.h | 4 - .../main/interface/video_coding.h | 10 --- .../main/source/generic_encoder.cc | 87 +++++++------------ .../main/source/generic_encoder.h | 37 +++----- .../main/source/video_coding_impl.cc | 4 - .../main/source/video_coding_impl.h | 9 -- .../video_coding/main/source/video_sender.cc | 46 +++------- .../main/source/video_sender_unittest.cc | 35 +++++++- .../test/configurable_frame_size_encoder.cc | 5 -- webrtc/test/configurable_frame_size_encoder.h | 2 - webrtc/video_encoder.h | 3 - webrtc/video_engine/vie_encoder.cc | 13 --- webrtc/video_engine/vie_encoder.h | 4 - 14 files changed, 92 insertions(+), 171 deletions(-) diff --git a/webrtc/modules/video_coding/codecs/i420/include/i420.h b/webrtc/modules/video_coding/codecs/i420/include/i420.h index e54e78db57..8990ccf878 100644 --- a/webrtc/modules/video_coding/codecs/i420/include/i420.h +++ b/webrtc/modules/video_coding/codecs/i420/include/i420.h @@ -73,10 +73,6 @@ class I420Encoder : public VideoEncoder { return WEBRTC_VIDEO_CODEC_OK; } - int CodecConfigParameters(uint8_t* /*buffer*/, int /*size*/) override { - return WEBRTC_VIDEO_CODEC_OK; - } - void OnDroppedFrame() override {} private: diff --git a/webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h b/webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h index 5243d9afb8..6c926d4794 100644 --- a/webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h +++ b/webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h @@ -43,8 +43,6 @@ class MockVideoEncoder : public VideoEncoder { MOCK_METHOD2(SetChannelParameters, int32_t(uint32_t packetLoss, int64_t rtt)); MOCK_METHOD2(SetRates, int32_t(uint32_t newBitRate, uint32_t frameRate)); MOCK_METHOD1(SetPeriodicKeyFrames, int32_t(bool enable)); - MOCK_METHOD2(CodecConfigParameters, - int32_t(uint8_t* /*buffer*/, int32_t)); }; class MockDecodedImageCallback : public DecodedImageCallback { @@ -69,8 +67,6 @@ class MockVideoDecoder : public VideoDecoder { int32_t(DecodedImageCallback* callback)); MOCK_METHOD0(Release, int32_t()); MOCK_METHOD0(Reset, int32_t()); - MOCK_METHOD2(SetCodecConfigParameters, - int32_t(const uint8_t* /*buffer*/, int32_t)); MOCK_METHOD0(Copy, VideoDecoder*()); }; diff --git a/webrtc/modules/video_coding/main/interface/video_coding.h b/webrtc/modules/video_coding/main/interface/video_coding.h index d2dd05057a..67f7b635cb 100644 --- a/webrtc/modules/video_coding/main/interface/video_coding.h +++ b/webrtc/modules/video_coding/main/interface/video_coding.h @@ -185,16 +185,6 @@ public: uint8_t payloadType, bool internalSource = false) = 0; - // API to get codec config parameters to be sent out-of-band to a receiver. - // - // Input: - // - buffer : Memory where the codec config parameters should be written. - // - size : Size of the memory available. - // - // Return value : Number of bytes written, on success. - // < 0, on error. - virtual int32_t CodecConfigParameters(uint8_t* buffer, int32_t size) = 0; - // API to get currently configured encoder target bitrate in bits/s. // // Return value : 0, on success. diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.cc b/webrtc/modules/video_coding/main/source/generic_encoder.cc index dbb1c17ff9..e3ae0dd44b 100644 --- a/webrtc/modules/video_coding/main/source/generic_encoder.cc +++ b/webrtc/modules/video_coding/main/source/generic_encoder.cc @@ -94,12 +94,10 @@ VCMGenericEncoder::VCMGenericEncoder(VideoEncoder* encoder, : encoder_(encoder), rate_observer_(rate_observer), vcm_encoded_frame_callback_(nullptr), - bit_rate_(0), - frame_rate_(0), + encoder_params_({0, 0, 0, 0}), internal_source_(internalSource), rotation_(kVideoRotation_0), - is_screenshare_(false) { -} + is_screenshare_(false) {} VCMGenericEncoder::~VCMGenericEncoder() { @@ -108,9 +106,8 @@ VCMGenericEncoder::~VCMGenericEncoder() int32_t VCMGenericEncoder::Release() { { - rtc::CritScope lock(&rates_lock_); - bit_rate_ = 0; - frame_rate_ = 0; + rtc::CritScope lock(¶ms_lock_); + encoder_params_ = {0, 0, 0, 0}; vcm_encoded_frame_callback_ = nullptr; } @@ -123,9 +120,9 @@ VCMGenericEncoder::InitEncode(const VideoCodec* settings, size_t maxPayloadSize) { { - rtc::CritScope lock(&rates_lock_); - bit_rate_ = settings->startBitrate * 1000; - frame_rate_ = settings->maxFramerate; + rtc::CritScope lock(¶ms_lock_); + encoder_params_.target_bitrate = settings->startBitrate * 1000; + encoder_params_.input_frame_rate = settings->maxFramerate; } is_screenshare_ = settings->mode == VideoCodecMode::kScreensharing; @@ -162,54 +159,34 @@ int32_t VCMGenericEncoder::Encode(const VideoFrame& inputFrame, return result; } -int32_t -VCMGenericEncoder::SetChannelParameters(int32_t packetLoss, int64_t rtt) -{ - return encoder_->SetChannelParameters(packetLoss, rtt); -} - -int32_t -VCMGenericEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate) -{ - uint32_t target_bitrate_kbps = (newBitRate + 500) / 1000; - int32_t ret = encoder_->SetRates(target_bitrate_kbps, frameRate); - if (ret < 0) - { - return ret; +void VCMGenericEncoder::SetEncoderParameters(const EncoderParameters& params) { + bool channel_parameters_have_changed; + bool rates_have_changed; + { + rtc::CritScope lock(¶ms_lock_); + channel_parameters_have_changed = + params.loss_rate != encoder_params_.loss_rate || + params.rtt != encoder_params_.rtt; + rates_have_changed = + params.target_bitrate != encoder_params_.target_bitrate || + params.input_frame_rate != encoder_params_.input_frame_rate; + encoder_params_ = params; + } + if (channel_parameters_have_changed) + encoder_->SetChannelParameters(params.loss_rate, params.rtt); + if (rates_have_changed) { + uint32_t target_bitrate_kbps = (params.target_bitrate + 500) / 1000; + encoder_->SetRates(target_bitrate_kbps, params.input_frame_rate); + if (rate_observer_ != nullptr) { + rate_observer_->OnSetRates(params.target_bitrate, + params.input_frame_rate); } - - { - rtc::CritScope lock(&rates_lock_); - bit_rate_ = newBitRate; - frame_rate_ = frameRate; - } - - if (rate_observer_ != nullptr) - rate_observer_->OnSetRates(newBitRate, frameRate); - return VCM_OK; + } } -int32_t -VCMGenericEncoder::CodecConfigParameters(uint8_t* buffer, int32_t size) -{ - int32_t ret = encoder_->CodecConfigParameters(buffer, size); - if (ret < 0) - { - return ret; - } - return ret; -} - -uint32_t VCMGenericEncoder::BitRate() const -{ - rtc::CritScope lock(&rates_lock_); - return bit_rate_; -} - -uint32_t VCMGenericEncoder::FrameRate() const -{ - rtc::CritScope lock(&rates_lock_); - return frame_rate_; +EncoderParameters VCMGenericEncoder::GetEncoderParameters() const { + rtc::CritScope lock(¶ms_lock_); + return encoder_params_; } int32_t diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.h b/webrtc/modules/video_coding/main/source/generic_encoder.h index 25235b6b46..5482e507d7 100644 --- a/webrtc/modules/video_coding/main/source/generic_encoder.h +++ b/webrtc/modules/video_coding/main/source/generic_encoder.h @@ -26,6 +26,13 @@ namespace media_optimization { class MediaOptimization; } // namespace media_optimization +struct EncoderParameters { + uint32_t target_bitrate; + uint8_t loss_rate; + int64_t rtt; + uint32_t input_frame_rate; +}; + /*************************************/ /* VCMEncodeFrameCallback class */ /***********************************/ @@ -102,33 +109,16 @@ public: int32_t Encode(const VideoFrame& inputFrame, const CodecSpecificInfo* codecSpecificInfo, const std::vector& frameTypes); - /** - * Set new target bitrate (bits/s) and framerate. - * Return Value: new bit rate if OK, otherwise <0s. - */ - // TODO(tommi): We could replace BitRate and FrameRate below with a GetRates - // method that matches SetRates. For fetching current rates, we'd then only - // grab the lock once instead of twice. - int32_t SetRates(uint32_t target_bitrate, uint32_t frameRate); - /** - * Set a new packet loss rate and a new round-trip time in milliseconds. - */ - int32_t SetChannelParameters(int32_t packetLoss, int64_t rtt); - int32_t CodecConfigParameters(uint8_t* buffer, int32_t size); + + void SetEncoderParameters(const EncoderParameters& params); /** * Register a transport callback which will be called to deliver the encoded * buffers */ int32_t RegisterEncodeCallback( VCMEncodedFrameCallback* VCMencodedFrameCallback); - /** - * Get encoder bit rate - */ - uint32_t BitRate() const; - /** - * Get encoder frame rate - */ - uint32_t FrameRate() const; + + EncoderParameters GetEncoderParameters() const; int32_t SetPeriodicKeyFrames(bool enable); @@ -146,10 +136,9 @@ private: VideoEncoder* const encoder_; VideoEncoderRateObserver* const rate_observer_; VCMEncodedFrameCallback* vcm_encoded_frame_callback_; - uint32_t bit_rate_; - uint32_t frame_rate_; + EncoderParameters encoder_params_ GUARDED_BY(params_lock_); const bool internal_source_; - mutable rtc::CriticalSection rates_lock_; + mutable rtc::CriticalSection params_lock_; VideoRotation rotation_; bool is_screenshare_; }; // end of VCMGenericEncoder class diff --git a/webrtc/modules/video_coding/main/source/video_coding_impl.cc b/webrtc/modules/video_coding/main/source/video_coding_impl.cc index cae00423c0..b0a6754cbd 100644 --- a/webrtc/modules/video_coding/main/source/video_coding_impl.cc +++ b/webrtc/modules/video_coding/main/source/video_coding_impl.cc @@ -133,10 +133,6 @@ class VideoCodingModuleImpl : public VideoCodingModule { externalEncoder, payloadType, internalSource); } - int32_t CodecConfigParameters(uint8_t* buffer, int32_t size) override { - return sender_->CodecConfigParameters(buffer, size); - } - int Bitrate(unsigned int* bitrate) const override { return sender_->Bitrate(bitrate); } diff --git a/webrtc/modules/video_coding/main/source/video_coding_impl.h b/webrtc/modules/video_coding/main/source/video_coding_impl.h index 34900273de..57f38dad13 100644 --- a/webrtc/modules/video_coding/main/source/video_coding_impl.h +++ b/webrtc/modules/video_coding/main/source/video_coding_impl.h @@ -86,7 +86,6 @@ class VideoSender { uint8_t payloadType, bool internalSource); - int32_t CodecConfigParameters(uint8_t* buffer, int32_t size) const; int Bitrate(unsigned int* bitrate) const; int FrameRate(unsigned int* framerate) const; @@ -113,14 +112,6 @@ class VideoSender { int32_t Process(); private: - struct EncoderParameters { - uint32_t target_bitrate; - uint8_t loss_rate; - int64_t rtt; - uint32_t input_frame_rate; - bool updated; - }; - void SetEncoderParameters(EncoderParameters params) EXCLUSIVE_LOCKS_REQUIRED(send_crit_); diff --git a/webrtc/modules/video_coding/main/source/video_sender.cc b/webrtc/modules/video_coding/main/source/video_sender.cc index cdbaffd75f..38089f7113 100644 --- a/webrtc/modules/video_coding/main/source/video_sender.cc +++ b/webrtc/modules/video_coding/main/source/video_sender.cc @@ -40,8 +40,8 @@ VideoSender::VideoSender(Clock* clock, _sendStatsTimer(1000, clock_), current_codec_(), qm_settings_callback_(qm_settings_callback), - protection_callback_(nullptr) { - encoder_params_ = {0, 0, 0, 0, false}; + protection_callback_(nullptr), + encoder_params_({0, 0, 0, 0}) { // Allow VideoSender to be created on one thread but used on another, post // construction. This is currently how this class is being used by at least // one external project (diffractor). @@ -70,7 +70,6 @@ int32_t VideoSender::Process() { // Force an encoder parameters update, so that incoming frame rate is // updated even if bandwidth hasn't changed. encoder_params_.input_frame_rate = _mediaOpt.InputFrameRate(); - encoder_params_.updated = true; } return returnValue; @@ -180,27 +179,15 @@ int32_t VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder, return 0; } -// Get codec config parameters -int32_t VideoSender::CodecConfigParameters(uint8_t* buffer, - int32_t size) const { - rtc::CritScope lock(&send_crit_); - if (_encoder != nullptr) { - return _encoder->CodecConfigParameters(buffer, size); - } - return VCM_UNINITIALIZED; -} - // Get encode bitrate int VideoSender::Bitrate(unsigned int* bitrate) const { RTC_DCHECK(main_thread_.CalledOnValidThread()); // Since we're running on the thread that's the only thread known to modify // the value of _encoder, we don't need to grab the lock here. - // return the bit rate which the encoder is set to - if (!_encoder) { + if (!_encoder) return VCM_UNINITIALIZED; - } - *bitrate = _encoder->BitRate(); + *bitrate = _encoder->GetEncoderParameters().target_bitrate; return 0; } @@ -210,11 +197,10 @@ int VideoSender::FrameRate(unsigned int* framerate) const { // Since we're running on the thread that's the only thread known to modify // the value of _encoder, we don't need to grab the lock here. - // input frame rate, not compensated - if (!_encoder) { + if (!_encoder) return VCM_UNINITIALIZED; - } - *framerate = _encoder->FrameRate(); + + *framerate = _encoder->GetEncoderParameters().input_frame_rate; return 0; } @@ -228,25 +214,21 @@ int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate, uint32_t input_frame_rate = _mediaOpt.InputFrameRate(); rtc::CritScope cs(¶ms_lock_); - encoder_params_ = - EncoderParameters{target_rate, lossRate, rtt, input_frame_rate, true}; + encoder_params_ = {target_rate, lossRate, rtt, input_frame_rate}; return VCM_OK; } void VideoSender::SetEncoderParameters(EncoderParameters params) { - if (!params.updated || params.target_bitrate == 0) + if (params.target_bitrate == 0) return; if (params.input_frame_rate == 0) { // No frame rate estimate available, use default. params.input_frame_rate = current_codec_.maxFramerate; } - if (_encoder != nullptr) { - _encoder->SetChannelParameters(params.loss_rate, params.rtt); - _encoder->SetRates(params.target_bitrate, params.input_frame_rate); - } - return; + if (_encoder != nullptr) + _encoder->SetEncoderParameters(params); } int32_t VideoSender::RegisterTransportCallback( @@ -304,13 +286,11 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame, { rtc::CritScope lock(¶ms_lock_); encoder_params = encoder_params_; - encoder_params_.updated = false; } rtc::CritScope lock(&send_crit_); - SetEncoderParameters(encoder_params); - if (_encoder == nullptr) { + if (_encoder == nullptr) return VCM_UNINITIALIZED; - } + SetEncoderParameters(encoder_params); // TODO(holmer): Add support for dropping frames per stream. Currently we // only have one frame dropper for all streams. if (_nextFrameTypes[0] == kEmptyFrame) { diff --git a/webrtc/modules/video_coding/main/source/video_sender_unittest.cc b/webrtc/modules/video_coding/main/source/video_sender_unittest.cc index b1d645d12d..e9c8bd79b6 100644 --- a/webrtc/modules/video_coding/main/source/video_sender_unittest.cc +++ b/webrtc/modules/video_coding/main/source/video_sender_unittest.cc @@ -318,7 +318,7 @@ TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequestsInternalCapture) { } TEST_F(TestVideoSenderWithMockEncoder, EncoderFramerateUpdatedViaProcess) { - sender_->SetChannelParameters(settings_.startBitrate, 0, 200); + sender_->SetChannelParameters(settings_.startBitrate * 1000, 0, 200); const int64_t kRateStatsWindowMs = 2000; const uint32_t kInputFps = 20; int64_t start_time = clock_.TimeInMilliseconds(); @@ -331,6 +331,39 @@ TEST_F(TestVideoSenderWithMockEncoder, EncoderFramerateUpdatedViaProcess) { AddFrame(); } +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. + EXPECT_CALL(encoder_, SetChannelParameters(kLossRate, kRtt)) + .Times(1) + .WillOnce(Return(0)); + sender_->SetChannelParameters(settings_.startBitrate * 1000, kLossRate, kRtt); + while (clock_.TimeInMilliseconds() < start_time + kRateStatsWindowMs) { + AddFrame(); + clock_.AdvanceTimeMilliseconds(1000 / kInputFps); + } + // After process, input framerate should be updated but not ChannelParameters + // as they are the same as before. + EXPECT_CALL(encoder_, SetRates(_, kInputFps)).Times(1).WillOnce(Return(0)); + sender_->Process(); + AddFrame(); + // Call to SetChannelParameters with changed bitrate should call encoder + // SetRates but not encoder SetChannelParameters (that are unchanged). + EXPECT_CALL(encoder_, SetRates(2 * settings_.startBitrate, kInputFps)) + .Times(1) + .WillOnce(Return(0)); + sender_->SetChannelParameters(2 * settings_.startBitrate * 1000, kLossRate, + kRtt); + AddFrame(); +} + class TestVideoSenderWithVp8 : public TestVideoSender { public: TestVideoSenderWithVp8() diff --git a/webrtc/test/configurable_frame_size_encoder.cc b/webrtc/test/configurable_frame_size_encoder.cc index 3d9cf39f33..2cd47504a5 100644 --- a/webrtc/test/configurable_frame_size_encoder.cc +++ b/webrtc/test/configurable_frame_size_encoder.cc @@ -82,11 +82,6 @@ int32_t ConfigurableFrameSizeEncoder::SetPeriodicKeyFrames(bool enable) { return WEBRTC_VIDEO_CODEC_OK; } -int32_t ConfigurableFrameSizeEncoder::CodecConfigParameters(uint8_t* buffer, - int32_t size) { - return WEBRTC_VIDEO_CODEC_OK; -} - int32_t ConfigurableFrameSizeEncoder::SetFrameSize(size_t size) { assert(size <= max_frame_size_); current_frame_size_ = size; diff --git a/webrtc/test/configurable_frame_size_encoder.h b/webrtc/test/configurable_frame_size_encoder.h index 79514ef6ae..3794e8db08 100644 --- a/webrtc/test/configurable_frame_size_encoder.h +++ b/webrtc/test/configurable_frame_size_encoder.h @@ -43,8 +43,6 @@ class ConfigurableFrameSizeEncoder : public VideoEncoder { int32_t SetPeriodicKeyFrames(bool enable) override; - int32_t CodecConfigParameters(uint8_t* buffer, int32_t size) override; - int32_t SetFrameSize(size_t size); private: diff --git a/webrtc/video_encoder.h b/webrtc/video_encoder.h index 609c07391f..f255336a25 100644 --- a/webrtc/video_encoder.h +++ b/webrtc/video_encoder.h @@ -121,9 +121,6 @@ class VideoEncoder { virtual int32_t SetRates(uint32_t bitrate, uint32_t framerate) = 0; virtual int32_t SetPeriodicKeyFrames(bool enable) { return -1; } - virtual int32_t CodecConfigParameters(uint8_t* /*buffer*/, int32_t /*size*/) { - return -1; - } virtual void OnDroppedFrame() {} virtual int GetTargetFramerate() { return -1; } virtual bool SupportsNativeHandle() const { return false; } diff --git a/webrtc/video_engine/vie_encoder.cc b/webrtc/video_engine/vie_encoder.cc index f705bc26c2..0f4a5a14f5 100644 --- a/webrtc/video_engine/vie_encoder.cc +++ b/webrtc/video_engine/vie_encoder.cc @@ -258,19 +258,6 @@ int32_t ViEEncoder::GetEncoder(VideoCodec* video_codec) { return 0; } -int32_t ViEEncoder::GetCodecConfigParameters( - unsigned char config_parameters[kConfigParameterSize], - unsigned char& config_parameters_size) { - int32_t num_parameters = - vcm_->CodecConfigParameters(config_parameters, kConfigParameterSize); - if (num_parameters <= 0) { - config_parameters_size = 0; - return -1; - } - config_parameters_size = static_cast(num_parameters); - return 0; -} - int32_t ViEEncoder::ScaleInputImage(bool enable) { VideoFrameResampling resampling_mode = kFastRescaling; // TODO(mflodman) What? diff --git a/webrtc/video_engine/vie_encoder.h b/webrtc/video_engine/vie_encoder.h index 6b4449f149..54aacdbfa9 100644 --- a/webrtc/video_engine/vie_encoder.h +++ b/webrtc/video_engine/vie_encoder.h @@ -90,10 +90,6 @@ class ViEEncoder : public RtcpIntraFrameObserver, int32_t SetEncoder(const VideoCodec& video_codec); int32_t GetEncoder(VideoCodec* video_codec); - int32_t GetCodecConfigParameters( - unsigned char config_parameters[kConfigParameterSize], - unsigned char& config_parameters_size); - // Scale or crop/pad image. int32_t ScaleInputImage(bool enable);