generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Speculative revert of 8631 "Remove lock from Bitrate() and FrameRate() in Video..."

Browse Source 
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
This commit is contained in:
tommi@webrtc.org 2015-03-07 09:26:16 +00:00
parent dc08a230da
commit 92696cd0c6
3 changed files with 52 additions and 80 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
webrtc/modules/video_coding/main/source/generic_encoder.cc
View File

@ -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;
}
/***************************

19
webrtc/modules/video_coding/main/source/generic_encoder.h
View File

@ -16,7 +16,6 @@
#include <stdio.h>
#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

65
webrtc/modules/video_coding/main/source/video_sender.cc
View File

@ -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;
}