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}
diff --git a/webrtc/modules/video_coding/codecs/i420/include/i420.h b/webrtc/modules/video_coding/codecs/i420/include/i420.h
index e54e78d..8990ccf 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 @@
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 5243d9a..6c926d4 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 @@
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 @@
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 d2dd050..67f7b63 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 @@
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 dbb1c17..e3ae0dd 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 @@
: 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 @@
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 @@
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 @@
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 25235b6..5482e50 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 @@
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 @@
int32_t Encode(const VideoFrame& inputFrame,
const CodecSpecificInfo* codecSpecificInfo,
const std::vector<FrameType>& 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 @@
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 cae0042..b0a6754 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 @@
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 3490027..57f38da 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 @@
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 @@
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 cdbaffd..38089f7 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 @@
_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 @@
// 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 @@
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 @@
// 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 @@
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 @@
{
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 b1d645d..e9c8bd7 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, 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 @@
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 3d9cf39..2cd4750 100644
--- a/webrtc/test/configurable_frame_size_encoder.cc
+++ b/webrtc/test/configurable_frame_size_encoder.cc
@@ -82,11 +82,6 @@
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 79514ef..3794e8d 100644
--- a/webrtc/test/configurable_frame_size_encoder.h
+++ b/webrtc/test/configurable_frame_size_encoder.h
@@ -43,8 +43,6 @@
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 609c073..f255336 100644
--- a/webrtc/video_encoder.h
+++ b/webrtc/video_encoder.h
@@ -121,9 +121,6 @@
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 f705bc2..0f4a5a1 100644
--- a/webrtc/video_engine/vie_encoder.cc
+++ b/webrtc/video_engine/vie_encoder.cc
@@ -258,19 +258,6 @@
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<unsigned char>(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 6b4449f..54aacdb 100644
--- a/webrtc/video_engine/vie_encoder.h
+++ b/webrtc/video_engine/vie_encoder.h
@@ -90,10 +90,6 @@
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);
