diff --git a/video/corruption_detection/frame_instrumentation_generator.cc b/video/corruption_detection/frame_instrumentation_generator.cc index cc2b1122e4..df6fe865a5 100644 --- a/video/corruption_detection/frame_instrumentation_generator.cc +++ b/video/corruption_detection/frame_instrumentation_generator.cc @@ -36,6 +36,10 @@ namespace webrtc { namespace { +// Avoid holding on to frames that might have been dropped by encoder, as that +// can lead to frame buffer pools draining. +constexpr size_t kMaxPendingFrames = 3; + std::optional GetCorruptionFilterSettings( const EncodedImage& encoded_image, VideoCodecType video_codec_type, @@ -75,6 +79,9 @@ FrameInstrumentationGenerator::FrameInstrumentationGenerator( : video_codec_type_(video_codec_type) {} void FrameInstrumentationGenerator::OnCapturedFrame(VideoFrame frame) { + while (captured_frames_.size() >= kMaxPendingFrames) { + captured_frames_.pop(); + } captured_frames_.push(frame); } diff --git a/video/corruption_detection/frame_instrumentation_generator_unittest.cc b/video/corruption_detection/frame_instrumentation_generator_unittest.cc index 2305127699..7f8992be51 100644 --- a/video/corruption_detection/frame_instrumentation_generator_unittest.cc +++ b/video/corruption_detection/frame_instrumentation_generator_unittest.cc @@ -11,6 +11,7 @@ #include "video/corruption_detection/frame_instrumentation_generator.h" #include +#include #include #include @@ -686,5 +687,40 @@ TEST(FrameInstrumentationGeneratorTest, GetterAndSetterOperatesAsExpected) { #endif // GTEST_HAS_DEATH_TEST } +TEST(FrameInstrumentationGeneratorTest, QueuesAtMostThreeInputFrames) { + auto generator = std::make_unique( + VideoCodecType::kVideoCodecVP8); + + bool frames_destroyed[4] = {}; + class TestBuffer : public webrtc::I420Buffer { + public: + TestBuffer(int width, int height, bool* frame_destroyed_indicator) + : I420Buffer(width, height), + frame_destroyed_indicator_(frame_destroyed_indicator) {} + + private: + friend class RefCountedObject; + ~TestBuffer() override { *frame_destroyed_indicator_ = true; } + + bool* frame_destroyed_indicator_; + }; + + // Insert four frames, the first one should expire and be released. + for (int i = 0; i < 4; ++i) { + generator->OnCapturedFrame( + VideoFrame::Builder() + .set_video_frame_buffer(rtc::make_ref_counted( + kDefaultScaledWidth, kDefaultScaledHeight, + &frames_destroyed[i])) + .set_rtp_timestamp(1 + (33 * i)) + .build()); + } + + EXPECT_THAT(frames_destroyed, ElementsAre(true, false, false, false)); + + generator.reset(); + EXPECT_THAT(frames_destroyed, ElementsAre(true, true, true, true)); +} + } // namespace } // namespace webrtc diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index e2037575fa..c62a3e1c90 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -1560,9 +1560,6 @@ void VideoStreamEncoder::OnFrame(Timestamp post_time, encoder_stats_observer_->OnIncomingFrame(incoming_frame.width(), incoming_frame.height()); - if (frame_instrumentation_generator_) { - frame_instrumentation_generator_->OnCapturedFrame(incoming_frame); - } ++captured_frame_count_; bool cwnd_frame_drop = cwnd_frame_drop_interval_ && @@ -2032,6 +2029,10 @@ void VideoStreamEncoder::EncodeVideoFrame(const VideoFrame& video_frame, frame_encode_metadata_writer_.OnEncodeStarted(out_frame); + if (frame_instrumentation_generator_) { + frame_instrumentation_generator_->OnCapturedFrame(out_frame); + } + const int32_t encode_status = encoder_->Encode(out_frame, &next_frame_types_); was_encode_called_since_last_initialization_ = true; diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 859c7927be..08ae5cae95 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -1702,6 +1702,61 @@ TEST_F(VideoStreamEncoderTest, PopulatesFrameInstrumentationDataWhenSetTo) { video_stream_encoder_->Stop(); } +TEST_F(VideoStreamEncoderTest, + FrameInstrumentationGeneratorDoesNotStashDroppedFrames) { + // Set low rate but high resolution. Make sure input frame is dropped and + // instance is released, even with corruption detection enabled. + const DataRate kLowRate = DataRate::KilobitsPerSec(300); + codec_width_ = 1280; + codec_height_ = 720; + + video_send_config_.encoder_settings.enable_frame_instrumentation_generator = + true; + ConfigureEncoder(video_encoder_config_.Copy()); + video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( + kLowRate, kLowRate, kLowRate, 0, 0, 0); + + rtc::Event frame_destroyed_event; + // Insert two frames, so that the first one isn't stored in the encoder queue. + video_source_.IncomingCapturedFrame(CreateFrame(1, &frame_destroyed_event)); + video_source_.IncomingCapturedFrame(CreateFrame(34, /*event=*/nullptr)); + EXPECT_TRUE(frame_destroyed_event.Wait(kDefaultTimeout)); + + EXPECT_FALSE(sink_.GetLastFrameInstrumentationData().has_value()); + video_stream_encoder_->Stop(); +} + +TEST_F(VideoStreamEncoderTest, + FrameInstrumentationGeneratorHandlesQueuedFrames) { + video_send_config_.encoder_settings.enable_frame_instrumentation_generator = + true; + ConfigureEncoder(video_encoder_config_.Copy()); + + // Mark stream as suspended. + video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( + DataRate::Zero(), DataRate::Zero(), DataRate::Zero(), 0, 0, 0); + video_stream_encoder_->WaitUntilTaskQueueIsIdle(); + + // We need a QP for the encoded frame. + fake_encoder_.SetEncodedImageData(EncodedImageBuffer::Create( + kCodedFrameVp8Qp25, sizeof(kCodedFrameVp8Qp25))); + + // Insert a frame, that should be treated as dropped due to suspended state. + video_source_.IncomingCapturedFrame( + CreateFrame(1, codec_width_, codec_height_)); + + ExpectDroppedFrame(); + + // Resume and increase bitrate budget, process stashed frames. + video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( + kTargetBitrate, kTargetBitrate, kTargetBitrate, 0, 0, 0); + + WaitForEncodedFrame(1); + EXPECT_TRUE(sink_.GetLastFrameInstrumentationData().has_value()); + + video_stream_encoder_->Stop(); +} + TEST_F(VideoStreamEncoderTest, DoesNotPopulateFrameInstrumentationDataWhenSetNotTo) { video_send_config_.encoder_settings.enable_frame_instrumentation_generator =