diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.cc b/webrtc/modules/video_coding/main/source/generic_encoder.cc index 208f925563..dcddf2f5ce 100644 --- a/webrtc/modules/video_coding/main/source/generic_encoder.cc +++ b/webrtc/modules/video_coding/main/source/generic_encoder.cc @@ -60,11 +60,9 @@ VCMGenericEncoder::VCMGenericEncoder(VideoEncoder* encoder, bool internalSource) : encoder_(encoder), rate_observer_(rate_observer), - _codecType(kVideoCodecUnknown), - _VCMencodedFrameCallback(NULL), - _bitRate(0), - _frameRate(0), - _internalSource(internalSource) { + bit_rate_(0), + frame_rate_(0), + internal_source_(internalSource) { } VCMGenericEncoder::~VCMGenericEncoder() @@ -73,9 +71,12 @@ VCMGenericEncoder::~VCMGenericEncoder() int32_t VCMGenericEncoder::Release() { - _bitRate = 0; - _frameRate = 0; - _VCMencodedFrameCallback = NULL; + { + rtc::CritScope lock(&rates_lock_); + bit_rate_ = 0; + frame_rate_ = 0; + } + return encoder_->Release(); } @@ -84,9 +85,12 @@ VCMGenericEncoder::InitEncode(const VideoCodec* settings, int32_t numberOfCores, size_t maxPayloadSize) { - _bitRate = settings->startBitrate * 1000; - _frameRate = settings->maxFramerate; - _codecType = settings->codecType; + { + rtc::CritScope lock(&rates_lock_); + bit_rate_ = settings->startBitrate * 1000; + frame_rate_ = settings->maxFramerate; + } + if (encoder_->InitEncode(settings, numberOfCores, maxPayloadSize) != 0) { LOG(LS_ERROR) << "Failed to initialize the encoder associated with " "payload name: " << settings->plName; @@ -120,8 +124,13 @@ VCMGenericEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate) { return ret; } - _bitRate = newBitRate; - _frameRate = frameRate; + + { + rtc::CritScope lock(&rates_lock_); + bit_rate_ = newBitRate; + frame_rate_ = frameRate; + } + if (rate_observer_ != nullptr) rate_observer_->OnSetRates(newBitRate, frameRate); return VCM_OK; @@ -140,12 +149,14 @@ VCMGenericEncoder::CodecConfigParameters(uint8_t* buffer, int32_t size) uint32_t VCMGenericEncoder::BitRate() const { - return _bitRate; + rtc::CritScope lock(&rates_lock_); + return bit_rate_; } uint32_t VCMGenericEncoder::FrameRate() const { - return _frameRate; + rtc::CritScope lock(&rates_lock_); + return frame_rate_; } int32_t @@ -166,15 +177,14 @@ int32_t VCMGenericEncoder::RequestFrame( int32_t VCMGenericEncoder::RegisterEncodeCallback(VCMEncodedFrameCallback* VCMencodedFrameCallback) { - _VCMencodedFrameCallback = VCMencodedFrameCallback; - _VCMencodedFrameCallback->SetInternalSource(_internalSource); - return encoder_->RegisterEncodeCompleteCallback(_VCMencodedFrameCallback); + VCMencodedFrameCallback->SetInternalSource(internal_source_); + return encoder_->RegisterEncodeCompleteCallback(VCMencodedFrameCallback); } bool VCMGenericEncoder::InternalSource() const { - return _internalSource; + return internal_source_; } /*************************** diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.h b/webrtc/modules/video_coding/main/source/generic_encoder.h index 1fefc405cb..eba77150be 100644 --- a/webrtc/modules/video_coding/main/source/generic_encoder.h +++ b/webrtc/modules/video_coding/main/source/generic_encoder.h @@ -16,6 +16,7 @@ #include +#include "webrtc/base/criticalsection.h" #include "webrtc/base/scoped_ptr.h" namespace webrtc { @@ -74,9 +75,9 @@ class VCMGenericEncoder { friend class VCMCodecDataBase; public: - VCMGenericEncoder(VideoEncoder* encoder, - VideoEncoderRateObserver* rate_observer, - bool internalSource); + VCMGenericEncoder(VideoEncoder* encoder, + VideoEncoderRateObserver* rate_observer, + bool internalSource); ~VCMGenericEncoder(); /** * Free encoder memory @@ -102,6 +103,9 @@ public: * 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. @@ -132,11 +136,10 @@ public: private: VideoEncoder* const encoder_; VideoEncoderRateObserver* const rate_observer_; - VideoCodecType _codecType; - VCMEncodedFrameCallback* _VCMencodedFrameCallback; - uint32_t _bitRate; - uint32_t _frameRate; - bool _internalSource; + uint32_t bit_rate_; + uint32_t frame_rate_; + const bool internal_source_; + mutable rtc::CriticalSection rates_lock_; }; // end of VCMGenericEncoder class } // namespace webrtc diff --git a/webrtc/modules/video_coding/main/source/video_sender.cc b/webrtc/modules/video_coding/main/source/video_sender.cc index 5a4ed1f9d7..032dab7587 100644 --- a/webrtc/modules/video_coding/main/source/video_sender.cc +++ b/webrtc/modules/video_coding/main/source/video_sender.cc @@ -193,6 +193,8 @@ VideoCodecType VideoSender::SendCodecBlocking() const { int32_t VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder, uint8_t payloadType, bool internalSource /*= false*/) { + DCHECK(main_thread_.CalledOnValidThread()); + CriticalSectionScoped cs(_sendCritSect); if (externalEncoder == NULL) { @@ -229,7 +231,10 @@ int32_t VideoSender::SentFrameCount(VCMFrameCount* frameCount) { // Get encode bitrate int VideoSender::Bitrate(unsigned int* bitrate) const { - CriticalSectionScoped cs(_sendCritSect); + 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) { return VCM_UNINITIALIZED; @@ -240,7 +245,10 @@ int VideoSender::Bitrate(unsigned int* bitrate) const { // Get encode frame rate int VideoSender::FrameRate(unsigned int* framerate) const { - CriticalSectionScoped cs(_sendCritSect); + 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. + // input frame rate, not compensated if (!_encoder) { return VCM_UNINITIALIZED; @@ -249,32 +257,33 @@ int VideoSender::FrameRate(unsigned int* framerate) const { return 0; } -// Set channel parameters int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate, uint8_t lossRate, int64_t rtt) { - int32_t ret = 0; - { - CriticalSectionScoped sendCs(_sendCritSect); - uint32_t targetRate = _mediaOpt.SetTargetRates(target_bitrate, - lossRate, - rtt, - protection_callback_, - qm_settings_callback_); - if (_encoder != NULL) { - ret = _encoder->SetChannelParameters(lossRate, rtt); - if (ret < 0) { - return ret; - } - ret = (int32_t)_encoder->SetRates(targetRate, _mediaOpt.InputFrameRate()); - if (ret < 0) { - return ret; - } - } else { - return VCM_UNINITIALIZED; - } // encoder - } // send side - return VCM_OK; + // TODO(tommi,mflodman): This method is called on the network thread via the + // OnNetworkChanged event (ViEEncoder::OnNetworkChanged). Could we instead + // post the updated information to the encoding thread and not grab a lock + // here? This effectively means that the network thread will be blocked for + // as much as frame encoding period. + + CriticalSectionScoped sendCs(_sendCritSect); + uint32_t target_rate = _mediaOpt.SetTargetRates(target_bitrate, + lossRate, + rtt, + protection_callback_, + qm_settings_callback_); + uint32_t input_frame_rate = _mediaOpt.InputFrameRate(); + + int32_t ret = VCM_UNINITIALIZED; + static_assert(VCM_UNINITIALIZED < 0, "VCM_UNINITIALIZED must be negative."); + + if (_encoder != NULL) { + ret = _encoder->SetChannelParameters(lossRate, rtt); + if (ret >= 0) { + ret = _encoder->SetRates(target_rate, input_frame_rate); + } + } + return ret; } int32_t VideoSender::RegisterTransportCallback( @@ -300,6 +309,9 @@ int32_t VideoSender::RegisterSendStatisticsCallback( int32_t VideoSender::RegisterVideoQMCallback( VCMQMSettingsCallback* qm_settings_callback) { CriticalSectionScoped cs(_sendCritSect); + DCHECK(qm_settings_callback_ == qm_settings_callback || + !qm_settings_callback_ || + !qm_settings_callback) << "Overwriting the previous callback?"; qm_settings_callback_ = qm_settings_callback; _mediaOpt.EnableQM(qm_settings_callback_ != NULL); return VCM_OK; @@ -310,6 +322,9 @@ int32_t VideoSender::RegisterVideoQMCallback( int32_t VideoSender::RegisterProtectionCallback( VCMProtectionCallback* protection_callback) { CriticalSectionScoped cs(_sendCritSect); + DCHECK(protection_callback_ == protection_callback || + !protection_callback_ || + !protection_callback) << "Overwriting the previous callback?"; protection_callback_ = protection_callback; return VCM_OK; }