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);