From 92696cd0c6f2082bea29270a11e2ffbd03f7e2b7 Mon Sep 17 00:00:00 2001 From: "tommi@webrtc.org" Date: Sat, 7 Mar 2015 09:26:16 +0000 Subject: [PATCH] Speculative revert of 8631 "Remove lock from Bitrate() and FrameRate() in Video..." We ran into the alignment problem on Mac 10.9 debug again. This is the only CL I see in the range that adds an rtc::CriticalSection, so I'm trying out reverting it before attempting another roll. > Remove lock from Bitrate() and FrameRate() in VideoSender. > These methods are called on the VideoSender's construction thread, which is the same thread as modifies the value of _encoder. It's therefore safe to not require a lock to access _encoder on this thread. > > I'm making access to the rate variables from VCMGenericEncoder, thread safe, by using a lock that's not associated with the encoder. There should be little to no contention there. While modifying VCMGenericEncoder, I noticed that a couple of member variables weren't needed, so I removed them. > > The reason for this change is that getStats is currently contending with the encoder when Bitrate() is called. On my machine, this means that getStats can take about 25-30ms instead of ~1ms. > > Also adding some documentation for other methods and a suggestion for how we could avoid contention between the encoder and the network thread. > > BUG=2822 > R=mflodman@webrtc.org > > Review URL: https://webrtc-codereview.appspot.com/43479004 TBR=tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/45529004 Cr-Commit-Position: refs/heads/master@{#8640} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8640 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../main/source/generic_encoder.cc | 48 ++++++-------- .../main/source/generic_encoder.h | 19 +++--- .../video_coding/main/source/video_sender.cc | 65 +++++++------------ 3 files changed, 52 insertions(+), 80 deletions(-) diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.cc b/webrtc/modules/video_coding/main/source/generic_encoder.cc index dcddf2f5ce..208f925563 100644 --- a/webrtc/modules/video_coding/main/source/generic_encoder.cc +++ b/webrtc/modules/video_coding/main/source/generic_encoder.cc @@ -60,9 +60,11 @@ VCMGenericEncoder::VCMGenericEncoder(VideoEncoder* encoder, bool internalSource) : encoder_(encoder), rate_observer_(rate_observer), - bit_rate_(0), - frame_rate_(0), - internal_source_(internalSource) { + _codecType(kVideoCodecUnknown), + _VCMencodedFrameCallback(NULL), + _bitRate(0), + _frameRate(0), + _internalSource(internalSource) { } VCMGenericEncoder::~VCMGenericEncoder() @@ -71,12 +73,9 @@ VCMGenericEncoder::~VCMGenericEncoder() int32_t VCMGenericEncoder::Release() { - { - rtc::CritScope lock(&rates_lock_); - bit_rate_ = 0; - frame_rate_ = 0; - } - + _bitRate = 0; + _frameRate = 0; + _VCMencodedFrameCallback = NULL; return encoder_->Release(); } @@ -85,12 +84,9 @@ VCMGenericEncoder::InitEncode(const VideoCodec* settings, int32_t numberOfCores, size_t maxPayloadSize) { - { - rtc::CritScope lock(&rates_lock_); - bit_rate_ = settings->startBitrate * 1000; - frame_rate_ = settings->maxFramerate; - } - + _bitRate = settings->startBitrate * 1000; + _frameRate = settings->maxFramerate; + _codecType = settings->codecType; if (encoder_->InitEncode(settings, numberOfCores, maxPayloadSize) != 0) { LOG(LS_ERROR) << "Failed to initialize the encoder associated with " "payload name: " << settings->plName; @@ -124,13 +120,8 @@ VCMGenericEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate) { return ret; } - - { - rtc::CritScope lock(&rates_lock_); - bit_rate_ = newBitRate; - frame_rate_ = frameRate; - } - + _bitRate = newBitRate; + _frameRate = frameRate; if (rate_observer_ != nullptr) rate_observer_->OnSetRates(newBitRate, frameRate); return VCM_OK; @@ -149,14 +140,12 @@ VCMGenericEncoder::CodecConfigParameters(uint8_t* buffer, int32_t size) uint32_t VCMGenericEncoder::BitRate() const { - rtc::CritScope lock(&rates_lock_); - return bit_rate_; + return _bitRate; } uint32_t VCMGenericEncoder::FrameRate() const { - rtc::CritScope lock(&rates_lock_); - return frame_rate_; + return _frameRate; } int32_t @@ -177,14 +166,15 @@ int32_t VCMGenericEncoder::RequestFrame( int32_t VCMGenericEncoder::RegisterEncodeCallback(VCMEncodedFrameCallback* VCMencodedFrameCallback) { - VCMencodedFrameCallback->SetInternalSource(internal_source_); - return encoder_->RegisterEncodeCompleteCallback(VCMencodedFrameCallback); + _VCMencodedFrameCallback = VCMencodedFrameCallback; + _VCMencodedFrameCallback->SetInternalSource(_internalSource); + return encoder_->RegisterEncodeCompleteCallback(_VCMencodedFrameCallback); } bool VCMGenericEncoder::InternalSource() const { - return internal_source_; + return _internalSource; } /*************************** diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.h b/webrtc/modules/video_coding/main/source/generic_encoder.h index eba77150be..1fefc405cb 100644 --- a/webrtc/modules/video_coding/main/source/generic_encoder.h +++ b/webrtc/modules/video_coding/main/source/generic_encoder.h @@ -16,7 +16,6 @@ #include -#include "webrtc/base/criticalsection.h" #include "webrtc/base/scoped_ptr.h" namespace webrtc { @@ -75,9 +74,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 @@ -103,9 +102,6 @@ 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. @@ -136,10 +132,11 @@ public: private: VideoEncoder* const encoder_; VideoEncoderRateObserver* const rate_observer_; - uint32_t bit_rate_; - uint32_t frame_rate_; - const bool internal_source_; - mutable rtc::CriticalSection rates_lock_; + VideoCodecType _codecType; + VCMEncodedFrameCallback* _VCMencodedFrameCallback; + uint32_t _bitRate; + uint32_t _frameRate; + bool _internalSource; }; // 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 032dab7587..5a4ed1f9d7 100644 --- a/webrtc/modules/video_coding/main/source/video_sender.cc +++ b/webrtc/modules/video_coding/main/source/video_sender.cc @@ -193,8 +193,6 @@ 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) { @@ -231,10 +229,7 @@ int32_t VideoSender::SentFrameCount(VCMFrameCount* frameCount) { // Get encode bitrate int VideoSender::Bitrate(unsigned int* bitrate) const { - 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. - + CriticalSectionScoped cs(_sendCritSect); // return the bit rate which the encoder is set to if (!_encoder) { return VCM_UNINITIALIZED; @@ -245,10 +240,7 @@ int VideoSender::Bitrate(unsigned int* bitrate) const { // Get encode frame rate int VideoSender::FrameRate(unsigned int* framerate) const { - 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. - + CriticalSectionScoped cs(_sendCritSect); // input frame rate, not compensated if (!_encoder) { return VCM_UNINITIALIZED; @@ -257,33 +249,32 @@ 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) { - // 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 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; } int32_t VideoSender::RegisterTransportCallback( @@ -309,9 +300,6 @@ 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; @@ -322,9 +310,6 @@ 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; }