From de7ae5755b7666d9e2790b9dbfce3f20c370a219 Mon Sep 17 00:00:00 2001 From: Evan Shrubsole Date: Wed, 17 Aug 2022 14:35:58 +0000 Subject: [PATCH] Remove statistics tracking from FrameBuffer2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was only set to nullptr in non-test environments and was thusly unused. With this change, the stats callbacks are gaurenteed to only come from the VideoStreamBufferController and so the thread checks can be removed. Bug: webrtc:14003 Change-Id: Iaf0e77aa7c45a317e38ae27739edcefd3123d832 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272021 Reviewed-by: Erik Språng Reviewed-by: Philip Eliasson Commit-Queue: Evan Shrubsole Cr-Commit-Position: refs/heads/main@{#37816} --- modules/video_coding/frame_buffer2.cc | 53 ------------------- modules/video_coding/frame_buffer2.h | 6 --- .../video_coding/frame_buffer2_unittest.cc | 35 +----------- test/fuzzers/frame_buffer2_fuzzer.cc | 2 +- video/receive_statistics_proxy2.cc | 22 -------- video/video_stream_decoder_impl.cc | 5 +- 6 files changed, 4 insertions(+), 119 deletions(-) diff --git a/modules/video_coding/frame_buffer2.cc b/modules/video_coding/frame_buffer2.cc index fbd3c53be3..9ecbf40bb8 100644 --- a/modules/video_coding/frame_buffer2.cc +++ b/modules/video_coding/frame_buffer2.cc @@ -57,7 +57,6 @@ constexpr int64_t kLogNonDecodedIntervalMs = 5000; FrameBuffer::FrameBuffer(Clock* clock, VCMTiming* timing, - VCMReceiveStatisticsCallback* stats_callback, const FieldTrialsView& field_trials) : decoded_frames_history_(kMaxFramesHistory), clock_(clock), @@ -66,7 +65,6 @@ FrameBuffer::FrameBuffer(Clock* clock, timing_(timing), stopped_(false), protection_mode_(kProtectionNack), - stats_callback_(stats_callback), last_log_non_decoded_ms_(-kLogNonDecodedIntervalMs), rtt_mult_settings_(RttMultExperiment::GetRttMultValue()), zero_playout_delay_max_decode_queue_size_( @@ -276,18 +274,6 @@ std::unique_ptr FrameBuffer::GetNextFrame() { PropagateDecodability(frame_it->second); decoded_frames_history_.InsertDecoded(frame_it->first, frame->Timestamp()); - // Remove decoded frame and all undecoded frames before it. - if (stats_callback_) { - unsigned int dropped_frames = - std::count_if(frames_.begin(), frame_it, - [](const std::pair& frame) { - return frame.second.frame != nullptr; - }); - if (dropped_frames > 0) { - stats_callback_->OnDroppedFrames(dropped_frames); - } - } - frames_.erase(frames_.begin(), ++frame_it); frames_out.emplace_back(std::move(frame)); @@ -316,9 +302,6 @@ std::unique_ptr FrameBuffer::GetNextFrame() { jitter_estimator_.FrameNacked(); } - UpdateJitterDelay(); - UpdateTimingFrameInfo(); - if (frames_out.size() == 1) { return std::move(frames_out[0]); } else { @@ -456,11 +439,6 @@ int64_t FrameBuffer::InsertFrame(std::unique_ptr frame) { // It can happen that a frame will be reported as fully received even if a // lower spatial layer frame is missing. - if (stats_callback_ && frame->is_last_spatial_layer) { - stats_callback_->OnCompleteFrame(frame->is_keyframe(), frame->size(), - frame->contentType()); - } - info->second.frame = std::move(frame); if (info->second.num_missing_continuous == 0) { @@ -591,39 +569,8 @@ bool FrameBuffer::UpdateFrameInfoWithIncomingFrame(const EncodedFrame& frame, return true; } -void FrameBuffer::UpdateJitterDelay() { - TRACE_EVENT0("webrtc", "FrameBuffer::UpdateJitterDelay"); - if (!stats_callback_) - return; - - auto timings = timing_->GetTimings(); - if (timings.num_decoded_frames > 0) { - stats_callback_->OnFrameBufferTimingsUpdated( - timings.max_decode_duration.ms(), timings.current_delay.ms(), - timings.target_delay.ms(), timings.jitter_buffer_delay.ms(), - timings.min_playout_delay.ms(), timings.render_delay.ms()); - } -} - -void FrameBuffer::UpdateTimingFrameInfo() { - TRACE_EVENT0("webrtc", "FrameBuffer::UpdateTimingFrameInfo"); - absl::optional info = timing_->GetTimingFrameInfo(); - if (info && stats_callback_) - stats_callback_->OnTimingFrameInfoUpdated(*info); -} - void FrameBuffer::ClearFramesAndHistory() { TRACE_EVENT0("webrtc", "FrameBuffer::ClearFramesAndHistory"); - if (stats_callback_) { - unsigned int dropped_frames = - std::count_if(frames_.begin(), frames_.end(), - [](const std::pair& frame) { - return frame.second.frame != nullptr; - }); - if (dropped_frames > 0) { - stats_callback_->OnDroppedFrames(dropped_frames); - } - } frames_.clear(); last_continuous_frame_.reset(); frames_to_decode_.clear(); diff --git a/modules/video_coding/frame_buffer2.h b/modules/video_coding/frame_buffer2.h index 13918b0e18..1383c40ae3 100644 --- a/modules/video_coding/frame_buffer2.h +++ b/modules/video_coding/frame_buffer2.h @@ -48,7 +48,6 @@ class FrameBuffer { public: FrameBuffer(Clock* clock, VCMTiming* timing, - VCMReceiveStatisticsCallback* stats_callback, const FieldTrialsView& field_trials); FrameBuffer() = delete; @@ -143,10 +142,6 @@ class FrameBuffer { FrameMap::iterator info) RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); - void UpdateJitterDelay() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); - - void UpdateTimingFrameInfo() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); - void ClearFramesAndHistory() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); // The cleaner solution would be to have the NextFrame function return a @@ -179,7 +174,6 @@ class FrameBuffer { std::vector frames_to_decode_ RTC_GUARDED_BY(mutex_); bool stopped_ RTC_GUARDED_BY(mutex_); VCMVideoProtection protection_mode_ RTC_GUARDED_BY(mutex_); - VCMReceiveStatisticsCallback* const stats_callback_; int64_t last_log_non_decoded_ms_ RTC_GUARDED_BY(mutex_); // rtt_mult experiment settings. diff --git a/modules/video_coding/frame_buffer2_unittest.cc b/modules/video_coding/frame_buffer2_unittest.cc index 13ad66c5c3..0fabd9b496 100644 --- a/modules/video_coding/frame_buffer2_unittest.cc +++ b/modules/video_coding/frame_buffer2_unittest.cc @@ -137,7 +137,6 @@ class TestFrameBuffer2 : public ::testing::Test { timing_(time_controller_.GetClock(), field_trials_), buffer_(new FrameBuffer(time_controller_.GetClock(), &timing_, - &stats_callback_, field_trials_)), rand_(0x34678213) {} @@ -225,7 +224,6 @@ class TestFrameBuffer2 : public ::testing::Test { std::unique_ptr buffer_; std::vector> frames_; Random rand_; - ::testing::NiceMock stats_callback_; }; // From https://en.cppreference.com/w/cpp/language/static: "If ... a constexpr @@ -284,8 +282,8 @@ TEST_F(TestFrameBuffer2, OneSuperFrame) { TEST_F(TestFrameBuffer2, ZeroPlayoutDelay) { test::ScopedKeyValueConfig field_trials; VCMTiming timing(time_controller_.GetClock(), field_trials); - buffer_.reset(new FrameBuffer(time_controller_.GetClock(), &timing, - &stats_callback_, field_trials)); + buffer_ = std::make_unique(time_controller_.GetClock(), &timing, + field_trials); const VideoPlayoutDelay kPlayoutDelayMs = {0, 0}; std::unique_ptr test_frame(new FrameObjectFake()); test_frame->SetId(0); @@ -379,8 +377,6 @@ TEST_F(TestFrameBuffer2, DropTemporalLayerSlowDecoder) { pid + i - 1); } - EXPECT_CALL(stats_callback_, OnDroppedFrames(1)).Times(3); - for (int i = 0; i < 10; ++i) { ExtractFrame(); time_controller_.AdvanceTime(TimeDelta::Millis(70)); @@ -411,7 +407,6 @@ TEST_F(TestFrameBuffer2, DropFramesIfSystemIsStalled) { // Jump forward in time, simulating the system being stalled for some reason. time_controller_.AdvanceTime(TimeDelta::Millis(3) * kFps10); // Extract one more frame, expect second and third frame to be dropped. - EXPECT_CALL(stats_callback_, OnDroppedFrames(2)).Times(1); ExtractFrame(); CheckFrame(0, pid + 0, 0); @@ -428,7 +423,6 @@ TEST_F(TestFrameBuffer2, DroppedFramesCountedOnClear) { } // All frames should be dropped when Clear is called. - EXPECT_CALL(stats_callback_, OnDroppedFrames(5)).Times(1); buffer_->Clear(); } @@ -520,31 +514,6 @@ TEST_F(TestFrameBuffer2, PictureIdJumpBack) { CheckNoFrame(2); } -TEST_F(TestFrameBuffer2, StatsCallback) { - uint16_t pid = Rand(); - uint32_t ts = Rand(); - const int kFrameSize = 5000; - - EXPECT_CALL(stats_callback_, - OnCompleteFrame(true, kFrameSize, VideoContentType::UNSPECIFIED)); - EXPECT_CALL(stats_callback_, OnFrameBufferTimingsUpdated(_, _, _, _, _, _)); - // Stats callback requires a previously decoded frame. - timing_.StopDecodeTimer(TimeDelta::Millis(1), Timestamp::Zero()); - - { - std::unique_ptr frame(new FrameObjectFake()); - frame->SetEncodedData(EncodedImageBuffer::Create(kFrameSize)); - frame->SetId(pid); - frame->SetTimestamp(ts); - frame->num_references = 0; - - EXPECT_EQ(buffer_->InsertFrame(std::move(frame)), pid); - } - - ExtractFrame(); - CheckFrame(0, pid, 0); -} - TEST_F(TestFrameBuffer2, ForwardJumps) { EXPECT_EQ(5453, InsertFrame(5453, 0, 1, true, kFrameSize)); ExtractFrame(); diff --git a/test/fuzzers/frame_buffer2_fuzzer.cc b/test/fuzzers/frame_buffer2_fuzzer.cc index 944f0699d9..ec1bbbb4c1 100644 --- a/test/fuzzers/frame_buffer2_fuzzer.cc +++ b/test/fuzzers/frame_buffer2_fuzzer.cc @@ -75,7 +75,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) { test::ScopedKeyValueConfig field_trials; VCMTiming timing(time_controller.GetClock(), field_trials); video_coding::FrameBuffer frame_buffer(time_controller.GetClock(), &timing, - nullptr, field_trials); + field_trials); bool next_frame_task_running = false; diff --git a/video/receive_statistics_proxy2.cc b/video/receive_statistics_proxy2.cc index 3448ab5b71..01e4f09e12 100644 --- a/video/receive_statistics_proxy2.cc +++ b/video/receive_statistics_proxy2.cc @@ -664,20 +664,6 @@ void ReceiveStatisticsProxy::OnFrameBufferTimingsUpdated( int jitter_buffer_ms, int min_playout_delay_ms, int render_delay_ms) { - // Only called on main_thread_ with FrameBuffer3 - if (!worker_thread_->IsCurrent()) { - RTC_DCHECK_RUN_ON(&decode_queue_); - worker_thread_->PostTask(SafeTask( - task_safety_.flag(), - [max_decode_ms, current_delay_ms, target_delay_ms, jitter_buffer_ms, - min_playout_delay_ms, render_delay_ms, this]() { - OnFrameBufferTimingsUpdated(max_decode_ms, current_delay_ms, - target_delay_ms, jitter_buffer_ms, - min_playout_delay_ms, render_delay_ms); - })); - return; - } - RTC_DCHECK_RUN_ON(&main_thread_); stats_.max_decode_ms = max_decode_ms; stats_.current_delay_ms = current_delay_ms; @@ -700,14 +686,6 @@ void ReceiveStatisticsProxy::OnUniqueFramesCounted(int num_unique_frames) { void ReceiveStatisticsProxy::OnTimingFrameInfoUpdated( const TimingFrameInfo& info) { - // Only called on main_thread_ with FrameBuffer3 - if (!worker_thread_->IsCurrent()) { - RTC_DCHECK_RUN_ON(&decode_queue_); - worker_thread_->PostTask(SafeTask(task_safety_.flag(), [info, this]() { - OnTimingFrameInfoUpdated(info); - })); - return; - } RTC_DCHECK_RUN_ON(&main_thread_); if (info.flags != VideoSendTiming::kInvalid) { int64_t now_ms = clock_->TimeInMilliseconds(); diff --git a/video/video_stream_decoder_impl.cc b/video/video_stream_decoder_impl.cc index fdc79eb229..516aceb680 100644 --- a/video/video_stream_decoder_impl.cc +++ b/video/video_stream_decoder_impl.cc @@ -33,10 +33,7 @@ VideoStreamDecoderImpl::VideoStreamDecoderImpl( decoder_factory_(decoder_factory), decoder_settings_(std::move(decoder_settings)), shut_down_(false), - frame_buffer_(Clock::GetRealTimeClock(), - &timing_, - nullptr, - *field_trials_), + frame_buffer_(Clock::GetRealTimeClock(), &timing_, *field_trials_), bookkeeping_queue_(task_queue_factory->CreateTaskQueue( "video_stream_decoder_bookkeeping_queue", TaskQueueFactory::Priority::NORMAL)),