Reland 8631 "Speculative revert of 8631 "Remove lock from Bitrat..."
> 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
TBR=tommi@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/46519004
Cr-Commit-Position: refs/heads/master@{#8645}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8645 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/webrtc/modules/video_coding/main/source/generic_encoder.cc b/webrtc/modules/video_coding/main/source/generic_encoder.cc
index 208f925..dcddf2f 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 @@
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 @@
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 @@
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 @@
{
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 @@
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::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 1fefc40..eba7715 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 <stdio.h>
+#include "webrtc/base/criticalsection.h"
#include "webrtc/base/scoped_ptr.h"
namespace webrtc {
@@ -74,9 +75,9 @@
{
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 @@
* 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 @@
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 5a4ed1f..032dab7 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 @@
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 @@
// 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 @@
// 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 @@
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::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::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;
}
