zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove lock from Bitrate() and FrameRate() in VideoSender.

Browse Source 
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

Cr-Commit-Position: refs/heads/master@{#8631}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8631 4adac7df-926f-26a2-2b94-8c16560cd09d
This commit is contained in:
tommi@webrtc.org
2015-03-06 12:21:30 +00:00
parent f98030b029
commit 0d5ea21325
3 changed files with 80 additions and 52 deletions
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,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_;
}
/***************************

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

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

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

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