Refactor FrameInstrumentationGenerator to handle missing VideoFrames. If a raw `VideoFrame` is not available to when trying to generate frame instrumentation data, still update the state with a sync messages. This is especially important on keyframe where we've state that frame instrumentation data must be available. Otherwise we may end up in a state where we're unable to produce instrumentation at all for a layer. Bug: webrtc:358039777 Change-Id: Id64e139ddcfa2bf4211aa624ec701fe6dcdcda7f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/488400 Commit-Queue: Erik Språng <sprang@webrtc.org> Reviewed-by: Sergey Silkin <ssilkin@webrtc.org> Cr-Commit-Position: refs/heads/main@{#48180}
diff --git a/video/corruption_detection/frame_instrumentation_generator_impl.cc b/video/corruption_detection/frame_instrumentation_generator_impl.cc index 40bbfca..420bcfa 100644 --- a/video/corruption_detection/frame_instrumentation_generator_impl.cc +++ b/video/corruption_detection/frame_instrumentation_generator_impl.cc
@@ -135,16 +135,14 @@ captured_frames_.front().rtp_timestamp())) { captured_frames_.pop(); } - if (captured_frames_.empty() || captured_frames_.front().rtp_timestamp() != - rtp_timestamp_encoded_image) { - RTC_LOG(LS_VERBOSE) << "No captured frames for RTC timestamp " - << rtp_timestamp_encoded_image << "."; - return std::nullopt; - } - captured_frame = captured_frames_.front(); - - if (encoded_image.is_end_of_temporal_unit()) { - captured_frames_.pop(); + const bool has_captured_frame = + !captured_frames_.empty() && + captured_frames_.front().rtp_timestamp() == rtp_timestamp_encoded_image; + if (has_captured_frame) { + captured_frame = captured_frames_.front(); + if (encoded_image.is_end_of_temporal_unit()) { + captured_frames_.pop(); + } } layer_id = GetSpatialLayerId(encoded_image); @@ -195,31 +193,34 @@ RTC_CHECK(data.SetSequenceIndex(sequence_index)); - // TODO: bugs.webrtc.org/358039777 - Maybe allow other sample sizes as well + bool should_instrument = false; if (frame_selector_) { - if (frame_selector_->ShouldInstrumentFrame(*captured_frame, - encoded_image)) { - sample_coordinates = - contexts_[layer_id].frame_sampler.GetSampleCoordinatesForFrame( - /*num_samples=*/13); - } + should_instrument = + frame_selector_->ShouldInstrumentFrame(captured_frame, encoded_image); } else { - sample_coordinates = - contexts_[layer_id] - .frame_sampler.GetSampleCoordinatesForFrameIfFrameShouldBeSampled( - is_key_frame, captured_frame->rtp_timestamp(), - /*num_samples=*/13); + should_instrument = contexts_[layer_id].frame_sampler.ShouldSampleFrame( + is_key_frame, encoded_image.RtpTimestamp()); } - if (sample_coordinates.empty()) { + if (!should_instrument) { if (!is_key_frame) { return std::nullopt; } // Sync message only. return data; } + + if (!captured_frame.has_value()) { + RTC_LOG(LS_VERBOSE) << "No captured frames for RTC timestamp " + << rtp_timestamp_encoded_image << "."; + // Sync message only. + return data; + } + + sample_coordinates = + contexts_[layer_id].frame_sampler.GetSampleCoordinatesForFrame( + /*num_samples=*/13); } - RTC_DCHECK(captured_frame.has_value()); RTC_DCHECK(!sample_coordinates.empty()); std::optional<CorruptionDetectionFilterSettings> filter_settings =
diff --git a/video/corruption_detection/frame_instrumentation_generator_unittest.cc b/video/corruption_detection/frame_instrumentation_generator_unittest.cc index d60176f..1cf7228 100644 --- a/video/corruption_detection/frame_instrumentation_generator_unittest.cc +++ b/video/corruption_detection/frame_instrumentation_generator_unittest.cc
@@ -99,6 +99,100 @@ } TEST(FrameInstrumentationGeneratorTest, + ReturnsSyncMessageForKeyFrameWhenNoCapturedFrameProvided) { + const Environment env = CreateTestEnvironment(); + FrameInstrumentationGeneratorImpl generator( + &env, VideoCodecType::kVideoCodecVP8, ScalabilityMode::kL1T1); + + EncodedImage encoded_image; + encoded_image.SetRtpTimestamp(1); + encoded_image.set_frame_type(VideoFrameType::kVideoFrameKey); + + std::optional<FrameInstrumentationData> data = + generator.OnEncodedImage(encoded_image); + ASSERT_TRUE(data.has_value()); + EXPECT_TRUE(data->is_sync_only()); + EXPECT_EQ(data->sequence_index(), 0); +} + +TEST(FrameInstrumentationGeneratorTest, + EstablishesContextWithSyncMessageWhenCapturedFrameIsMissingOnKeyFrame) { + const Environment env = CreateTestEnvironment(); + FrameInstrumentationGeneratorImpl generator( + &env, VideoCodecType::kVideoCodecVP8, ScalabilityMode::kL1T1); + + // 1. Send KeyFrame without captured frame -> returns sync message. + EncodedImage key_image; + key_image.SetRtpTimestamp(1); + key_image.set_frame_type(VideoFrameType::kVideoFrameKey); + std::optional<FrameInstrumentationData> sync_data = + generator.OnEncodedImage(key_image); + ASSERT_TRUE(sync_data.has_value()); + EXPECT_TRUE(sync_data->is_sync_only()); + + // 2. Send DeltaFrame WITH captured frame -> should succeed. + VideoFrame frame = VideoFrame::Builder() + .set_video_frame_buffer(MakeDefaultI420FrameBuffer()) + .set_rtp_timestamp(90002) + .build(); + generator.OnCapturedFrame(frame); + + EncodedImage delta_image; + delta_image.SetRtpTimestamp(90002); + delta_image.set_frame_type(VideoFrameType::kVideoFrameDelta); + delta_image.qp_ = 10; + delta_image._encodedWidth = kDefaultScaledWidth; + delta_image._encodedHeight = kDefaultScaledHeight; + + std::optional<FrameInstrumentationData> delta_data = + generator.OnEncodedImage(delta_image); + EXPECT_TRUE(delta_data.has_value()); +} + +TEST(FrameInstrumentationGeneratorTest, + ReturnsSyncMessageForDeltaFrameWhenNoRawFrameButShouldBeInstrumented) { + const Environment env = CreateTestEnvironment(); + FrameInstrumentationGeneratorImpl generator( + &env, VideoCodecType::kVideoCodecVP8, ScalabilityMode::kL1T1); + + // 1. Send KeyFrame with captured frame (to establish context and set last + // sampled timestamp to 1). + VideoFrame key_frame = + VideoFrame::Builder() + .set_video_frame_buffer(MakeDefaultI420FrameBuffer()) + .set_rtp_timestamp(1) + .build(); + generator.OnCapturedFrame(key_frame); + + EncodedImage key_image; + key_image.SetRtpTimestamp(1); + key_image.set_frame_type(VideoFrameType::kVideoFrameKey); + key_image.qp_ = 10; + key_image._encodedWidth = kDefaultScaledWidth; + key_image._encodedHeight = kDefaultScaledHeight; + + std::optional<FrameInstrumentationData> key_data = + generator.OnEncodedImage(key_image); + ASSERT_TRUE(key_data.has_value()); + EXPECT_FALSE(key_data->is_sync_only()); + + // 2. Send DeltaFrame WITHOUT captured frame, but with timestamp 90001 + // (EnoughTimeHasPassed is true, should be instrumented). + EncodedImage delta_image; + delta_image.SetRtpTimestamp(90001); + delta_image.set_frame_type(VideoFrameType::kVideoFrameDelta); + delta_image.qp_ = 10; + delta_image._encodedWidth = kDefaultScaledWidth; + delta_image._encodedHeight = kDefaultScaledHeight; + + std::optional<FrameInstrumentationData> delta_data = + generator.OnEncodedImage(delta_image); + ASSERT_TRUE(delta_data.has_value()); + EXPECT_TRUE(delta_data->is_sync_only()); + EXPECT_EQ(delta_data->sequence_index(), 13); +} + +TEST(FrameInstrumentationGeneratorTest, ReturnsNothingWhenTheFirstFrameOfASpatialOrSimulcastLayerIsNotAKeyFrame) { const Environment env = CreateTestEnvironment(); FrameInstrumentationGeneratorImpl generator(
diff --git a/video/corruption_detection/frame_selector.cc b/video/corruption_detection/frame_selector.cc index eceab9d..e7ea425 100644 --- a/video/corruption_detection/frame_selector.cc +++ b/video/corruption_detection/frame_selector.cc
@@ -12,6 +12,7 @@ #include <algorithm> #include <cstdint> +#include <optional> #include "api/environment/environment.h" #include "api/units/time_delta.h" @@ -59,6 +60,12 @@ bool FrameSelector::ShouldInstrumentFrame(const VideoFrame& raw_frame, const EncodedImage& encoded_frame) { + return ShouldInstrumentFrame(std::make_optional(raw_frame), encoded_frame); +} + +bool FrameSelector::ShouldInstrumentFrame( + const std::optional<VideoFrame>& raw_frame, + const EncodedImage& encoded_frame) { int layer_id = std::max(encoded_frame.SpatialIndex().value_or(0), encoded_frame.SimulcastIndex().value_or(0)); if (encoded_frame.IsKey()) { @@ -81,8 +88,11 @@ Timestamp::Millis(encoded_frame.RtpTimestamp() / kVideoRtpTicksPerMs); } - bool is_low_overhead = - CanNativelyHandleFormat(raw_frame.video_frame_buffer()->type()); + bool is_low_overhead = true; + if (raw_frame.has_value()) { + is_low_overhead = + CanNativelyHandleFormat(raw_frame->video_frame_buffer()->type()); + } const Timespan& span = is_low_overhead ? low_overhead_frame_span_ : high_overhead_frame_span_;
diff --git a/video/corruption_detection/frame_selector.h b/video/corruption_detection/frame_selector.h index 92107d2..4321858 100644 --- a/video/corruption_detection/frame_selector.h +++ b/video/corruption_detection/frame_selector.h
@@ -12,6 +12,7 @@ #define VIDEO_CORRUPTION_DETECTION_FRAME_SELECTOR_H_ #include <map> +#include <optional> #include "api/environment/environment.h" #include "api/units/time_delta.h" @@ -54,6 +55,8 @@ bool ShouldInstrumentFrame(const VideoFrame& raw_frame, const EncodedImage& encoded_frame); + bool ShouldInstrumentFrame(const std::optional<VideoFrame>& raw_frame, + const EncodedImage& encoded_frame); private: const InterLayerPredMode inter_layer_pred_mode_;
diff --git a/video/corruption_detection/halton_frame_sampler.cc b/video/corruption_detection/halton_frame_sampler.cc index 4efadcf..413525d 100644 --- a/video/corruption_detection/halton_frame_sampler.cc +++ b/video/corruption_detection/halton_frame_sampler.cc
@@ -49,14 +49,8 @@ HaltonFrameSampler::HaltonFrameSampler() : coordinate_sampler_prng_(HaltonSequence(2)) {} -std::vector<HaltonFrameSampler::Coordinates> -HaltonFrameSampler::GetSampleCoordinatesForFrameIfFrameShouldBeSampled( - bool is_key_frame, - uint32_t rtp_timestamp, - int num_samples) { - if (num_samples < 1) { - return {}; - } +bool HaltonFrameSampler::ShouldSampleFrame(bool is_key_frame, + uint32_t rtp_timestamp) { if (rtp_timestamp_last_frame_sampled_.has_value()) { RTC_CHECK_NE(*rtp_timestamp_last_frame_sampled_, rtp_timestamp); } @@ -67,9 +61,23 @@ (kMaxFramesBetweenSamples - 1) - (frames_sampled_ % 8); ++frames_sampled_; rtp_timestamp_last_frame_sampled_ = rtp_timestamp; - return GetSampleCoordinatesForFrame(num_samples); + return true; } --frames_until_next_sample_; + return false; +} + +std::vector<HaltonFrameSampler::Coordinates> +HaltonFrameSampler::GetSampleCoordinatesForFrameIfFrameShouldBeSampled( + bool is_key_frame, + uint32_t rtp_timestamp, + int num_samples) { + if (num_samples < 1) { + return {}; + } + if (ShouldSampleFrame(is_key_frame, rtp_timestamp)) { + return GetSampleCoordinatesForFrame(num_samples); + } return {}; }
diff --git a/video/corruption_detection/halton_frame_sampler.h b/video/corruption_detection/halton_frame_sampler.h index 0026ee7..d3f2963 100644 --- a/video/corruption_detection/halton_frame_sampler.h +++ b/video/corruption_detection/halton_frame_sampler.h
@@ -45,6 +45,7 @@ HaltonFrameSampler& operator=(const HaltonFrameSampler&) = default; HaltonFrameSampler& operator=(HaltonFrameSampler&&) = default; + bool ShouldSampleFrame(bool is_key_frame, uint32_t rtp_timestamp); std::vector<Coordinates> GetSampleCoordinatesForFrameIfFrameShouldBeSampled( bool is_key_frame, uint32_t rtp_timestamp,
diff --git a/video/rtp_video_stream_receiver2_unittest.cc b/video/rtp_video_stream_receiver2_unittest.cc index d7f50ec..f723719 100644 --- a/video/rtp_video_stream_receiver2_unittest.cc +++ b/video/rtp_video_stream_receiver2_unittest.cc
@@ -559,6 +559,57 @@ } TEST_F(RtpVideoStreamReceiver2Test, + FrameInstrumentationDataGetsPopulatedForSyncMessage) { + const std::vector<uint8_t> kKeyFramePayload = {0, 1, 2, 3, 4}; + + // Prepare the receiver for VP9. + CodecParameterMap codec_params; + rtp_video_stream_receiver_->AddReceiveCodec(kVp9PayloadType, kVideoCodecVP9, + codec_params, + /*raw_payload=*/false); + + ReceivedPacketGenerator received_packet_generator; + // Create a sync-only message. + CorruptionDetectionMessage::Builder builder; + builder.WithSequenceIndex(0).WithInterpretSequenceIndexAsMostSignificantBits( + true); + std::optional<CorruptionDetectionMessage> corruption_detection_msg = + builder.Build(); + ASSERT_TRUE(corruption_detection_msg.has_value()); + received_packet_generator.SetCorruptionDetectionHeader( + *corruption_detection_msg); + + // Generate key frame packets. + received_packet_generator.SetPayload(kKeyFramePayload, + VideoFrameType::kVideoFrameKey); + // Have corruption header on the key frame. + RtpPacketReceived key_frame_packet = + received_packet_generator.NextPacket(/*include_corruption_header=*/true); + + rtp_video_stream_receiver_->StartReceive(); + mock_on_complete_frame_callback_.AppendExpectedBitstream( + kKeyFramePayload.data(), kKeyFramePayload.size()); + + EXPECT_TRUE(key_frame_packet.GetExtension<CorruptionDetectionExtension>()); + std::unique_ptr<EncodedFrame> key_encoded_frame; + EXPECT_CALL(mock_on_complete_frame_callback_, DoOnCompleteFrame(_)) + .WillOnce([&](EncodedFrame* encoded_frame) { + key_encoded_frame = std::make_unique<EncodedFrame>(*encoded_frame); + }); + rtp_video_stream_receiver_->OnRtpPacket(key_frame_packet); + ASSERT_TRUE(key_encoded_frame != nullptr); + std::optional<FrameInstrumentationData> frame_inst_data_key_frame = + key_encoded_frame->CodecSpecific()->frame_instrumentation_data; + ASSERT_TRUE(frame_inst_data_key_frame.has_value()); + EXPECT_EQ(frame_inst_data_key_frame->sequence_index(), 0); + EXPECT_TRUE(frame_inst_data_key_frame->is_sync_only()); + EXPECT_EQ(frame_inst_data_key_frame->std_dev(), 0.0); + EXPECT_EQ(frame_inst_data_key_frame->luma_error_threshold(), 0); + EXPECT_EQ(frame_inst_data_key_frame->chroma_error_threshold(), 0); + EXPECT_TRUE(frame_inst_data_key_frame->sample_values().empty()); +} + +TEST_F(RtpVideoStreamReceiver2Test, FrameInstrumentationDataGetsPopulatedMSBIncreasedCorrectly) { const std::vector<uint8_t> kKeyFramePayload = {0, 1, 2, 3, 4}; const std::vector<uint8_t> kDeltaFramePayload = {5, 6, 7, 8, 9};