From 233bfd2da4acef06ad9de1cbb1d4ba4a0bf7380e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Mon, 18 Jan 2016 20:23:40 +0100 Subject: [PATCH] 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} --- .../modules/video_coding/video_coding_impl.h | 17 ++-- webrtc/modules/video_coding/video_sender.cc | 88 +++++++++++++------ .../video_coding/video_sender_unittest.cc | 46 ++++++---- webrtc/video/vie_encoder.cc | 4 +- webrtc/video/vie_encoder.h | 2 +- 5 files changed, 101 insertions(+), 56 deletions(-) diff --git a/webrtc/modules/video_coding/video_coding_impl.h b/webrtc/modules/video_coding/video_coding_impl.h index f105fa9c18..1ed96e126b 100644 --- a/webrtc/modules/video_coding/video_coding_impl.h +++ b/webrtc/modules/video_coding/video_coding_impl.h @@ -99,19 +99,18 @@ class VideoSender { private: void SetEncoderParameters(EncoderParameters params) - EXCLUSIVE_LOCKS_REQUIRED(send_crit_); + EXCLUSIVE_LOCKS_REQUIRED(encoder_crit_); Clock* const clock_; rtc::scoped_ptr process_crit_sect_; - mutable rtc::CriticalSection send_crit_; + mutable rtc::CriticalSection encoder_crit_; VCMGenericEncoder* _encoder; - VCMEncodedFrameCallback _encodedFrameCallback; - std::vector _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 @@ class VideoSender { 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 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 ac901f95b9..f14f852114 100644 --- a/webrtc/modules/video_coding/video_sender.cc +++ b/webrtc/modules/video_coding/video_sender.cc @@ -32,7 +32,6 @@ VideoSender::VideoSender(Clock* clock, 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 @@ VideoSender::VideoSender(Clock* clock, 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 @@ int32_t VideoSender::Process() { } { - 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 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec, 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 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec, 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 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec, } 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 @@ void VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder, 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 @@ void VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder, _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 @@ int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate, 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 @@ void VideoSender::SetEncoderParameters(EncoderParameters params) { 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 @@ int32_t VideoSender::RegisterProtectionCallback( // 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 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame, const VideoContentMetrics* contentMetrics, const CodecSpecificInfo* codecSpecificInfo) { EncoderParameters encoder_params; + std::vector 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 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame, << "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::AddVideoFrame(const VideoFrame& videoFrame, } int32_t VideoSender::IntraFrameRequest(int stream_index) { - rtc::CritScope lock(&send_crit_); - if (stream_index < 0 || - static_cast(stream_index) >= _nextFrameTypes.size()) { - return -1; + { + rtc::CritScope lock(¶ms_crit_); + if (stream_index < 0 || + static_cast(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(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 741c7b7a60..f766b862eb 100644 --- a/webrtc/modules/video_coding/video_sender_unittest.cc +++ b/webrtc/modules/video_coding/video_sender_unittest.cc @@ -40,7 +40,12 @@ using webrtc::test::FrameGenerator; 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 TestVideoSender : public ::testing::Test { 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 @@ class TestVideoSenderWithMockEncoder : public TestVideoSender { 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 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 @@ class TestVideoSenderWithMockEncoder : public TestVideoSender { }; 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 @@ TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequestsInternalCapture) { // 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 a147b2415c..ed645bc4fc 100644 --- a/webrtc/video/vie_encoder.cc +++ b/webrtc/video/vie_encoder.cc @@ -401,8 +401,8 @@ void ViEEncoder::DeliverFrame(VideoFrame video_frame) { 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 a15fd8920b..a7452f7da8 100644 --- a/webrtc/video/vie_encoder.h +++ b/webrtc/video/vie_encoder.h @@ -91,7 +91,7 @@ class ViEEncoder : public RtcpIntraFrameObserver, // Implementing VideoCaptureCallback. void DeliverFrame(VideoFrame video_frame) override; - int32_t SendKeyFrame(); + void SendKeyFrame(); uint32_t LastObservedBitrateBps() const; int CodecTargetBitrate(uint32_t* bitrate) const;