From d0a71ba1ae69f73025e7381fa7e5593b3a0f7833 Mon Sep 17 00:00:00 2001 From: tommi Date: Tue, 14 Mar 2017 04:16:20 -0700 Subject: [PATCH] Updates to VCMDecodedFrameCallback, VideoReceiver and a few related classes/tests. * The _receiveCallback member of VCMDecodedFrameCallback does actually not require locking now that the threading model is slightly clearer. Documentation and checks have been added. * UserReceiveCallback() never returns null and must always be called on the decoder thread. Checks have been added and the two test suites that were failing to set this callback, have been fixed and a new mock class added. (looks like sakal@ may have hit some issues with flaky tests there). * Changed VcmPayloadSink to use move semantics which I suspect was the intention at the time the code was written (when we didn't have move semantics). * Added thread checker to a couple of classes and started adding thread checks for known behavior. There's more to be done there. * Remove the |_decoder| member variable in VideoReceiver. It is not needed and as it could be used, left us open to a race. * TODOs added for places where we can reduce locking. I suspect that we can get away with not needing a lock around _codecDataBase in VideoReceiver once we've got a clear picture of the threading model and ensured that all adhere to it. BUG=webrtc:7328 Review-Url: https://codereview.webrtc.org/2744013002 Cr-Commit-Position: refs/heads/master@{#17226} --- webrtc/modules/video_coding/codec_database.cc | 16 +++--- .../modules/video_coding/generic_decoder.cc | 40 +++++--------- webrtc/modules/video_coding/generic_decoder.h | 52 +++++++++++-------- .../include/mock/mock_vcm_callbacks.h | 11 ++++ .../test/vcm_payload_sink_factory.cc | 49 +++++++++-------- .../modules/video_coding/video_coding_impl.cc | 12 +++-- .../modules/video_coding/video_coding_impl.h | 8 ++- .../video_coding_robustness_unittest.cc | 15 ++++-- webrtc/modules/video_coding/video_receiver.cc | 36 ++++++++++--- .../video_coding/video_receiver_unittest.cc | 9 ++++ webrtc/video/video_receive_stream.cc | 4 +- webrtc/video/video_stream_decoder.cc | 4 ++ 12 files changed, 158 insertions(+), 98 deletions(-) diff --git a/webrtc/modules/video_coding/codec_database.cc b/webrtc/modules/video_coding/codec_database.cc index 75e7043509..01e51ce287 100644 --- a/webrtc/modules/video_coding/codec_database.cc +++ b/webrtc/modules/video_coding/codec_database.cc @@ -10,8 +10,6 @@ #include "webrtc/modules/video_coding/codec_database.h" -#include - #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" #include "webrtc/modules/video_coding/codecs/h264/include/h264.h" @@ -79,7 +77,7 @@ VCMDecoderMapItem::VCMDecoderMapItem(VideoCodec* settings, : settings(settings), number_of_cores(number_of_cores), require_key_frame(require_key_frame) { - assert(number_of_cores >= 0); + RTC_DCHECK_GE(number_of_cores, 0); } VCMExtDecoderMapItem::VCMExtDecoderMapItem( @@ -280,7 +278,7 @@ VideoCodecType VCMCodecDataBase::SendCodec() const { bool VCMCodecDataBase::DeregisterExternalEncoder(uint8_t payload_type, bool* was_send_codec) { - assert(was_send_codec); + RTC_DCHECK(was_send_codec); *was_send_codec = false; if (encoder_payload_type_ != payload_type) { return false; @@ -452,7 +450,7 @@ bool VCMCodecDataBase::DeregisterReceiveCodec(uint8_t payload_type) { } bool VCMCodecDataBase::ReceiveCodec(VideoCodec* current_receive_codec) const { - assert(current_receive_codec); + RTC_DCHECK(current_receive_codec); if (!ptr_decoder_) { return false; } @@ -470,6 +468,7 @@ VideoCodecType VCMCodecDataBase::ReceiveCodec() const { VCMGenericDecoder* VCMCodecDataBase::GetDecoder( const VCMEncodedFrame& frame, VCMDecodedFrameCallback* decoded_frame_callback) { + RTC_DCHECK(decoded_frame_callback->UserReceiveCallback()); uint8_t payload_type = frame.PayloadType(); if (payload_type == receive_codec_.plType || payload_type == 0) { return ptr_decoder_; @@ -485,8 +484,7 @@ VCMGenericDecoder* VCMCodecDataBase::GetDecoder( return nullptr; } VCMReceiveCallback* callback = decoded_frame_callback->UserReceiveCallback(); - if (callback) - callback->OnIncomingPayloadType(receive_codec_.plType); + callback->OnIncomingPayloadType(receive_codec_.plType); if (ptr_decoder_->RegisterDecodeCompleteCallback(decoded_frame_callback) < 0) { ReleaseDecoder(ptr_decoder_); @@ -499,7 +497,7 @@ VCMGenericDecoder* VCMCodecDataBase::GetDecoder( void VCMCodecDataBase::ReleaseDecoder(VCMGenericDecoder* decoder) const { if (decoder) { - assert(decoder->_decoder); + RTC_DCHECK(decoder->_decoder); decoder->Release(); if (!decoder->External()) { delete decoder->_decoder; @@ -524,7 +522,7 @@ VCMGenericDecoder* VCMCodecDataBase::CreateAndInitDecoder( uint8_t payload_type = frame.PayloadType(); LOG(LS_INFO) << "Initializing decoder with payload type '" << static_cast(payload_type) << "'."; - assert(new_codec); + RTC_DCHECK(new_codec); const VCMDecoderMapItem* decoder_item = FindDecoderItem(payload_type); if (!decoder_item) { LOG(LS_ERROR) << "Can't find a decoder associated with payload type: " diff --git a/webrtc/modules/video_coding/generic_decoder.cc b/webrtc/modules/video_coding/generic_decoder.cc index 34500c69e2..2121ab6306 100644 --- a/webrtc/modules/video_coding/generic_decoder.cc +++ b/webrtc/modules/video_coding/generic_decoder.cc @@ -20,25 +20,26 @@ namespace webrtc { VCMDecodedFrameCallback::VCMDecodedFrameCallback(VCMTiming* timing, Clock* clock) - : _critSect(CriticalSectionWrapper::CreateCriticalSection()), - _clock(clock), - _receiveCallback(NULL), + : _clock(clock), _timing(timing), _timestampMap(kDecoderFrameMemoryLength), _lastReceivedPictureID(0) {} VCMDecodedFrameCallback::~VCMDecodedFrameCallback() { - delete _critSect; } void VCMDecodedFrameCallback::SetUserReceiveCallback( VCMReceiveCallback* receiveCallback) { - CriticalSectionScoped cs(_critSect); + RTC_DCHECK(construction_thread_.CalledOnValidThread()); + RTC_DCHECK((!_receiveCallback && receiveCallback) || + (_receiveCallback && !receiveCallback)); _receiveCallback = receiveCallback; } VCMReceiveCallback* VCMDecodedFrameCallback::UserReceiveCallback() { - CriticalSectionScoped cs(_critSect); + // Called on the decode thread via VCMCodecDataBase::GetDecoder. + // The callback must always have been set before this happens. + RTC_DCHECK(_receiveCallback); return _receiveCallback; } @@ -58,16 +59,15 @@ int32_t VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage, void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage, rtc::Optional decode_time_ms, rtc::Optional qp) { + RTC_DCHECK(_receiveCallback) << "Callback must not be null at this point"; TRACE_EVENT_INSTANT1("webrtc", "VCMDecodedFrameCallback::Decoded", "timestamp", decodedImage.timestamp()); // TODO(holmer): We should improve this so that we can handle multiple // callbacks from one call to Decode(). VCMFrameInformation* frameInfo; - VCMReceiveCallback* callback; { - CriticalSectionScoped cs(_critSect); + rtc::CritScope cs(&lock_); frameInfo = _timestampMap.Pop(decodedImage.timestamp()); - callback = _receiveCallback; } if (frameInfo == NULL) { @@ -87,22 +87,12 @@ void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage, decodedImage.set_timestamp_us( frameInfo->renderTimeMs * rtc::kNumMicrosecsPerMillisec); decodedImage.set_rotation(frameInfo->rotation); - // TODO(sakal): Investigate why callback is NULL sometimes and replace if - // statement with a DCHECK. - if (callback) { - callback->FrameToRender(decodedImage, qp); - } else { - LOG(LS_WARNING) << "No callback, dropping frame."; - } + _receiveCallback->FrameToRender(decodedImage, qp); } int32_t VCMDecodedFrameCallback::ReceivedDecodedReferenceFrame( const uint64_t pictureId) { - CriticalSectionScoped cs(_critSect); - if (_receiveCallback != NULL) { - return _receiveCallback->ReceivedDecodedReferenceFrame(pictureId); - } - return -1; + return _receiveCallback->ReceivedDecodedReferenceFrame(pictureId); } int32_t VCMDecodedFrameCallback::ReceivedDecodedFrame( @@ -117,19 +107,17 @@ uint64_t VCMDecodedFrameCallback::LastReceivedPictureID() const { void VCMDecodedFrameCallback::OnDecoderImplementationName( const char* implementation_name) { - CriticalSectionScoped cs(_critSect); - if (_receiveCallback) - _receiveCallback->OnDecoderImplementationName(implementation_name); + _receiveCallback->OnDecoderImplementationName(implementation_name); } void VCMDecodedFrameCallback::Map(uint32_t timestamp, VCMFrameInformation* frameInfo) { - CriticalSectionScoped cs(_critSect); + rtc::CritScope cs(&lock_); _timestampMap.Add(timestamp, frameInfo); } int32_t VCMDecodedFrameCallback::Pop(uint32_t timestamp) { - CriticalSectionScoped cs(_critSect); + rtc::CritScope cs(&lock_); if (_timestampMap.Pop(timestamp) == NULL) { return VCM_GENERAL_ERROR; } diff --git a/webrtc/modules/video_coding/generic_decoder.h b/webrtc/modules/video_coding/generic_decoder.h index 48e25196e9..891ec893ff 100644 --- a/webrtc/modules/video_coding/generic_decoder.h +++ b/webrtc/modules/video_coding/generic_decoder.h @@ -11,9 +11,11 @@ #ifndef WEBRTC_MODULES_VIDEO_CODING_GENERIC_DECODER_H_ #define WEBRTC_MODULES_VIDEO_CODING_GENERIC_DECODER_H_ +#include "webrtc/base/criticalsection.h" +#include "webrtc/base/thread_checker.h" #include "webrtc/modules/include/module_common_types.h" -#include "webrtc/modules/video_coding/include/video_codec_interface.h" #include "webrtc/modules/video_coding/encoded_frame.h" +#include "webrtc/modules/video_coding/include/video_codec_interface.h" #include "webrtc/modules/video_coding/timestamp_map.h" #include "webrtc/modules/video_coding/timing.h" @@ -33,32 +35,38 @@ struct VCMFrameInformation { class VCMDecodedFrameCallback : public DecodedImageCallback { public: VCMDecodedFrameCallback(VCMTiming* timing, Clock* clock); - virtual ~VCMDecodedFrameCallback(); - void SetUserReceiveCallback(VCMReceiveCallback* receiveCallback); - VCMReceiveCallback* UserReceiveCallback(); + ~VCMDecodedFrameCallback() override; + void SetUserReceiveCallback(VCMReceiveCallback* receiveCallback); + VCMReceiveCallback* UserReceiveCallback(); - int32_t Decoded(VideoFrame& decodedImage) override; - int32_t Decoded(VideoFrame& decodedImage, int64_t decode_time_ms) override; - void Decoded(VideoFrame& decodedImage, - rtc::Optional decode_time_ms, - rtc::Optional qp) override; - int32_t ReceivedDecodedReferenceFrame(const uint64_t pictureId) override; - int32_t ReceivedDecodedFrame(const uint64_t pictureId) override; + int32_t Decoded(VideoFrame& decodedImage) override; + int32_t Decoded(VideoFrame& decodedImage, int64_t decode_time_ms) override; + void Decoded(VideoFrame& decodedImage, + rtc::Optional decode_time_ms, + rtc::Optional qp) override; + int32_t ReceivedDecodedReferenceFrame(const uint64_t pictureId) override; + int32_t ReceivedDecodedFrame(const uint64_t pictureId) override; - uint64_t LastReceivedPictureID() const; - void OnDecoderImplementationName(const char* implementation_name); + uint64_t LastReceivedPictureID() const; + void OnDecoderImplementationName(const char* implementation_name); - void Map(uint32_t timestamp, VCMFrameInformation* frameInfo); - int32_t Pop(uint32_t timestamp); + void Map(uint32_t timestamp, VCMFrameInformation* frameInfo); + int32_t Pop(uint32_t timestamp); private: - // Protect |_receiveCallback| and |_timestampMap|. - CriticalSectionWrapper* _critSect; - Clock* _clock; - VCMReceiveCallback* _receiveCallback GUARDED_BY(_critSect); - VCMTiming* _timing; - VCMTimestampMap _timestampMap GUARDED_BY(_critSect); - uint64_t _lastReceivedPictureID; + rtc::ThreadChecker construction_thread_; + // Protect |_timestampMap|. + Clock* const _clock; + // This callback must be set before the decoder thread starts running + // and must only be unset when external threads (e.g decoder thread) + // have been stopped. Due to that, the variable should regarded as const + // while there are more than one threads involved, it must be set + // from the same thread, and therfore a lock is not required to access it. + VCMReceiveCallback* _receiveCallback = nullptr; + VCMTiming* _timing; + rtc::CriticalSection lock_; + VCMTimestampMap _timestampMap GUARDED_BY(lock_); + uint64_t _lastReceivedPictureID; }; class VCMGenericDecoder { diff --git a/webrtc/modules/video_coding/include/mock/mock_vcm_callbacks.h b/webrtc/modules/video_coding/include/mock/mock_vcm_callbacks.h index 35362b9616..383798fa15 100644 --- a/webrtc/modules/video_coding/include/mock/mock_vcm_callbacks.h +++ b/webrtc/modules/video_coding/include/mock/mock_vcm_callbacks.h @@ -29,6 +29,17 @@ class MockPacketRequestCallback : public VCMPacketRequestCallback { int32_t(const uint16_t* sequenceNumbers, uint16_t length)); }; +class MockVCMReceiveCallback : public VCMReceiveCallback { + public: + MockVCMReceiveCallback() {} + virtual ~MockVCMReceiveCallback() {} + + MOCK_METHOD2(FrameToRender, int32_t(VideoFrame&, rtc::Optional)); + MOCK_METHOD1(ReceivedDecodedReferenceFrame, int32_t(const uint64_t)); + MOCK_METHOD1(OnIncomingPayloadType, void(int)); + MOCK_METHOD1(OnDecoderImplementationName, void(const char*)); +}; + } // namespace webrtc #endif // WEBRTC_MODULES_VIDEO_CODING_INCLUDE_MOCK_MOCK_VCM_CALLBACKS_H_ diff --git a/webrtc/modules/video_coding/test/vcm_payload_sink_factory.cc b/webrtc/modules/video_coding/test/vcm_payload_sink_factory.cc index 5604e62893..03923e6e16 100644 --- a/webrtc/modules/video_coding/test/vcm_payload_sink_factory.cc +++ b/webrtc/modules/video_coding/test/vcm_payload_sink_factory.cc @@ -10,10 +10,10 @@ #include "webrtc/modules/video_coding/test/vcm_payload_sink_factory.h" -#include - #include +#include +#include "webrtc/base/checks.h" #include "webrtc/base/constructormagic.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h" #include "webrtc/modules/video_coding/test/test_util.h" @@ -28,22 +28,21 @@ class VcmPayloadSinkFactory::VcmPayloadSink : public PayloadSinkInterface, public: VcmPayloadSink(VcmPayloadSinkFactory* factory, RtpStreamInterface* stream, - std::unique_ptr* vcm, - std::unique_ptr* frame_receiver) - : factory_(factory), stream_(stream), vcm_(), frame_receiver_() { - assert(factory); - assert(stream); - assert(vcm); - assert(vcm->get()); - assert(frame_receiver); - assert(frame_receiver->get()); - vcm_.swap(*vcm); - frame_receiver_.swap(*frame_receiver); + std::unique_ptr vcm, + std::unique_ptr frame_receiver) + : factory_(factory), + stream_(stream), + vcm_(std::move(vcm)), + frame_receiver_(std::move(frame_receiver)) { + RTC_DCHECK(factory); + RTC_DCHECK(stream); + RTC_DCHECK(vcm_); + RTC_DCHECK(frame_receiver_); vcm_->RegisterPacketRequestCallback(this); vcm_->RegisterReceiveCallback(frame_receiver_.get()); } - virtual ~VcmPayloadSink() { factory_->Remove(this); } + ~VcmPayloadSink() override { factory_->Remove(this); } // PayloadSinkInterface int32_t OnReceivedPayloadData(const uint8_t* payload_data, @@ -86,8 +85,8 @@ class VcmPayloadSinkFactory::VcmPayloadSink : public PayloadSinkInterface, } private: - VcmPayloadSinkFactory* factory_; - RtpStreamInterface* stream_; + VcmPayloadSinkFactory* const factory_; + RtpStreamInterface* const stream_; std::unique_ptr vcm_; std::unique_ptr frame_receiver_; @@ -112,17 +111,17 @@ VcmPayloadSinkFactory::VcmPayloadSinkFactory( null_event_factory_(new NullEventFactory()), crit_sect_(CriticalSectionWrapper::CreateCriticalSection()), sinks_() { - assert(clock); - assert(crit_sect_.get()); + RTC_DCHECK(clock); + RTC_DCHECK(crit_sect_.get()); } VcmPayloadSinkFactory::~VcmPayloadSinkFactory() { - assert(sinks_.empty()); + RTC_DCHECK(sinks_.empty()); } PayloadSinkInterface* VcmPayloadSinkFactory::Create( RtpStreamInterface* stream) { - assert(stream); + RTC_DCHECK(stream); CriticalSectionScoped cs(crit_sect_.get()); std::unique_ptr vcm( @@ -152,8 +151,8 @@ PayloadSinkInterface* VcmPayloadSinkFactory::Create( std::unique_ptr frame_receiver( new FileOutputFrameReceiver(base_out_filename_, stream->ssrc())); - std::unique_ptr sink( - new VcmPayloadSink(this, stream, &vcm, &frame_receiver)); + std::unique_ptr sink(new VcmPayloadSink( + this, stream, std::move(vcm), std::move(frame_receiver))); sinks_.push_back(sink.get()); return sink.release(); @@ -161,7 +160,7 @@ PayloadSinkInterface* VcmPayloadSinkFactory::Create( int VcmPayloadSinkFactory::DecodeAndProcessAll(bool decode_dual_frame) { CriticalSectionScoped cs(crit_sect_.get()); - assert(clock_); + RTC_DCHECK(clock_); bool should_decode = (clock_->TimeInMilliseconds() % 5) == 0; for (Sinks::iterator it = sinks_.begin(); it != sinks_.end(); ++it) { if ((*it)->DecodeAndProcess(should_decode, decode_dual_frame) < 0) { @@ -192,10 +191,10 @@ bool VcmPayloadSinkFactory::DecodeAll() { } void VcmPayloadSinkFactory::Remove(VcmPayloadSink* sink) { - assert(sink); + RTC_DCHECK(sink); CriticalSectionScoped cs(crit_sect_.get()); Sinks::iterator it = std::find(sinks_.begin(), sinks_.end(), sink); - assert(it != sinks_.end()); + RTC_DCHECK(it != sinks_.end()); sinks_.erase(it); } diff --git a/webrtc/modules/video_coding/video_coding_impl.cc b/webrtc/modules/video_coding/video_coding_impl.cc index c53a687965..8718d1eeb7 100644 --- a/webrtc/modules/video_coding/video_coding_impl.cc +++ b/webrtc/modules/video_coding/video_coding_impl.cc @@ -14,6 +14,7 @@ #include #include "webrtc/base/criticalsection.h" +#include "webrtc/base/thread_checker.h" #include "webrtc/common_types.h" #include "webrtc/common_video/include/video_bitrate_allocator.h" #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h" @@ -98,8 +99,8 @@ class VideoCodingModuleImpl : public VideoCodingModule { int64_t TimeUntilNextProcess() override { int64_t sender_time = sender_.TimeUntilNextProcess(); int64_t receiver_time = receiver_.TimeUntilNextProcess(); - assert(sender_time >= 0); - assert(receiver_time >= 0); + RTC_DCHECK_GE(sender_time, 0); + RTC_DCHECK_GE(receiver_time, 0); return VCM_MIN(sender_time, receiver_time); } @@ -190,6 +191,7 @@ class VideoCodingModuleImpl : public VideoCodingModule { int32_t RegisterReceiveCallback( VCMReceiveCallback* receiveCallback) override { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); return receiver_.RegisterReceiveCallback(receiveCallback); } @@ -210,6 +212,7 @@ class VideoCodingModuleImpl : public VideoCodingModule { int32_t RegisterPacketRequestCallback( VCMPacketRequestCallback* callback) override { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); return receiver_.RegisterPacketRequestCallback(callback); } @@ -277,6 +280,7 @@ class VideoCodingModuleImpl : public VideoCodingModule { void TriggerDecoderShutdown() override { receiver_.TriggerDecoderShutdown(); } private: + rtc::ThreadChecker construction_thread_; EncodedImageCallbackWrapper post_encode_callback_; vcm::VideoSender sender_; std::unique_ptr rate_allocator_; @@ -316,8 +320,8 @@ VideoCodingModule* VideoCodingModule::Create( EventFactory* event_factory, NackSender* nack_sender, KeyFrameRequestSender* keyframe_request_sender) { - assert(clock); - assert(event_factory); + RTC_DCHECK(clock); + RTC_DCHECK(event_factory); return new VideoCodingModuleImpl(clock, event_factory, nack_sender, keyframe_request_sender, nullptr); } diff --git a/webrtc/modules/video_coding/video_coding_impl.h b/webrtc/modules/video_coding/video_coding_impl.h index a45105a6ce..30641dcf2f 100644 --- a/webrtc/modules/video_coding/video_coding_impl.h +++ b/webrtc/modules/video_coding/video_coding_impl.h @@ -18,8 +18,9 @@ #include #include "webrtc/base/onetimeevent.h" -#include "webrtc/base/thread_annotations.h" #include "webrtc/base/sequenced_task_checker.h" +#include "webrtc/base/thread_annotations.h" +#include "webrtc/base/thread_checker.h" #include "webrtc/common_video/include/frame_callback.h" #include "webrtc/modules/video_coding/codec_database.h" #include "webrtc/modules/video_coding/frame_buffer.h" @@ -171,6 +172,9 @@ class VideoReceiver : public Module { int32_t Decode(const webrtc::VCMEncodedFrame* frame); + // Called on the decoder thread when thread is exiting. + void DecodingStopped(); + int32_t ReceiveCodec(VideoCodec* currentReceiveCodec) const; VideoCodecType ReceiveCodec() const; @@ -206,6 +210,7 @@ class VideoReceiver : public Module { int32_t RequestSliceLossIndication(const uint64_t pictureID) const; private: + rtc::ThreadChecker construction_thread_; Clock* const clock_; rtc::CriticalSection process_crit_; rtc::CriticalSection receive_crit_; @@ -216,7 +221,6 @@ class VideoReceiver : public Module { VCMReceiveStatisticsCallback* _receiveStatsCallback GUARDED_BY(process_crit_); VCMDecoderTimingCallback* _decoderTimingCallback GUARDED_BY(process_crit_); VCMPacketRequestCallback* _packetRequestCallback GUARDED_BY(process_crit_); - VCMGenericDecoder* _decoder; VCMFrameBuffer _frameFromFile; bool _scheduleKeyRequest GUARDED_BY(process_crit_); diff --git a/webrtc/modules/video_coding/video_coding_robustness_unittest.cc b/webrtc/modules/video_coding/video_coding_robustness_unittest.cc index 55a05720c3..4c9c830200 100644 --- a/webrtc/modules/video_coding/video_coding_robustness_unittest.cc +++ b/webrtc/modules/video_coding/video_coding_robustness_unittest.cc @@ -20,14 +20,15 @@ namespace webrtc { -using ::testing::Return; using ::testing::_; -using ::testing::ElementsAre; using ::testing::AllOf; +using ::testing::AnyNumber; using ::testing::Args; +using ::testing::ElementsAre; using ::testing::Field; -using ::testing::Pointee; using ::testing::NiceMock; +using ::testing::Pointee; +using ::testing::Return; using ::testing::Sequence; class VCMRobustnessTest : public ::testing::Test { @@ -47,6 +48,13 @@ class VCMRobustnessTest : public ::testing::Test { VideoCodingModule::Codec(kVideoCodecVP8, &video_codec_); ASSERT_EQ(VCM_OK, vcm_->RegisterReceiveCodec(&video_codec_, 1)); vcm_->RegisterExternalDecoder(&decoder_, video_codec_.plType); + + // Since we call Decode, we need to provide a valid receive callback. + // However, for the purposes of these tests, we ignore the callbacks. + EXPECT_CALL(receive_callback_, OnIncomingPayloadType(_)).Times(AnyNumber()); + EXPECT_CALL(receive_callback_, OnDecoderImplementationName(_)) + .Times(AnyNumber()); + vcm_->RegisterReceiveCallback(&receive_callback_); } virtual void TearDown() { vcm_.reset(); } @@ -72,6 +80,7 @@ class VCMRobustnessTest : public ::testing::Test { } std::unique_ptr vcm_; + MockVCMReceiveCallback receive_callback_; VideoCodec video_codec_; MockVCMFrameTypeCallback frame_type_callback_; MockPacketRequestCallback request_callback_; diff --git a/webrtc/modules/video_coding/video_receiver.cc b/webrtc/modules/video_coding/video_receiver.cc index 9dc8eaab99..e3f5040cc5 100644 --- a/webrtc/modules/video_coding/video_receiver.cc +++ b/webrtc/modules/video_coding/video_receiver.cc @@ -41,7 +41,6 @@ VideoReceiver::VideoReceiver(Clock* clock, _receiveStatsCallback(nullptr), _decoderTimingCallback(nullptr), _packetRequestCallback(nullptr), - _decoder(nullptr), _frameFromFile(), _scheduleKeyRequest(false), drop_frames_until_keyframe_(false), @@ -168,6 +167,11 @@ int32_t VideoReceiver::SetVideoProtection(VCMVideoProtection videoProtection, // ready for rendering. int32_t VideoReceiver::RegisterReceiveCallback( VCMReceiveCallback* receiveCallback) { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); + // TODO(tommi): Callback may be null, but only after the decoder thread has + // been stopped. Use the signal we now get that tells us when the decoder + // thread isn't running, to DCHECK that the method is never called while it + // is. Once we're confident, we can remove the lock. rtc::CritScope cs(&receive_crit_); _decodedFrameCallback.SetUserReceiveCallback(receiveCallback); return VCM_OK; @@ -175,6 +179,7 @@ int32_t VideoReceiver::RegisterReceiveCallback( int32_t VideoReceiver::RegisterReceiveStatisticsCallback( VCMReceiveStatisticsCallback* receiveStats) { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); rtc::CritScope cs(&process_crit_); _receiver.RegisterStatsCallback(receiveStats); _receiveStatsCallback = receiveStats; @@ -183,6 +188,7 @@ int32_t VideoReceiver::RegisterReceiveStatisticsCallback( int32_t VideoReceiver::RegisterDecoderTimingCallback( VCMDecoderTimingCallback* decoderTiming) { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); rtc::CritScope cs(&process_crit_); _decoderTimingCallback = decoderTiming; return VCM_OK; @@ -191,10 +197,11 @@ int32_t VideoReceiver::RegisterDecoderTimingCallback( // Register an externally defined decoder object. void VideoReceiver::RegisterExternalDecoder(VideoDecoder* externalDecoder, uint8_t payloadType) { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); + // TODO(tommi): This method must be called when the decoder thread is not + // running. Do we need a lock in that case? rtc::CritScope cs(&receive_crit_); if (externalDecoder == nullptr) { - // Make sure the VCM updates the decoder next time it decodes. - _decoder = nullptr; RTC_CHECK(_codecDataBase.DeregisterExternalDecoder(payloadType)); return; } @@ -217,6 +224,7 @@ int32_t VideoReceiver::RegisterPacketRequestCallback( } void VideoReceiver::TriggerDecoderShutdown() { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); _receiver.TriggerDecoderShutdown(); } @@ -225,6 +233,7 @@ void VideoReceiver::TriggerDecoderShutdown() { int32_t VideoReceiver::Decode(uint16_t maxWaitTimeMs) { bool prefer_late_decoding = false; { + // TODO(tommi): Chances are that this lock isn't required. rtc::CritScope cs(&receive_crit_); prefer_late_decoding = _codecDataBase.PrefersLateDecoding(); } @@ -292,6 +301,11 @@ int32_t VideoReceiver::Decode(const webrtc::VCMEncodedFrame* frame) { return Decode(*frame); } +void VideoReceiver::DecodingStopped() { + // No further calls to Decode() will be made after this point. + // TODO(tommi): Make use of this to clarify and check threading model. +} + int32_t VideoReceiver::RequestSliceLossIndication( const uint64_t pictureID) const { TRACE_EVENT1("webrtc", "RequestSLI", "picture_id", pictureID); @@ -327,12 +341,13 @@ int32_t VideoReceiver::RequestKeyFrame() { int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) { TRACE_EVENT0("webrtc", "VideoReceiver::Decode"); // Change decoder if payload type has changed - _decoder = _codecDataBase.GetDecoder(frame, &_decodedFrameCallback); - if (_decoder == nullptr) { + VCMGenericDecoder* decoder = + _codecDataBase.GetDecoder(frame, &_decodedFrameCallback); + if (decoder == nullptr) { return VCM_NO_CODEC_REGISTERED; } // Decode a frame - int32_t ret = _decoder->Decode(frame, clock_->TimeInMilliseconds()); + int32_t ret = decoder->Decode(frame, clock_->TimeInMilliseconds()); // Check for failed decoding, run frame type request callback if needed. bool request_key_frame = false; @@ -362,6 +377,10 @@ int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) { int32_t VideoReceiver::RegisterReceiveCodec(const VideoCodec* receiveCodec, int32_t numberOfCores, bool requireKeyFrame) { + RTC_DCHECK(construction_thread_.CalledOnValidThread()); + // TODO(tommi): This method must only be called when the decoder thread + // is not running. Do we need a lock? If not, it looks like we might not need + // a lock at all for |_codecDataBase|. rtc::CritScope cs(&receive_crit_); if (receiveCodec == nullptr) { return VCM_PARAMETER_ERROR; @@ -374,6 +393,9 @@ int32_t VideoReceiver::RegisterReceiveCodec(const VideoCodec* receiveCodec, } // Get current received codec +// TODO(tommi): See if there are any actual callers to this method. +// Neither me nor Stefan could find callers. If we can remove it, threading +// will be simpler. int32_t VideoReceiver::ReceiveCodec(VideoCodec* currentReceiveCodec) const { rtc::CritScope cs(&receive_crit_); if (currentReceiveCodec == nullptr) { @@ -383,6 +405,8 @@ int32_t VideoReceiver::ReceiveCodec(VideoCodec* currentReceiveCodec) const { } // Get current received codec +// TODO(tommi): See if there are any actual callers to this method. +// If not, it will make threading simpler. VideoCodecType VideoReceiver::ReceiveCodec() const { rtc::CritScope cs(&receive_crit_); return _codecDataBase.ReceiveCodec(); diff --git a/webrtc/modules/video_coding/video_receiver_unittest.cc b/webrtc/modules/video_coding/video_receiver_unittest.cc index 9cac45b86b..3e4d3244c6 100644 --- a/webrtc/modules/video_coding/video_receiver_unittest.cc +++ b/webrtc/modules/video_coding/video_receiver_unittest.cc @@ -21,6 +21,7 @@ #include "webrtc/test/gtest.h" using ::testing::_; +using ::testing::AnyNumber; using ::testing::NiceMock; namespace webrtc { @@ -45,6 +46,13 @@ class TestVideoReceiver : public ::testing::Test { VideoCodingModule::Codec(kVideoCodecVP8, &settings_); settings_.plType = kUnusedPayloadType; // Use the mocked encoder. EXPECT_EQ(0, receiver_->RegisterReceiveCodec(&settings_, 1, true)); + + // Since we call Decode, we need to provide a valid receive callback. + // However, for the purposes of these tests, we ignore the callbacks. + EXPECT_CALL(receive_callback_, OnIncomingPayloadType(_)).Times(AnyNumber()); + EXPECT_CALL(receive_callback_, OnDecoderImplementationName(_)) + .Times(AnyNumber()); + receiver_->RegisterReceiveCallback(&receive_callback_); } void InsertAndVerifyPaddingFrame(const uint8_t* payload, @@ -79,6 +87,7 @@ class TestVideoReceiver : public ::testing::Test { NiceMock packet_request_callback_; std::unique_ptr timing_; + MockVCMReceiveCallback receive_callback_; std::unique_ptr receiver_; }; diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index be623cef56..2b0a5d10fe 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -488,8 +488,10 @@ bool VideoReceiveStream::Decode() { video_coding::FrameBuffer::ReturnReason res = frame_buffer_->NextFrame(kMaxWaitForFrameMs, &frame); - if (res == video_coding::FrameBuffer::ReturnReason::kStopped) + if (res == video_coding::FrameBuffer::ReturnReason::kStopped) { + video_receiver_.DecodingStopped(); return false; + } if (frame) { if (video_receiver_.Decode(frame.get()) == VCM_OK) diff --git a/webrtc/video/video_stream_decoder.cc b/webrtc/video/video_stream_decoder.cc index b0308f25d8..17f29dfe2a 100644 --- a/webrtc/video/video_stream_decoder.cc +++ b/webrtc/video/video_stream_decoder.cc @@ -61,6 +61,10 @@ VideoStreamDecoder::VideoStreamDecoder( } VideoStreamDecoder::~VideoStreamDecoder() { + // Note: There's an assumption at this point that the decoder thread is + // *not* running. If it was, then there could be a race for each of these + // callbacks. + // Unset all the callback pointers that we set in the ctor. video_receiver_->RegisterPacketRequestCallback(nullptr); video_receiver_->RegisterDecoderTimingCallback(nullptr);