Move keyframe requests outside encoder mutex.
Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.
BUG=webrtc:5410
R=stefan@webrtc.org
Review URL: https://codereview.webrtc.org/1600553003 .
Cr-Commit-Position: refs/heads/master@{#11294}
diff --git a/webrtc/modules/video_coding/video_coding_impl.h b/webrtc/modules/video_coding/video_coding_impl.h
index f105fa9..1ed96e1 100644
--- a/webrtc/modules/video_coding/video_coding_impl.h
+++ b/webrtc/modules/video_coding/video_coding_impl.h
@@ -99,19 +99,18 @@
private:
void SetEncoderParameters(EncoderParameters params)
- EXCLUSIVE_LOCKS_REQUIRED(send_crit_);
+ EXCLUSIVE_LOCKS_REQUIRED(encoder_crit_);
Clock* const clock_;
rtc::scoped_ptr<CriticalSectionWrapper> process_crit_sect_;
- mutable rtc::CriticalSection send_crit_;
+ mutable rtc::CriticalSection encoder_crit_;
VCMGenericEncoder* _encoder;
- VCMEncodedFrameCallback _encodedFrameCallback;
- std::vector<FrameType> _nextFrameTypes;
+ VCMEncodedFrameCallback _encodedFrameCallback GUARDED_BY(encoder_crit_);
media_optimization::MediaOptimization _mediaOpt;
VCMSendStatisticsCallback* _sendStatsCallback GUARDED_BY(process_crit_sect_);
- VCMCodecDataBase _codecDataBase GUARDED_BY(send_crit_);
- bool frame_dropper_enabled_ GUARDED_BY(send_crit_);
+ VCMCodecDataBase _codecDataBase GUARDED_BY(encoder_crit_);
+ bool frame_dropper_enabled_ GUARDED_BY(encoder_crit_);
VCMProcessTimer _sendStatsTimer;
// Must be accessed on the construction thread of VideoSender.
@@ -121,8 +120,10 @@
VCMQMSettingsCallback* const qm_settings_callback_;
VCMProtectionCallback* protection_callback_;
- rtc::CriticalSection params_lock_;
- EncoderParameters encoder_params_ GUARDED_BY(params_lock_);
+ rtc::CriticalSection params_crit_;
+ EncoderParameters encoder_params_ GUARDED_BY(params_crit_);
+ bool encoder_has_internal_source_ GUARDED_BY(params_crit_);
+ std::vector<FrameType> next_frame_types_ GUARDED_BY(params_crit_);
};
class VideoReceiver {
diff --git a/webrtc/modules/video_coding/video_sender.cc b/webrtc/modules/video_coding/video_sender.cc
index ac901f9..f14f852 100644
--- a/webrtc/modules/video_coding/video_sender.cc
+++ b/webrtc/modules/video_coding/video_sender.cc
@@ -32,7 +32,6 @@
process_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
_encoder(nullptr),
_encodedFrameCallback(post_encode_callback),
- _nextFrameTypes(1, kVideoFrameDelta),
_mediaOpt(clock_),
_sendStatsCallback(nullptr),
_codecDataBase(encoder_rate_observer, &_encodedFrameCallback),
@@ -41,7 +40,9 @@
current_codec_(),
qm_settings_callback_(qm_settings_callback),
protection_callback_(nullptr),
- encoder_params_({0, 0, 0, 0}) {
+ encoder_params_({0, 0, 0, 0}),
+ encoder_has_internal_source_(false),
+ next_frame_types_(1, kVideoFrameDelta) {
// Allow VideoSender to be created on one thread but used on another, post
// construction. This is currently how this class is being used by at least
// one external project (diffractor).
@@ -66,7 +67,7 @@
}
{
- rtc::CritScope cs(¶ms_lock_);
+ rtc::CritScope cs(¶ms_crit_);
// Force an encoder parameters update, so that incoming frame rate is
// updated even if bandwidth hasn't changed.
encoder_params_.input_frame_rate = _mediaOpt.InputFrameRate();
@@ -84,7 +85,7 @@
uint32_t numberOfCores,
uint32_t maxPayloadSize) {
RTC_DCHECK(main_thread_.CalledOnValidThread());
- rtc::CritScope lock(&send_crit_);
+ rtc::CritScope lock(&encoder_crit_);
if (sendCodec == nullptr) {
return VCM_PARAMETER_ERROR;
}
@@ -105,6 +106,9 @@
return VCM_CODEC_ERROR;
}
+ // SetSendCodec succeeded, _encoder should be set.
+ RTC_DCHECK(_encoder);
+
int numLayers;
if (sendCodec->codecType == kVideoCodecVP8) {
numLayers = sendCodec->codecSpecific.VP8.numberOfTemporalLayers;
@@ -122,9 +126,15 @@
} else if (frame_dropper_enabled_) {
_mediaOpt.EnableFrameDropper(true);
}
- _nextFrameTypes.clear();
- _nextFrameTypes.resize(VCM_MAX(sendCodec->numberOfSimulcastStreams, 1),
- kVideoFrameDelta);
+ {
+ rtc::CritScope cs(¶ms_crit_);
+ next_frame_types_.clear();
+ next_frame_types_.resize(VCM_MAX(sendCodec->numberOfSimulcastStreams, 1),
+ kVideoFrameKey);
+ // Cache InternalSource() to have this available from IntraFrameRequest()
+ // without having to acquire encoder_crit_ (avoid blocking on encoder use).
+ encoder_has_internal_source_ = _encoder->InternalSource();
+ }
_mediaOpt.SetEncodingData(sendCodec->codecType, sendCodec->maxBitrate * 1000,
sendCodec->startBitrate * 1000, sendCodec->width,
@@ -140,7 +150,7 @@
bool internalSource /*= false*/) {
RTC_DCHECK(main_thread_.CalledOnValidThread());
- rtc::CritScope lock(&send_crit_);
+ rtc::CritScope lock(&encoder_crit_);
if (externalEncoder == nullptr) {
bool wasSendCodec = false;
@@ -148,7 +158,9 @@
_codecDataBase.DeregisterExternalEncoder(payloadType, &wasSendCodec));
if (wasSendCodec) {
// Make sure the VCM doesn't use the de-registered codec
+ rtc::CritScope params_lock(¶ms_crit_);
_encoder = nullptr;
+ encoder_has_internal_source_ = false;
}
return;
}
@@ -190,7 +202,7 @@
uint32_t input_frame_rate = _mediaOpt.InputFrameRate();
- rtc::CritScope cs(¶ms_lock_);
+ rtc::CritScope cs(¶ms_crit_);
encoder_params_ = {target_rate, lossRate, rtt, input_frame_rate};
return VCM_OK;
@@ -210,7 +222,7 @@
int32_t VideoSender::RegisterTransportCallback(
VCMPacketizationCallback* transport) {
- rtc::CritScope lock(&send_crit_);
+ rtc::CritScope lock(&encoder_crit_);
_encodedFrameCallback.SetMediaOpt(&_mediaOpt);
_encodedFrameCallback.SetTransportCallback(transport);
return VCM_OK;
@@ -239,7 +251,7 @@
// Enable or disable a video protection method.
void VideoSender::SetVideoProtection(VCMVideoProtection videoProtection) {
- rtc::CritScope lock(&send_crit_);
+ rtc::CritScope lock(&encoder_crit_);
switch (videoProtection) {
case kProtectionNone:
_mediaOpt.SetProtectionMethod(media_optimization::kNone);
@@ -260,19 +272,16 @@
const VideoContentMetrics* contentMetrics,
const CodecSpecificInfo* codecSpecificInfo) {
EncoderParameters encoder_params;
+ std::vector<FrameType> next_frame_types;
{
- rtc::CritScope lock(¶ms_lock_);
+ rtc::CritScope lock(¶ms_crit_);
encoder_params = encoder_params_;
+ next_frame_types = next_frame_types_;
}
- rtc::CritScope lock(&send_crit_);
+ rtc::CritScope lock(&encoder_crit_);
if (_encoder == nullptr)
return VCM_UNINITIALIZED;
SetEncoderParameters(encoder_params);
- // TODO(holmer): Add support for dropping frames per stream. Currently we
- // only have one frame dropper for all streams.
- if (_nextFrameTypes[0] == kEmptyFrame) {
- return VCM_OK;
- }
if (_mediaOpt.DropFrame()) {
_encoder->OnDroppedFrame();
return VCM_OK;
@@ -294,13 +303,20 @@
<< "Frame conversion failed, won't be able to encode frame.";
}
int32_t ret =
- _encoder->Encode(converted_frame, codecSpecificInfo, _nextFrameTypes);
+ _encoder->Encode(converted_frame, codecSpecificInfo, next_frame_types);
if (ret < 0) {
LOG(LS_ERROR) << "Failed to encode frame. Error code: " << ret;
return ret;
}
- for (size_t i = 0; i < _nextFrameTypes.size(); ++i) {
- _nextFrameTypes[i] = kVideoFrameDelta; // Default frame type.
+ {
+ // Change all keyframe requests to encode delta frames the next time.
+ rtc::CritScope lock(¶ms_crit_);
+ for (size_t i = 0; i < next_frame_types_.size(); ++i) {
+ // Check for equality (same requested as before encoding) to not
+ // accidentally drop a keyframe request while encoding.
+ if (next_frame_types[i] == next_frame_types_[i])
+ next_frame_types_[i] = kVideoFrameDelta;
+ }
}
if (qm_settings_callback_)
qm_settings_callback_->SetTargetFramerate(_encoder->GetTargetFramerate());
@@ -308,24 +324,38 @@
}
int32_t VideoSender::IntraFrameRequest(int stream_index) {
- rtc::CritScope lock(&send_crit_);
- if (stream_index < 0 ||
- static_cast<unsigned int>(stream_index) >= _nextFrameTypes.size()) {
- return -1;
+ {
+ rtc::CritScope lock(¶ms_crit_);
+ if (stream_index < 0 ||
+ static_cast<size_t>(stream_index) >= next_frame_types_.size()) {
+ return -1;
+ }
+ next_frame_types_[stream_index] = kVideoFrameKey;
+ if (!encoder_has_internal_source_)
+ return VCM_OK;
}
- _nextFrameTypes[stream_index] = kVideoFrameKey;
+ // TODO(pbos): Remove when InternalSource() is gone. Both locks have to be
+ // held here for internal consistency, since _encoder could be removed while
+ // not holding encoder_crit_. Checks have to be performed again since
+ // params_crit_ was dropped to not cause lock-order inversions with
+ // encoder_crit_.
+ rtc::CritScope lock(&encoder_crit_);
+ rtc::CritScope params_lock(¶ms_crit_);
+ if (static_cast<size_t>(stream_index) >= next_frame_types_.size())
+ return -1;
if (_encoder != nullptr && _encoder->InternalSource()) {
// Try to request the frame if we have an external encoder with
// internal source since AddVideoFrame never will be called.
- if (_encoder->RequestFrame(_nextFrameTypes) == WEBRTC_VIDEO_CODEC_OK) {
- _nextFrameTypes[stream_index] = kVideoFrameDelta;
+ if (_encoder->RequestFrame(next_frame_types_) == WEBRTC_VIDEO_CODEC_OK) {
+ // Try to remove just-performed keyframe request, if stream still exists.
+ next_frame_types_[stream_index] = kVideoFrameDelta;
}
}
return VCM_OK;
}
int32_t VideoSender::EnableFrameDropper(bool enable) {
- rtc::CritScope lock(&send_crit_);
+ rtc::CritScope lock(&encoder_crit_);
frame_dropper_enabled_ = enable;
_mediaOpt.EnableFrameDropper(enable);
return VCM_OK;
diff --git a/webrtc/modules/video_coding/video_sender_unittest.cc b/webrtc/modules/video_coding/video_sender_unittest.cc
index 741c7b7..f766b86 100644
--- a/webrtc/modules/video_coding/video_sender_unittest.cc
+++ b/webrtc/modules/video_coding/video_sender_unittest.cc
@@ -40,7 +40,12 @@
namespace webrtc {
namespace vcm {
namespace {
-enum { kMaxNumberOfTemporalLayers = 3 };
+static const int kDefaultHeight = 720;
+static const int kDefaultWidth = 1280;
+static const int kMaxNumberOfTemporalLayers = 3;
+static const int kNumberOfLayers = 3;
+static const int kNumberOfStreams = 3;
+static const int kUnusedPayloadType = 10;
struct Vp8StreamInfo {
float framerate_fps[kMaxNumberOfTemporalLayers];
@@ -196,12 +201,6 @@
class TestVideoSenderWithMockEncoder : public TestVideoSender {
protected:
- static const int kDefaultWidth = 1280;
- static const int kDefaultHeight = 720;
- static const int kNumberOfStreams = 3;
- static const int kNumberOfLayers = 3;
- static const int kUnusedPayloadType = 10;
-
void SetUp() override {
TestVideoSender::SetUp();
sender_->RegisterExternalEncoder(&encoder_, kUnusedPayloadType, false);
@@ -222,20 +221,29 @@
void TearDown() override { sender_.reset(); }
void ExpectIntraRequest(int stream) {
- if (stream == -1) {
- // No intra request expected.
- EXPECT_CALL(
- encoder_,
- Encode(_, _, Pointee(ElementsAre(kVideoFrameDelta, kVideoFrameDelta,
- kVideoFrameDelta))))
+ ExpectEncodeWithFrameTypes(stream, false);
+ }
+
+ void ExpectInitialKeyFrames() {
+ ExpectEncodeWithFrameTypes(-1, true);
+ }
+
+ void ExpectEncodeWithFrameTypes(int intra_request_stream, bool first_frame) {
+ if (intra_request_stream == -1) {
+ // No intra request expected, keyframes on first frame.
+ FrameType frame_type = first_frame ? kVideoFrameKey : kVideoFrameDelta;
+ EXPECT_CALL(encoder_,
+ Encode(_, _, Pointee(ElementsAre(frame_type, frame_type,
+ frame_type))))
.Times(1)
.WillRepeatedly(Return(0));
return;
}
- assert(stream >= 0);
- assert(stream < kNumberOfStreams);
+ ASSERT_FALSE(first_frame);
+ ASSERT_GE(intra_request_stream, 0);
+ ASSERT_LT(intra_request_stream, kNumberOfStreams);
std::vector<FrameType> frame_types(kNumberOfStreams, kVideoFrameDelta);
- frame_types[stream] = kVideoFrameKey;
+ frame_types[intra_request_stream] = kVideoFrameKey;
EXPECT_CALL(encoder_,
Encode(_, _, Pointee(ElementsAreArray(&frame_types[0],
frame_types.size()))))
@@ -260,6 +268,9 @@
};
TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequests) {
+ // Initial request should be all keyframes.
+ ExpectInitialKeyFrames();
+ AddFrame();
EXPECT_EQ(0, sender_->IntraFrameRequest(0));
ExpectIntraRequest(0);
AddFrame();
@@ -293,6 +304,9 @@
// Register encoder with internal capture.
sender_->RegisterExternalEncoder(&encoder_, kUnusedPayloadType, true);
EXPECT_EQ(0, sender_->RegisterSendCodec(&settings_, 1, 1200));
+ // Initial request should be all keyframes.
+ ExpectInitialKeyFrames();
+ AddFrame();
ExpectIntraRequest(0);
EXPECT_EQ(0, sender_->IntraFrameRequest(0));
ExpectIntraRequest(1);
diff --git a/webrtc/video/vie_encoder.cc b/webrtc/video/vie_encoder.cc
index a147b24..ed645bc 100644
--- a/webrtc/video/vie_encoder.cc
+++ b/webrtc/video/vie_encoder.cc
@@ -401,8 +401,8 @@
vcm_->AddVideoFrame(*frame_to_send);
}
-int ViEEncoder::SendKeyFrame() {
- return vcm_->IntraFrameRequest(0);
+void ViEEncoder::SendKeyFrame() {
+ vcm_->IntraFrameRequest(0);
}
uint32_t ViEEncoder::LastObservedBitrateBps() const {
diff --git a/webrtc/video/vie_encoder.h b/webrtc/video/vie_encoder.h
index a15fd89..a7452f7 100644
--- a/webrtc/video/vie_encoder.h
+++ b/webrtc/video/vie_encoder.h
@@ -91,7 +91,7 @@
// Implementing VideoCaptureCallback.
void DeliverFrame(VideoFrame video_frame) override;
- int32_t SendKeyFrame();
+ void SendKeyFrame();
uint32_t LastObservedBitrateBps() const;
int CodecTargetBitrate(uint32_t* bitrate) const;