From 79c96ded88372967b3ca89c415e4b34d4ba391bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Fri, 19 Aug 2022 11:48:10 +0200 Subject: [PATCH] On receive stream shutdown, deregister decoders on decoder thread. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now Configure(), Decode() and Release() calls to the decoders should all happen on the decoder thread. Added thread checkers to verify. Bug: None Change-Id: I2a1cf2cf7f3c3c7c50e382d82a3638e916ed9c34 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272368 Reviewed-by: Evan Shrubsole Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/main@{#37840} --- modules/video_coding/decoder_database.cc | 8 ++++ modules/video_coding/decoder_database.h | 14 +++++-- modules/video_coding/video_receiver2.cc | 50 ++++-------------------- modules/video_coding/video_receiver2.h | 17 -------- video/video_receive_stream2.cc | 14 +++---- 5 files changed, 31 insertions(+), 72 deletions(-) diff --git a/modules/video_coding/decoder_database.cc b/modules/video_coding/decoder_database.cc index 01120dc669..882de27489 100644 --- a/modules/video_coding/decoder_database.cc +++ b/modules/video_coding/decoder_database.cc @@ -15,7 +15,12 @@ namespace webrtc { +VCMDecoderDataBase::VCMDecoderDataBase() { + decoder_sequence_checker_.Detach(); +} + bool VCMDecoderDataBase::DeregisterExternalDecoder(uint8_t payload_type) { + RTC_DCHECK_RUN_ON(&decoder_sequence_checker_); auto it = decoders_.find(payload_type); if (it == decoders_.end()) { // Not found. @@ -37,6 +42,7 @@ bool VCMDecoderDataBase::DeregisterExternalDecoder(uint8_t payload_type) { void VCMDecoderDataBase::RegisterExternalDecoder( uint8_t payload_type, VideoDecoder* external_decoder) { + RTC_DCHECK_RUN_ON(&decoder_sequence_checker_); // If payload value already exists, erase old and insert new. DeregisterExternalDecoder(payload_type); decoders_[payload_type] = external_decoder; @@ -44,6 +50,7 @@ void VCMDecoderDataBase::RegisterExternalDecoder( bool VCMDecoderDataBase::IsExternalDecoderRegistered( uint8_t payload_type) const { + RTC_DCHECK_RUN_ON(&decoder_sequence_checker_); return payload_type == current_payload_type_ || decoders_.find(payload_type) != decoders_.end(); } @@ -72,6 +79,7 @@ bool VCMDecoderDataBase::DeregisterReceiveCodec(uint8_t payload_type) { VCMGenericDecoder* VCMDecoderDataBase::GetDecoder( const VCMEncodedFrame& frame, VCMDecodedFrameCallback* decoded_frame_callback) { + RTC_DCHECK_RUN_ON(&decoder_sequence_checker_); RTC_DCHECK(decoded_frame_callback->UserReceiveCallback()); uint8_t payload_type = frame.PayloadType(); if (payload_type == current_payload_type_ || payload_type == 0) { diff --git a/modules/video_coding/decoder_database.h b/modules/video_coding/decoder_database.h index 98deb1801f..98bbf851d6 100644 --- a/modules/video_coding/decoder_database.h +++ b/modules/video_coding/decoder_database.h @@ -16,6 +16,7 @@ #include #include "absl/types/optional.h" +#include "api/sequence_checker.h" #include "api/video_codecs/video_decoder.h" #include "modules/video_coding/encoded_frame.h" #include "modules/video_coding/generic_decoder.h" @@ -24,7 +25,7 @@ namespace webrtc { class VCMDecoderDataBase { public: - VCMDecoderDataBase() = default; + VCMDecoderDataBase(); VCMDecoderDataBase(const VCMDecoderDataBase&) = delete; VCMDecoderDataBase& operator=(const VCMDecoderDataBase&) = delete; ~VCMDecoderDataBase() = default; @@ -48,14 +49,19 @@ class VCMDecoderDataBase { VCMDecodedFrameCallback* decoded_frame_callback); private: - void CreateAndInitDecoder(const VCMEncodedFrame& frame); + void CreateAndInitDecoder(const VCMEncodedFrame& frame) + RTC_RUN_ON(decoder_sequence_checker_); + + SequenceChecker decoder_sequence_checker_; absl::optional current_payload_type_; - absl::optional current_decoder_; + absl::optional current_decoder_ + RTC_GUARDED_BY(decoder_sequence_checker_); // Initialization paramaters for decoders keyed by payload type. std::map decoder_settings_; // Decoders keyed by payload type. - std::map decoders_; + std::map decoders_ + RTC_GUARDED_BY(decoder_sequence_checker_); }; } // namespace webrtc diff --git a/modules/video_coding/video_receiver2.cc b/modules/video_coding/video_receiver2.cc index 2e100209e8..fe35420c73 100644 --- a/modules/video_coding/video_receiver2.cc +++ b/modules/video_coding/video_receiver2.cc @@ -47,33 +47,22 @@ VideoReceiver2::~VideoReceiver2() { int32_t VideoReceiver2::RegisterReceiveCallback( VCMReceiveCallback* receiveCallback) { RTC_DCHECK_RUN_ON(&construction_sequence_checker_); - RTC_DCHECK(!IsDecoderThreadRunning()); // This value is set before the decoder thread starts and unset after // the decoder thread has been stopped. decodedFrameCallback_.SetUserReceiveCallback(receiveCallback); return VCM_OK; } -// Register an externally defined decoder object. This may be called on either -// the construction sequence or the decoder sequence to allow for lazy creation -// of video decoders. If called on the decoder sequence `externalDecoder` cannot -// be a nullptr. It's the responsibility of the caller to make sure that the -// access from the two sequences are mutually exclusive. void VideoReceiver2::RegisterExternalDecoder(VideoDecoder* externalDecoder, uint8_t payloadType) { - if (IsDecoderThreadRunning()) { - RTC_DCHECK_RUN_ON(&decoder_sequence_checker_); - // Don't allow deregistering decoders on the decoder thread. - RTC_DCHECK(externalDecoder != nullptr); - } else { - RTC_DCHECK_RUN_ON(&construction_sequence_checker_); - } + RTC_DCHECK_RUN_ON(&decoder_sequence_checker_); + RTC_DCHECK(decodedFrameCallback_.UserReceiveCallback()); if (externalDecoder == nullptr) { codecDataBase_.DeregisterExternalDecoder(payloadType); - return; + } else { + codecDataBase_.RegisterExternalDecoder(payloadType, externalDecoder); } - codecDataBase_.RegisterExternalDecoder(payloadType, externalDecoder); } bool VideoReceiver2::IsExternalDecoderRegistered(uint8_t payloadType) const { @@ -81,28 +70,11 @@ bool VideoReceiver2::IsExternalDecoderRegistered(uint8_t payloadType) const { return codecDataBase_.IsExternalDecoderRegistered(payloadType); } -void VideoReceiver2::DecoderThreadStarting() { - RTC_DCHECK_RUN_ON(&construction_sequence_checker_); - RTC_DCHECK(!IsDecoderThreadRunning()); -#if RTC_DCHECK_IS_ON - decoder_thread_is_running_ = true; -#endif -} - -void VideoReceiver2::DecoderThreadStopped() { - RTC_DCHECK_RUN_ON(&construction_sequence_checker_); - RTC_DCHECK(IsDecoderThreadRunning()); -#if RTC_DCHECK_IS_ON - decoder_thread_is_running_ = false; - decoder_sequence_checker_.Detach(); -#endif -} - // Must be called from inside the receive side critical section. int32_t VideoReceiver2::Decode(const VCMEncodedFrame* frame) { RTC_DCHECK_RUN_ON(&decoder_sequence_checker_); TRACE_EVENT0("webrtc", "VideoReceiver2::Decode"); - // Change decoder if payload type has changed + // Change decoder if payload type has changed. VCMGenericDecoder* decoder = codecDataBase_.GetDecoder(*frame, &decodedFrameCallback_); if (decoder == nullptr) { @@ -111,21 +83,13 @@ int32_t VideoReceiver2::Decode(const VCMEncodedFrame* frame) { return decoder->Decode(*frame, clock_->CurrentTime()); } -// Register possible receive codecs, can be called multiple times +// Register possible receive codecs, can be called multiple times. +// Called before decoder thread is started. void VideoReceiver2::RegisterReceiveCodec( uint8_t payload_type, const VideoDecoder::Settings& settings) { RTC_DCHECK_RUN_ON(&construction_sequence_checker_); - RTC_DCHECK(!IsDecoderThreadRunning()); codecDataBase_.RegisterReceiveCodec(payload_type, settings); } -bool VideoReceiver2::IsDecoderThreadRunning() { -#if RTC_DCHECK_IS_ON - return decoder_thread_is_running_; -#else - return true; -#endif -} - } // namespace webrtc diff --git a/modules/video_coding/video_receiver2.h b/modules/video_coding/video_receiver2.h index c7db2fe4e0..86f7f55096 100644 --- a/modules/video_coding/video_receiver2.h +++ b/modules/video_coding/video_receiver2.h @@ -44,20 +44,7 @@ class VideoReceiver2 { int32_t Decode(const webrtc::VCMEncodedFrame* frame); - // Notification methods that are used to check our internal state and validate - // threading assumptions. These are called by VideoReceiveStreamInterface. - // See `IsDecoderThreadRunning()` for more details. - void DecoderThreadStarting(); - void DecoderThreadStopped(); - private: - // Used for DCHECKing thread correctness. - // In build where DCHECKs are enabled, will return false before - // DecoderThreadStarting is called, then true until DecoderThreadStopped - // is called. - // In builds where DCHECKs aren't enabled, it will return true. - bool IsDecoderThreadRunning(); - SequenceChecker construction_sequence_checker_; SequenceChecker decoder_sequence_checker_; Clock* const clock_; @@ -68,10 +55,6 @@ class VideoReceiver2 { // Once the decoder thread has been started, usage of `_codecDataBase` moves // over to the decoder thread. VCMDecoderDataBase codecDataBase_; - -#if RTC_DCHECK_IS_ON - bool decoder_thread_is_running_ = false; -#endif }; } // namespace webrtc diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 97a8a54a5d..94dedaa6fc 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -386,7 +386,6 @@ void VideoReceiveStream2::Start() { call_stats_->RegisterStatsObserver(this); // Start decoding on task queue. - video_receiver_.DecoderThreadStarting(); stats_proxy_.DecoderThreadStarting(); decode_queue_.PostTask([this] { RTC_DCHECK_RUN_ON(&decode_queue_); @@ -433,20 +432,19 @@ void VideoReceiveStream2::Stop() { rtc::Event done; decode_queue_.PostTask([this, &done] { RTC_DCHECK_RUN_ON(&decode_queue_); + // Set `decoder_stopped_` before deregistering all decoders. This means + // that any pending encoded frame will return early without trying to + // access the decoder database. decoder_stopped_ = true; + for (const Decoder& decoder : config_.decoders) { + video_receiver_.RegisterExternalDecoder(nullptr, decoder.payload_type); + } done.Set(); }); done.Wait(rtc::Event::kForever); decoder_running_ = false; - video_receiver_.DecoderThreadStopped(); stats_proxy_.DecoderThreadStopped(); - // Deregister external decoders so they are no longer running during - // destruction. This effectively stops the VCM since the decoder thread is - // stopped, the VCM is deregistered and no asynchronous decoder threads are - // running. - for (const Decoder& decoder : config_.decoders) - video_receiver_.RegisterExternalDecoder(nullptr, decoder.payload_type); UpdateHistograms(); }