From 132e28e6aada4f90f30a4f0319b0f1d43d800f98 Mon Sep 17 00:00:00 2001 From: Tommi Date: Sat, 24 Feb 2018 17:57:33 +0100 Subject: [PATCH] Add thread checks to ReceiveStatisticsProxy that reflect design comments. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I4dd2b0be006440500cc7744de0e952263531ccd2 Bug: webrtc:8929 Reviewed-on: https://webrtc-review.googlesource.com/54940 Reviewed-by: Åsa Persson Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#22179} --- video/receive_statistics_proxy.cc | 24 ++++++++++++++++++++++++ video/receive_statistics_proxy.h | 11 ++++++++++- video/video_receive_stream.cc | 2 ++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/video/receive_statistics_proxy.cc b/video/receive_statistics_proxy.cc index ffb925d977..7aed47064a 100644 --- a/video/receive_statistics_proxy.cc +++ b/video/receive_statistics_proxy.cc @@ -113,6 +113,8 @@ ReceiveStatisticsProxy::ReceiveStatisticsProxy( avg_rtt_ms_(0), last_content_type_(VideoContentType::UNSPECIFIED), timing_frame_info_counter_(kMovingMaxWindowMs) { + decode_thread_.DetachFromThread(); + network_thread_.DetachFromThread(); stats_.ssrc = config_.rtp.remote_ssrc; // TODO(brandtr): Replace |rtx_stats_| with a single instance of // StreamDataCounters. @@ -122,10 +124,19 @@ ReceiveStatisticsProxy::ReceiveStatisticsProxy( } ReceiveStatisticsProxy::~ReceiveStatisticsProxy() { + RTC_DCHECK_RUN_ON(&main_thread_); + // In case you're reading this wondering "hmm... we're on the main thread but + // calling a method that needs to be called on the decoder thread...", then + // here's what's going on: + // - The decoder thread has been stopped and DecoderThreadStopped() has been + // called. + // - The decode_thread_ thread checker has been detached, and will now become + // attached to the current thread, which is OK since we're in the dtor. UpdateHistograms(); } void ReceiveStatisticsProxy::UpdateHistograms() { + RTC_DCHECK_RUN_ON(&decode_thread_); std::ostringstream logStream; int stream_duration_sec = (clock_->TimeInMilliseconds() - start_ms_) / 1000; if (stats_.frame_counts.key_frames > 0 || @@ -450,6 +461,8 @@ void ReceiveStatisticsProxy::UpdateHistograms() { } void ReceiveStatisticsProxy::QualitySample() { + RTC_DCHECK_RUN_ON(&network_thread_); + int64_t now = clock_->TimeInMilliseconds(); if (last_sample_time_ + kMinSampleLengthMs > now) return; @@ -559,6 +572,7 @@ void ReceiveStatisticsProxy::OnDecoderImplementationName( } void ReceiveStatisticsProxy::OnIncomingRate(unsigned int framerate, unsigned int bitrate_bps) { + RTC_DCHECK_RUN_ON(&network_thread_); rtc::CritScope lock(&crit_); if (stats_.rtp_stats.first_packet_time_ms != -1) QualitySample(); @@ -783,6 +797,7 @@ void ReceiveStatisticsProxy::OnDiscardedPacketsUpdated(int discarded_packets) { void ReceiveStatisticsProxy::OnPreDecode( const EncodedImage& encoded_image, const CodecSpecificInfo* codec_specific_info) { + RTC_DCHECK_RUN_ON(&decode_thread_); if (!codec_specific_info || encoded_image.qp_ == -1) { return; } @@ -839,6 +854,15 @@ void ReceiveStatisticsProxy::OnRttUpdate(int64_t avg_rtt_ms, avg_rtt_ms_ = avg_rtt_ms; } +void ReceiveStatisticsProxy::DecoderThreadStarting() { + RTC_DCHECK_RUN_ON(&main_thread_); +} + +void ReceiveStatisticsProxy::DecoderThreadStopped() { + RTC_DCHECK_RUN_ON(&main_thread_); + decode_thread_.DetachFromThread(); +} + ReceiveStatisticsProxy::ContentSpecificStats::ContentSpecificStats() : interframe_delay_percentiles(kMaxCommonInterframeDelayMs) {} diff --git a/video/receive_statistics_proxy.h b/video/receive_statistics_proxy.h index 836bb4b62f..2fa4e084e8 100644 --- a/video/receive_statistics_proxy.h +++ b/video/receive_statistics_proxy.h @@ -26,6 +26,7 @@ #include "rtc_base/rate_statistics.h" #include "rtc_base/ratetracker.h" #include "rtc_base/thread_annotations.h" +#include "rtc_base/thread_checker.h" #include "video/quality_threshold.h" #include "video/report_block_stats.h" #include "video/stats_counter.h" @@ -98,6 +99,11 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, // Implements CallStatsObserver. void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; + // Notification methods that are used to check our internal state and validate + // threading assumptions. These are called by VideoReceiveStream. + void DecoderThreadStarting(); + void DecoderThreadStopped(); + private: struct SampleCounter { SampleCounter() : sum(0), num_samples(0) {} @@ -179,7 +185,7 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, MaxCounter freq_offset_counter_ RTC_GUARDED_BY(crit_); int64_t first_report_block_time_ms_ RTC_GUARDED_BY(crit_); ReportBlockStats report_block_stats_ RTC_GUARDED_BY(crit_); - QpCounters qp_counters_; // Only accessed on the decoding thread. + QpCounters qp_counters_ RTC_GUARDED_BY(decode_thread_); std::map rtx_stats_ RTC_GUARDED_BY(crit_); int64_t avg_rtt_ms_ RTC_GUARDED_BY(crit_); mutable std::map frame_window_ RTC_GUARDED_BY(&crit_); @@ -191,6 +197,9 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, mutable rtc::MovingMaxCounter timing_frame_info_counter_ RTC_GUARDED_BY(&crit_); rtc::Optional num_unique_frames_ RTC_GUARDED_BY(crit_); + rtc::ThreadChecker decode_thread_; + rtc::ThreadChecker network_thread_; + rtc::ThreadChecker main_thread_; }; } // namespace webrtc diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index 549fb11638..055aa20afd 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -227,6 +227,7 @@ void VideoReceiveStream::Start() { // Start the decode thread video_receiver_.DecoderThreadStarting(); + stats_proxy_.DecoderThreadStarting(); decode_thread_.Start(); rtp_video_stream_receiver_.StartReceive(); } @@ -251,6 +252,7 @@ void VideoReceiveStream::Stop() { decode_thread_.Stop(); 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