diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 1cb61f5a61..4f84b0247d 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -774,8 +774,10 @@ std::vector ModuleRtpRtcpImpl::BoundingSet(bool* tmmbr_owner) { } void ModuleRtpRtcpImpl::set_rtt_ms(int64_t rtt_ms) { - rtc::CritScope cs(&critical_section_rtt_); - rtt_ms_ = rtt_ms; + { + rtc::CritScope cs(&critical_section_rtt_); + rtt_ms_ = rtt_ms; + } if (rtp_sender_) { rtp_sender_->packet_history.SetRtt(rtt_ms); } diff --git a/video/BUILD.gn b/video/BUILD.gn index d7085041b6..2404281727 100644 --- a/video/BUILD.gn +++ b/video/BUILD.gn @@ -74,6 +74,7 @@ rtc_library("video") { "../api/rtc_event_log", "../api/task_queue", "../api/transport/media:media_transport_interface", + "../api/units:timestamp", "../api/video:encoded_image", "../api/video:recordable_encoded_frame", "../api/video:video_bitrate_allocation", diff --git a/video/receive_statistics_proxy2.cc b/video/receive_statistics_proxy2.cc index bf2eda086d..c16dd8a526 100644 --- a/video/receive_statistics_proxy2.cc +++ b/video/receive_statistics_proxy2.cc @@ -23,6 +23,7 @@ #include "system_wrappers/include/clock.h" #include "system_wrappers/include/field_trial.h" #include "system_wrappers/include/metrics.h" +#include "video/video_receive_stream2.h" namespace webrtc { namespace internal { @@ -500,15 +501,14 @@ void ReceiveStatisticsProxy::UpdateHistograms( videocontenttypehelpers::IsScreenshare(last_content_type_)); } -void ReceiveStatisticsProxy::QualitySample() { - RTC_DCHECK_RUN_ON(&incoming_render_queue_); +void ReceiveStatisticsProxy::QualitySample(Timestamp now) { + RTC_DCHECK_RUN_ON(&main_thread_); - int64_t now = clock_->TimeInMilliseconds(); - if (last_sample_time_ + kMinSampleLengthMs > now) + if (last_sample_time_ + kMinSampleLengthMs > now.ms()) return; double fps = - render_fps_tracker_.ComputeRateForInterval(now - last_sample_time_); + render_fps_tracker_.ComputeRateForInterval(now.ms() - last_sample_time_); absl::optional qp = qp_sample_.Avg(1); bool prev_fps_bad = !fps_threshold_.IsHigh().value_or(true); @@ -531,36 +531,37 @@ void ReceiveStatisticsProxy::QualitySample() { bool any_bad = fps_bad || qp_bad || variance_bad; if (!prev_any_bad && any_bad) { - RTC_LOG(LS_INFO) << "Bad call (any) start: " << now; + RTC_LOG(LS_INFO) << "Bad call (any) start: " << now.ms(); } else if (prev_any_bad && !any_bad) { - RTC_LOG(LS_INFO) << "Bad call (any) end: " << now; + RTC_LOG(LS_INFO) << "Bad call (any) end: " << now.ms(); } if (!prev_fps_bad && fps_bad) { - RTC_LOG(LS_INFO) << "Bad call (fps) start: " << now; + RTC_LOG(LS_INFO) << "Bad call (fps) start: " << now.ms(); } else if (prev_fps_bad && !fps_bad) { - RTC_LOG(LS_INFO) << "Bad call (fps) end: " << now; + RTC_LOG(LS_INFO) << "Bad call (fps) end: " << now.ms(); } if (!prev_qp_bad && qp_bad) { - RTC_LOG(LS_INFO) << "Bad call (qp) start: " << now; + RTC_LOG(LS_INFO) << "Bad call (qp) start: " << now.ms(); } else if (prev_qp_bad && !qp_bad) { - RTC_LOG(LS_INFO) << "Bad call (qp) end: " << now; + RTC_LOG(LS_INFO) << "Bad call (qp) end: " << now.ms(); } if (!prev_variance_bad && variance_bad) { - RTC_LOG(LS_INFO) << "Bad call (variance) start: " << now; + RTC_LOG(LS_INFO) << "Bad call (variance) start: " << now.ms(); } else if (prev_variance_bad && !variance_bad) { - RTC_LOG(LS_INFO) << "Bad call (variance) end: " << now; + RTC_LOG(LS_INFO) << "Bad call (variance) end: " << now.ms(); } - RTC_LOG(LS_VERBOSE) << "SAMPLE: sample_length: " << (now - last_sample_time_) - << " fps: " << fps << " fps_bad: " << fps_bad - << " qp: " << qp.value_or(-1) << " qp_bad: " << qp_bad + RTC_LOG(LS_VERBOSE) << "SAMPLE: sample_length: " + << (now.ms() - last_sample_time_) << " fps: " << fps + << " fps_bad: " << fps_bad << " qp: " << qp.value_or(-1) + << " qp_bad: " << qp_bad << " variance_bad: " << variance_bad << " fps_variance: " << fps_variance; - last_sample_time_ = now; + last_sample_time_ = now.ms(); qp_sample_.Reset(); if (fps_threshold_.IsHigh() || variance_threshold_.IsHigh() || @@ -807,8 +808,6 @@ void ReceiveStatisticsProxy::OnDecodedFrame(const VideoFrame& frame, // See VCMDecodedFrameCallback::Decoded for info on what thread/queue we may // be on. // RTC_DCHECK_RUN_ON(&decode_queue_); - // TODO(bugs.webrtc.org/11489): - Same as OnRenderedFrame. Both called from - // within VideoStreamDecoder::FrameToRender rtc::CritScope lock(&crit_); @@ -826,7 +825,8 @@ void ReceiveStatisticsProxy::OnDecodedFrame(const VideoFrame& frame, video_quality_observer_.reset(new VideoQualityObserver()); } - video_quality_observer_->OnDecodedFrame(frame, qp, last_codec_type_); + video_quality_observer_->OnDecodedFrame(frame.timestamp(), qp, + last_codec_type_); ContentSpecificStats* content_specific_stats = &content_specific_stats_[content_type]; @@ -874,49 +874,46 @@ void ReceiveStatisticsProxy::OnDecodedFrame(const VideoFrame& frame, last_decoded_frame_time_ms_.emplace(now_ms); } -void ReceiveStatisticsProxy::OnRenderedFrame(const VideoFrame& frame) { - // See information in OnDecodedFrame for calling context. - // TODO(bugs.webrtc.org/11489): Consider posting the work to the worker - // thread. - // - Called from VideoReceiveStream::OnFrame. +void ReceiveStatisticsProxy::OnRenderedFrame( + const VideoFrameMetaData& frame_meta) { + RTC_DCHECK_RUN_ON(&main_thread_); + // Called from VideoReceiveStream2::OnFrame. - int width = frame.width(); - int height = frame.height(); - RTC_DCHECK_GT(width, 0); - RTC_DCHECK_GT(height, 0); - int64_t now_ms = clock_->TimeInMilliseconds(); + RTC_DCHECK_GT(frame_meta.width, 0); + RTC_DCHECK_GT(frame_meta.height, 0); + + // TODO(bugs.webrtc.org/11489): Remove lock once sync isn't needed. rtc::CritScope lock(&crit_); - // TODO(bugs.webrtc.org/11489): Lose the dependency on |frame| here, just - // include the frame metadata so that this can be done asynchronously without - // blocking the decoder thread. - video_quality_observer_->OnRenderedFrame(frame, now_ms); + video_quality_observer_->OnRenderedFrame(frame_meta); ContentSpecificStats* content_specific_stats = &content_specific_stats_[last_content_type_]; - renders_fps_estimator_.Update(1, now_ms); + renders_fps_estimator_.Update(1, frame_meta.decode_timestamp.ms()); ++stats_.frames_rendered; - stats_.width = width; - stats_.height = height; + stats_.width = frame_meta.width; + stats_.height = frame_meta.height; render_fps_tracker_.AddSamples(1); - render_pixel_tracker_.AddSamples(sqrt(width * height)); - content_specific_stats->received_width.Add(width); - content_specific_stats->received_height.Add(height); + render_pixel_tracker_.AddSamples(sqrt(frame_meta.width * frame_meta.height)); + content_specific_stats->received_width.Add(frame_meta.width); + content_specific_stats->received_height.Add(frame_meta.height); // Consider taking stats_.render_delay_ms into account. - const int64_t time_until_rendering_ms = frame.render_time_ms() - now_ms; + const int64_t time_until_rendering_ms = + frame_meta.render_time_ms() - frame_meta.decode_timestamp.ms(); if (time_until_rendering_ms < 0) { sum_missed_render_deadline_ms_ += -time_until_rendering_ms; ++num_delayed_frames_rendered_; } - if (frame.ntp_time_ms() > 0) { - int64_t delay_ms = clock_->CurrentNtpInMilliseconds() - frame.ntp_time_ms(); + if (frame_meta.ntp_time_ms > 0) { + int64_t delay_ms = + clock_->CurrentNtpInMilliseconds() - frame_meta.ntp_time_ms; if (delay_ms >= 0) { content_specific_stats->e2e_delay_counter.Add(delay_ms); } } - QualitySample(); + QualitySample(frame_meta.decode_timestamp); } void ReceiveStatisticsProxy::OnSyncOffsetUpdated(int64_t video_playout_ntp_ms, diff --git a/video/receive_statistics_proxy2.h b/video/receive_statistics_proxy2.h index 86a015ecea..a8b38242fe 100644 --- a/video/receive_statistics_proxy2.h +++ b/video/receive_statistics_proxy2.h @@ -18,6 +18,7 @@ #include "absl/types/optional.h" #include "api/task_queue/task_queue_base.h" +#include "api/units/timestamp.h" #include "call/video_receive_stream.h" #include "modules/include/module_common_types.h" #include "modules/video_coding/include/video_coding_defines.h" @@ -41,6 +42,8 @@ class Clock; struct CodecSpecificInfo; namespace internal { +// Declared in video_receive_stream2.h. +struct VideoFrameMetaData; class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, public RtcpCnameCallback, @@ -61,7 +64,7 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, void OnSyncOffsetUpdated(int64_t video_playout_ntp_ms, int64_t sync_offset_ms, double estimated_freq_khz); - void OnRenderedFrame(const VideoFrame& frame); + void OnRenderedFrame(const VideoFrameMetaData& frame_meta); void OnIncomingPayloadType(int payload_type); void OnDecoderImplementationName(const char* implementation_name); @@ -130,7 +133,7 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, rtc::HistogramPercentileCounter interframe_delay_percentiles; }; - void QualitySample() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); + void QualitySample(Timestamp now) RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); // Removes info about old frames and then updates the framerate. void UpdateFramerate(int64_t now_ms) const diff --git a/video/receive_statistics_proxy2_unittest.cc b/video/receive_statistics_proxy2_unittest.cc index bcc96cd76c..7ad71dcf2f 100644 --- a/video/receive_statistics_proxy2_unittest.cc +++ b/video/receive_statistics_proxy2_unittest.cc @@ -28,6 +28,7 @@ #include "test/field_trial.h" #include "test/gtest.h" #include "test/run_loop.h" +#include "video/video_receive_stream2.h" namespace webrtc { namespace internal { @@ -63,6 +64,10 @@ class ReceiveStatisticsProxy2Test : public ::testing::Test { return CreateVideoFrame(width, height, 0); } + VideoFrame CreateFrameWithRenderTime(Timestamp render_time) { + return CreateFrameWithRenderTimeMs(render_time.ms()); + } + VideoFrame CreateFrameWithRenderTimeMs(int64_t render_time_ms) { return CreateVideoFrame(kWidth, kHeight, render_time_ms); } @@ -79,6 +84,19 @@ class ReceiveStatisticsProxy2Test : public ::testing::Test { return frame; } + // Return the current fake time as a Timestamp. + Timestamp Now() { return fake_clock_.CurrentTime(); } + + // Creates a VideoFrameMetaData instance with a timestamp. + VideoFrameMetaData MetaData(const VideoFrame& frame, Timestamp ts) { + return VideoFrameMetaData(frame, ts); + } + + // Creates a VideoFrameMetaData instance with the current fake time. + VideoFrameMetaData MetaData(const VideoFrame& frame) { + return VideoFrameMetaData(frame, Now()); + } + SimulatedClock fake_clock_; const VideoReceiveStream::Config config_; std::unique_ptr statistics_proxy_; @@ -321,12 +339,12 @@ TEST_F(ReceiveStatisticsProxy2Test, ReportsFreezeMetrics) { for (size_t i = 0; i < VideoQualityObserver::kMinFrameSamplesToDetectFreeze; ++i) { fake_clock_.AdvanceTimeMilliseconds(30); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); } // Freeze. fake_clock_.AdvanceTimeMilliseconds(kFreezeDurationMs); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); stats = statistics_proxy_->GetStats(); EXPECT_EQ(1u, stats.freeze_count); @@ -339,12 +357,12 @@ TEST_F(ReceiveStatisticsProxy2Test, ReportsPauseMetrics) { ASSERT_EQ(0u, stats.total_pauses_duration_ms); webrtc::VideoFrame frame = CreateFrame(kWidth, kHeight); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); // Pause. fake_clock_.AdvanceTimeMilliseconds(5432); statistics_proxy_->OnStreamInactive(); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); stats = statistics_proxy_->GetStats(); EXPECT_EQ(1u, stats.pause_count); @@ -361,10 +379,10 @@ TEST_F(ReceiveStatisticsProxy2Test, PauseBeforeFirstAndAfterLastFrameIgnored) { // Pause -> Frame -> Pause fake_clock_.AdvanceTimeMilliseconds(5000); statistics_proxy_->OnStreamInactive(); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(30); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(5000); statistics_proxy_->OnStreamInactive(); @@ -387,7 +405,7 @@ TEST_F(ReceiveStatisticsProxy2Test, ReportsFramesDuration) { for (int i = 0; i <= 10; ++i) { fake_clock_.AdvanceTimeMilliseconds(30); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); } stats = statistics_proxy_->GetStats(); @@ -401,7 +419,7 @@ TEST_F(ReceiveStatisticsProxy2Test, ReportsSumSquaredFrameDurations) { webrtc::VideoFrame frame = CreateFrame(kWidth, kHeight); for (int i = 0; i <= 10; ++i) { fake_clock_.AdvanceTimeMilliseconds(30); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); } stats = statistics_proxy_->GetStats(); @@ -434,7 +452,7 @@ TEST_F(ReceiveStatisticsProxy2Test, OnRenderedFrameIncreasesFramesRendered) { EXPECT_EQ(0u, statistics_proxy_->GetStats().frames_rendered); webrtc::VideoFrame frame = CreateFrame(kWidth, kHeight); for (uint32_t i = 1; i <= 3; ++i) { - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); EXPECT_EQ(i, statistics_proxy_->GetStats().frames_rendered); } } @@ -633,7 +651,7 @@ TEST_F(ReceiveStatisticsProxy2Test, BadCallHistogramsAreUpdated) { for (int i = 0; i < kNumBadSamples; ++i) { fake_clock_.AdvanceTimeMilliseconds(kBadFameIntervalMs); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); } statistics_proxy_->UpdateHistograms(absl::nullopt, counters, nullptr); EXPECT_METRIC_EQ(1, metrics::NumSamples("WebRTC.Video.BadCall.Any")); @@ -898,7 +916,7 @@ TEST_F(ReceiveStatisticsProxy2Test, DoesNotReportStaleFramerates) { frame.set_ntp_time_ms(fake_clock_.CurrentNtpInMilliseconds()); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, VideoContentType::UNSPECIFIED); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(1000 / kDefaultFps); } @@ -916,7 +934,7 @@ TEST_F(ReceiveStatisticsProxy2Test, GetStatsReportsReceivedFrameStats) { EXPECT_EQ(0, statistics_proxy_->GetStats().height); EXPECT_EQ(0u, statistics_proxy_->GetStats().frames_rendered); - statistics_proxy_->OnRenderedFrame(CreateFrame(kWidth, kHeight)); + statistics_proxy_->OnRenderedFrame(MetaData(CreateFrame(kWidth, kHeight))); EXPECT_EQ(kWidth, statistics_proxy_->GetStats().width); EXPECT_EQ(kHeight, statistics_proxy_->GetStats().height); @@ -925,8 +943,9 @@ TEST_F(ReceiveStatisticsProxy2Test, GetStatsReportsReceivedFrameStats) { TEST_F(ReceiveStatisticsProxy2Test, ReceivedFrameHistogramsAreNotUpdatedForTooFewSamples) { - for (int i = 0; i < kMinRequiredSamples - 1; ++i) - statistics_proxy_->OnRenderedFrame(CreateFrame(kWidth, kHeight)); + for (int i = 0; i < kMinRequiredSamples - 1; ++i) { + statistics_proxy_->OnRenderedFrame(MetaData(CreateFrame(kWidth, kHeight))); + } statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), nullptr); @@ -941,8 +960,9 @@ TEST_F(ReceiveStatisticsProxy2Test, } TEST_F(ReceiveStatisticsProxy2Test, ReceivedFrameHistogramsAreUpdated) { - for (int i = 0; i < kMinRequiredSamples; ++i) - statistics_proxy_->OnRenderedFrame(CreateFrame(kWidth, kHeight)); + for (int i = 0; i < kMinRequiredSamples; ++i) { + statistics_proxy_->OnRenderedFrame(MetaData(CreateFrame(kWidth, kHeight))); + } statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), nullptr); @@ -966,8 +986,8 @@ TEST_F(ReceiveStatisticsProxy2Test, ZeroDelayReportedIfFrameNotDelayed) { VideoContentType::UNSPECIFIED); // Frame not delayed, delayed frames to render: 0%. - const int64_t kNowMs = fake_clock_.TimeInMilliseconds(); - statistics_proxy_->OnRenderedFrame(CreateFrameWithRenderTimeMs(kNowMs)); + statistics_proxy_->OnRenderedFrame( + MetaData(CreateFrameWithRenderTime(Now()))); // Min run time has passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000)); @@ -988,8 +1008,8 @@ TEST_F(ReceiveStatisticsProxy2Test, VideoContentType::UNSPECIFIED); // Frame not delayed, delayed frames to render: 0%. - const int64_t kNowMs = fake_clock_.TimeInMilliseconds(); - statistics_proxy_->OnRenderedFrame(CreateFrameWithRenderTimeMs(kNowMs)); + statistics_proxy_->OnRenderedFrame( + MetaData(CreateFrameWithRenderTime(Now()))); // Min run time has not passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000) - @@ -1024,8 +1044,8 @@ TEST_F(ReceiveStatisticsProxy2Test, DelayReportedIfFrameIsDelayed) { VideoContentType::UNSPECIFIED); // Frame delayed 1 ms, delayed frames to render: 100%. - const int64_t kNowMs = fake_clock_.TimeInMilliseconds(); - statistics_proxy_->OnRenderedFrame(CreateFrameWithRenderTimeMs(kNowMs - 1)); + statistics_proxy_->OnRenderedFrame( + MetaData(CreateFrameWithRenderTimeMs(Now().ms() - 1))); // Min run time has passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000)); @@ -1048,11 +1068,16 @@ TEST_F(ReceiveStatisticsProxy2Test, AverageDelayOfDelayedFramesIsReported) { VideoContentType::UNSPECIFIED); // Two frames delayed (6 ms, 10 ms), delayed frames to render: 50%. - const int64_t kNowMs = fake_clock_.TimeInMilliseconds(); - statistics_proxy_->OnRenderedFrame(CreateFrameWithRenderTimeMs(kNowMs - 10)); - statistics_proxy_->OnRenderedFrame(CreateFrameWithRenderTimeMs(kNowMs - 6)); - statistics_proxy_->OnRenderedFrame(CreateFrameWithRenderTimeMs(kNowMs)); - statistics_proxy_->OnRenderedFrame(CreateFrameWithRenderTimeMs(kNowMs + 1)); + const int64_t kNowMs = Now().ms(); + + statistics_proxy_->OnRenderedFrame( + MetaData(CreateFrameWithRenderTimeMs(kNowMs - 10))); + statistics_proxy_->OnRenderedFrame( + MetaData(CreateFrameWithRenderTimeMs(kNowMs - 6))); + statistics_proxy_->OnRenderedFrame( + MetaData(CreateFrameWithRenderTimeMs(kNowMs))); + statistics_proxy_->OnRenderedFrame( + MetaData(CreateFrameWithRenderTimeMs(kNowMs + 1))); // Min run time has passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000)); @@ -1159,17 +1184,17 @@ TEST_P(ReceiveStatisticsProxy2TestWithFreezeDuration, FreezeDetection) { // Add a very long frame. This is need to verify that average frame // duration, which is supposed to be calculated as mean of durations of // last 30 frames, is calculated correctly. - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(2000); for (size_t i = 0; i <= VideoQualityObserver::kAvgInterframeDelaysWindowSizeFrames; ++i) { fake_clock_.AdvanceTimeMilliseconds(frame_duration_ms_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); } fake_clock_.AdvanceTimeMilliseconds(freeze_duration_ms_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); stats = statistics_proxy_->GetStats(); EXPECT_EQ(stats.freeze_count, expected_freeze_count_); @@ -1292,8 +1317,8 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, statistics_proxy_->OnStreamInactive(); fake_clock_.AdvanceTimeMilliseconds(5000); - // Insert two more frames. The interval during the pause should be disregarded - // in the stats. + // Insert two more frames. The interval during the pause should be + // disregarded in the stats. statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); @@ -1332,13 +1357,13 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, FreezesAreReported) { for (int i = 0; i < kMinRequiredSamples; ++i) { statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } // Add extra freeze. fake_clock_.AdvanceTimeMilliseconds(kFreezeDelayMs); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), nullptr); @@ -1377,20 +1402,20 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, HarmonicFrameRateIsReported) { for (int i = 0; i < kMinRequiredSamples; ++i) { fake_clock_.AdvanceTimeMilliseconds(kFrameDurationMs); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); } // Freezes and pauses should be included into harmonic frame rate. // Add freeze. fake_clock_.AdvanceTimeMilliseconds(kFreezeDurationMs); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); // Add pause. fake_clock_.AdvanceTimeMilliseconds(kPauseDurationMs); statistics_proxy_->OnStreamInactive(); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), nullptr); @@ -1420,7 +1445,7 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, PausesAreIgnored) { for (int i = 0; i <= kMinRequiredSamples; ++i) { statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } // Add a pause. @@ -1430,7 +1455,7 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, PausesAreIgnored) { // Second playback interval with triple the length. for (int i = 0; i <= kMinRequiredSamples * 3; ++i) { statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } @@ -1472,7 +1497,8 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, ManyPausesAtTheBeginning) { statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), nullptr); - // No freezes should be detected, as all long inter-frame delays were pauses. + // No freezes should be detected, as all long inter-frame delays were + // pauses. if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_METRIC_EQ(-1, metrics::MinSample( "WebRTC.Video.Screenshare.MeanFreezeDurationMs")); @@ -1491,18 +1517,18 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, TimeInHdReported) { for (int i = 0; i < kMinRequiredSamples; ++i) { statistics_proxy_->OnDecodedFrame(frame_hd, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame_hd); + statistics_proxy_->OnRenderedFrame(MetaData(frame_hd)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } // SD frames. for (int i = 0; i < 2 * kMinRequiredSamples; ++i) { statistics_proxy_->OnDecodedFrame(frame_sd, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame_sd); + statistics_proxy_->OnRenderedFrame(MetaData(frame_sd)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } // Extra last frame. - statistics_proxy_->OnRenderedFrame(frame_sd); + statistics_proxy_->OnRenderedFrame(MetaData(frame_sd)); statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), nullptr); @@ -1526,18 +1552,18 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, TimeInBlockyVideoReported) { // High quality frames. for (int i = 0; i < kMinRequiredSamples; ++i) { statistics_proxy_->OnDecodedFrame(frame, kLowQp, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } // Blocky frames. for (int i = 0; i < 2 * kMinRequiredSamples; ++i) { statistics_proxy_->OnDecodedFrame(frame, kHighQp, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } // Extra last frame. statistics_proxy_->OnDecodedFrame(frame, kHighQp, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame); + statistics_proxy_->OnRenderedFrame(MetaData(frame)); statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), nullptr); @@ -1564,15 +1590,15 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, DownscalesReported) { // Call once to pass content type. statistics_proxy_->OnDecodedFrame(frame_hd, absl::nullopt, 0, content_type_); - statistics_proxy_->OnRenderedFrame(frame_hd); + statistics_proxy_->OnRenderedFrame(MetaData(frame_hd)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); // Downscale. - statistics_proxy_->OnRenderedFrame(frame_sd); + statistics_proxy_->OnRenderedFrame(MetaData(frame_sd)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); // Downscale. - statistics_proxy_->OnRenderedFrame(frame_ld); + statistics_proxy_->OnRenderedFrame(MetaData(frame_ld)); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); statistics_proxy_->UpdateHistograms(absl::nullopt, StreamDataCounters(), @@ -1581,8 +1607,8 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, DownscalesReported) { if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_METRIC_EQ( kExpectedDownscales, - metrics::MinSample( - "WebRTC.Video.Screenshare.NumberResolutionDownswitchesPerMinute")); + metrics::MinSample("WebRTC.Video.Screenshare." + "NumberResolutionDownswitchesPerMinute")); } else { EXPECT_METRIC_EQ(kExpectedDownscales, metrics::MinSample( diff --git a/video/rtp_streams_synchronizer.h b/video/rtp_streams_synchronizer.h index 60e2c8ee32..00ef526dc5 100644 --- a/video/rtp_streams_synchronizer.h +++ b/video/rtp_streams_synchronizer.h @@ -25,6 +25,11 @@ namespace webrtc { class Syncable; +// TODO(bugs.webrtc.org/11489): Remove dependency on ProcessThread/Module. +// Instead make this a single threaded class, constructed on a TQ and +// post a 1 sec timer there. There shouldn't be a need for locking internally +// and the callback from this class, should occur on the construction TQ +// which in turn means that the callback doesn't need locking either. class RtpStreamsSynchronizer : public Module { public: explicit RtpStreamsSynchronizer(Syncable* syncable_video); diff --git a/video/video_quality_observer2.cc b/video/video_quality_observer2.cc index b1282c1ca0..0751d3f4ed 100644 --- a/video/video_quality_observer2.cc +++ b/video/video_quality_observer2.cc @@ -18,6 +18,7 @@ #include "rtc_base/logging.h" #include "rtc_base/strings/string_builder.h" #include "system_wrappers/include/metrics.h" +#include "video/video_receive_stream2.h" namespace webrtc { namespace internal { @@ -133,20 +134,22 @@ void VideoQualityObserver::UpdateHistograms(bool screenshare) { RTC_LOG(LS_INFO) << log_stream.str(); } -void VideoQualityObserver::OnRenderedFrame(const VideoFrame& frame, - int64_t now_ms) { - RTC_DCHECK_LE(last_frame_rendered_ms_, now_ms); - RTC_DCHECK_LE(last_unfreeze_time_ms_, now_ms); +void VideoQualityObserver::OnRenderedFrame( + const VideoFrameMetaData& frame_meta) { + RTC_DCHECK_LE(last_frame_rendered_ms_, frame_meta.decode_timestamp.ms()); + RTC_DCHECK_LE(last_unfreeze_time_ms_, frame_meta.decode_timestamp.ms()); if (num_frames_rendered_ == 0) { - first_frame_rendered_ms_ = last_unfreeze_time_ms_ = now_ms; + first_frame_rendered_ms_ = last_unfreeze_time_ms_ = + frame_meta.decode_timestamp.ms(); } - auto blocky_frame_it = blocky_frames_.find(frame.timestamp()); + auto blocky_frame_it = blocky_frames_.find(frame_meta.rtp_timestamp); if (num_frames_rendered_ > 0) { // Process inter-frame delay. - const int64_t interframe_delay_ms = now_ms - last_frame_rendered_ms_; + const int64_t interframe_delay_ms = + frame_meta.decode_timestamp.ms() - last_frame_rendered_ms_; const double interframe_delays_secs = interframe_delay_ms / 1000.0; // Sum of squared inter frame intervals is used to calculate the harmonic @@ -172,7 +175,7 @@ void VideoQualityObserver::OnRenderedFrame(const VideoFrame& frame, freezes_durations_.Add(interframe_delay_ms); smooth_playback_durations_.Add(last_frame_rendered_ms_ - last_unfreeze_time_ms_); - last_unfreeze_time_ms_ = now_ms; + last_unfreeze_time_ms_ = frame_meta.decode_timestamp.ms(); } else { // Count spatial metrics if there were no freeze. time_in_resolution_ms_[current_resolution_] += interframe_delay_ms; @@ -193,14 +196,15 @@ void VideoQualityObserver::OnRenderedFrame(const VideoFrame& frame, smooth_playback_durations_.Add(last_frame_rendered_ms_ - last_unfreeze_time_ms_); } - last_unfreeze_time_ms_ = now_ms; + last_unfreeze_time_ms_ = frame_meta.decode_timestamp.ms(); if (num_frames_rendered_ > 0) { - pauses_durations_.Add(now_ms - last_frame_rendered_ms_); + pauses_durations_.Add(frame_meta.decode_timestamp.ms() - + last_frame_rendered_ms_); } } - int64_t pixels = frame.width() * frame.height(); + int64_t pixels = frame_meta.width * frame_meta.height; if (pixels >= kPixelsInHighResolution) { current_resolution_ = Resolution::High; } else if (pixels >= kPixelsInMediumResolution) { @@ -214,7 +218,7 @@ void VideoQualityObserver::OnRenderedFrame(const VideoFrame& frame, } last_frame_pixels_ = pixels; - last_frame_rendered_ms_ = now_ms; + last_frame_rendered_ms_ = frame_meta.decode_timestamp.ms(); is_last_frame_blocky_ = blocky_frame_it != blocky_frames_.end(); if (is_last_frame_blocky_) { @@ -224,36 +228,37 @@ void VideoQualityObserver::OnRenderedFrame(const VideoFrame& frame, ++num_frames_rendered_; } -void VideoQualityObserver::OnDecodedFrame(const VideoFrame& frame, +void VideoQualityObserver::OnDecodedFrame(uint32_t rtp_frame_timestamp, absl::optional qp, VideoCodecType codec) { - if (qp) { - absl::optional qp_blocky_threshold; - // TODO(ilnik): add other codec types when we have QP for them. - switch (codec) { - case kVideoCodecVP8: - qp_blocky_threshold = kBlockyQpThresholdVp8; - break; - case kVideoCodecVP9: - qp_blocky_threshold = kBlockyQpThresholdVp9; - break; - default: - qp_blocky_threshold = absl::nullopt; + if (!qp) + return; + + absl::optional qp_blocky_threshold; + // TODO(ilnik): add other codec types when we have QP for them. + switch (codec) { + case kVideoCodecVP8: + qp_blocky_threshold = kBlockyQpThresholdVp8; + break; + case kVideoCodecVP9: + qp_blocky_threshold = kBlockyQpThresholdVp9; + break; + default: + qp_blocky_threshold = absl::nullopt; + } + + RTC_DCHECK(blocky_frames_.find(rtp_frame_timestamp) == blocky_frames_.end()); + + if (qp_blocky_threshold && *qp > *qp_blocky_threshold) { + // Cache blocky frame. Its duration will be calculated in render callback. + if (blocky_frames_.size() > kMaxNumCachedBlockyFrames) { + RTC_LOG(LS_WARNING) << "Overflow of blocky frames cache."; + blocky_frames_.erase( + blocky_frames_.begin(), + std::next(blocky_frames_.begin(), kMaxNumCachedBlockyFrames / 2)); } - RTC_DCHECK(blocky_frames_.find(frame.timestamp()) == blocky_frames_.end()); - - if (qp_blocky_threshold && *qp > *qp_blocky_threshold) { - // Cache blocky frame. Its duration will be calculated in render callback. - if (blocky_frames_.size() > kMaxNumCachedBlockyFrames) { - RTC_LOG(LS_WARNING) << "Overflow of blocky frames cache."; - blocky_frames_.erase( - blocky_frames_.begin(), - std::next(blocky_frames_.begin(), kMaxNumCachedBlockyFrames / 2)); - } - - blocky_frames_.insert(frame.timestamp()); - } + blocky_frames_.insert(rtp_frame_timestamp); } } diff --git a/video/video_quality_observer2.h b/video/video_quality_observer2.h index 615e0d3c57..ed5a0b9f33 100644 --- a/video/video_quality_observer2.h +++ b/video/video_quality_observer2.h @@ -19,12 +19,13 @@ #include "absl/types/optional.h" #include "api/video/video_codec_type.h" #include "api/video/video_content_type.h" -#include "api/video/video_frame.h" #include "rtc_base/numerics/moving_average.h" #include "rtc_base/numerics/sample_counter.h" namespace webrtc { namespace internal { +// Declared in video_receive_stream2.h. +struct VideoFrameMetaData; // Calculates spatial and temporal quality metrics and reports them to UMA // stats. @@ -35,11 +36,11 @@ class VideoQualityObserver { VideoQualityObserver(); ~VideoQualityObserver() = default; - void OnDecodedFrame(const VideoFrame& frame, + void OnDecodedFrame(uint32_t rtp_frame_timestamp, absl::optional qp, VideoCodecType codec); - void OnRenderedFrame(const VideoFrame& frame, int64_t now_ms); + void OnRenderedFrame(const VideoFrameMetaData& frame_meta); void OnStreamInactive(); diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 0af17d5a45..9f40c4567b 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -274,6 +274,7 @@ VideoReceiveStream2::~VideoReceiveStream2() { RTC_LOG(LS_INFO) << "~VideoReceiveStream2: " << config_.ToString(); Stop(); process_thread_->DeRegisterModule(&rtp_stream_sync_); + task_safety_flag_->SetNotAlive(); } void VideoReceiveStream2::SignalNetworkState(NetworkState state) { @@ -491,28 +492,31 @@ int VideoReceiveStream2::GetBaseMinimumPlayoutDelayMs() const { return base_minimum_playout_delay_ms_; } -// TODO(bugs.webrtc.org/11489): This method grabs a lock 6 times. void VideoReceiveStream2::OnFrame(const VideoFrame& video_frame) { - int64_t video_playout_ntp_ms; - int64_t sync_offset_ms; - double estimated_freq_khz; - // TODO(bugs.webrtc.org/11489): GetStreamSyncOffsetInMs grabs three locks. One - // inside the function itself, another in GetChannel() and a third in - // GetPlayoutTimestamp. Seems excessive. Anyhow, I'm assuming the function - // succeeds most of the time, which leads to grabbing a fourth lock. - if (rtp_stream_sync_.GetStreamSyncOffsetInMs( - video_frame.timestamp(), video_frame.render_time_ms(), - &video_playout_ntp_ms, &sync_offset_ms, &estimated_freq_khz)) { - // TODO(bugs.webrtc.org/11489): OnSyncOffsetUpdated grabs a lock. - stats_proxy_.OnSyncOffsetUpdated(video_playout_ntp_ms, sync_offset_ms, - estimated_freq_khz); - } + VideoFrameMetaData frame_meta(video_frame, clock_->CurrentTime()); + + worker_thread_->PostTask( + ToQueuedTask(task_safety_flag_, [frame_meta, this]() { + RTC_DCHECK_RUN_ON(&worker_sequence_checker_); + int64_t video_playout_ntp_ms; + int64_t sync_offset_ms; + double estimated_freq_khz; + // TODO(bugs.webrtc.org/11489): GetStreamSyncOffsetInMs grabs three + // locks. One inside the function itself, another in GetChannel() and a + // third in GetPlayoutTimestamp. Seems excessive. Anyhow, I'm assuming + // the function succeeds most of the time, which leads to grabbing a + // fourth lock. + if (rtp_stream_sync_.GetStreamSyncOffsetInMs( + frame_meta.rtp_timestamp, frame_meta.render_time_ms(), + &video_playout_ntp_ms, &sync_offset_ms, &estimated_freq_khz)) { + stats_proxy_.OnSyncOffsetUpdated(video_playout_ntp_ms, sync_offset_ms, + estimated_freq_khz); + } + stats_proxy_.OnRenderedFrame(frame_meta); + })); + source_tracker_.OnFrameDelivered(video_frame.packet_infos()); - config_.renderer->OnFrame(video_frame); - - // TODO(bugs.webrtc.org/11489): OnRenderFrame grabs a lock too. - stats_proxy_.OnRenderedFrame(video_frame); } void VideoReceiveStream2::SetFrameDecryptor( diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h index 2a0c07c879..66fbc05e91 100644 --- a/video/video_receive_stream2.h +++ b/video/video_receive_stream2.h @@ -16,6 +16,7 @@ #include "api/task_queue/task_queue_factory.h" #include "api/transport/media/media_transport_interface.h" +#include "api/units/timestamp.h" #include "api/video/recordable_encoded_frame.h" #include "call/rtp_packet_sink_interface.h" #include "call/syncable.h" @@ -45,6 +46,33 @@ class VCMTiming; namespace internal { +// Utility struct for grabbing metadata from a VideoFrame and processing it +// asynchronously without needing the actual frame data. +// Additionally the caller can bundle information from the current clock +// when the metadata is captured, for accurate reporting and not needeing +// multiple calls to clock->Now(). +struct VideoFrameMetaData { + VideoFrameMetaData(const webrtc::VideoFrame& frame, Timestamp now) + : rtp_timestamp(frame.timestamp()), + timestamp_us(frame.timestamp_us()), + ntp_time_ms(frame.ntp_time_ms()), + width(frame.width()), + height(frame.height()), + decode_timestamp(now) {} + + int64_t render_time_ms() const { + return timestamp_us / rtc::kNumMicrosecsPerMillisec; + } + + const uint32_t rtp_timestamp; + const int64_t timestamp_us; + const int64_t ntp_time_ms; + const int width; + const int height; + + const Timestamp decode_timestamp; +}; + class VideoReceiveStream2 : public webrtc::VideoReceiveStream, public rtc::VideoSinkInterface, public NackSender, @@ -225,6 +253,10 @@ class VideoReceiveStream2 : public webrtc::VideoReceiveStream, // Defined last so they are destroyed before all other members. rtc::TaskQueue decode_queue_; + + // Used to signal destruction to potentially pending tasks. + PendingTaskSafetyFlag::Pointer task_safety_flag_ = + PendingTaskSafetyFlag::Create(); }; } // namespace internal } // namespace webrtc