diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 38f2d10001..807d63dab7 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -77,6 +77,7 @@ const char kNoMid[] = ""; using ::testing::_; using ::testing::AllOf; +using ::testing::AtLeast; using ::testing::Contains; using ::testing::Each; using ::testing::ElementsAreArray; @@ -2818,6 +2819,53 @@ TEST_P(RtpSenderTest, DoesntFecProtectRetransmissions) { EXPECT_GT(rtp_sender()->ReSendPacket(kSeqNum), 0); } +TEST_P(RtpSenderTest, MarksPacketsWithKeyframeStatus) { + FieldTrialBasedConfig field_trials; + RTPSenderVideo::Config video_config; + video_config.clock = clock_; + video_config.rtp_sender = rtp_sender(); + video_config.field_trials = &field_trials; + RTPSenderVideo rtp_sender_video(video_config); + + const uint8_t kPayloadType = 127; + const absl::optional kCodecType = + VideoCodecType::kVideoCodecGeneric; + + const uint32_t kCaptureTimeMsToRtpTimestamp = 90; // 90 kHz clock + + { + EXPECT_CALL(mock_paced_sender_, + EnqueuePackets(Each( + Pointee(Property(&RtpPacketToSend::is_key_frame, true))))) + .Times(AtLeast(1)); + RTPVideoHeader video_header; + video_header.frame_type = VideoFrameType::kVideoFrameKey; + int64_t capture_time_ms = clock_->TimeInMilliseconds(); + EXPECT_TRUE(rtp_sender_video.SendVideo( + kPayloadType, kCodecType, + capture_time_ms * kCaptureTimeMsToRtpTimestamp, capture_time_ms, + kPayloadData, video_header, kDefaultExpectedRetransmissionTimeMs)); + + time_controller_.AdvanceTime(TimeDelta::Millis(33)); + } + + { + EXPECT_CALL(mock_paced_sender_, + EnqueuePackets(Each( + Pointee(Property(&RtpPacketToSend::is_key_frame, false))))) + .Times(AtLeast(1)); + RTPVideoHeader video_header; + video_header.frame_type = VideoFrameType::kVideoFrameDelta; + int64_t capture_time_ms = clock_->TimeInMilliseconds(); + EXPECT_TRUE(rtp_sender_video.SendVideo( + kPayloadType, kCodecType, + capture_time_ms * kCaptureTimeMsToRtpTimestamp, capture_time_ms, + kPayloadData, video_header, kDefaultExpectedRetransmissionTimeMs)); + + time_controller_.AdvanceTime(TimeDelta::Millis(33)); + } +} + INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderTest, ::testing::Values(TestConfig{false}, diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 7a75973fa4..2499d35ccd 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -649,6 +649,8 @@ bool RTPSenderVideo::SendVideo( return false; packet->set_allow_retransmission(allow_retransmission); + packet->set_is_key_frame(video_header.frame_type == + VideoFrameType::kVideoFrameKey); // Put packetization finish timestamp into extension. if (packet->HasExtension()) { diff --git a/modules/rtp_rtcp/source/ulpfec_generator.cc b/modules/rtp_rtcp/source/ulpfec_generator.cc index 76d1bb5d87..4873693164 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator.cc +++ b/modules/rtp_rtcp/source/ulpfec_generator.cc @@ -77,7 +77,7 @@ UlpfecGenerator::UlpfecGenerator(int red_payload_type, fec_(ForwardErrorCorrection::CreateUlpfec(kUnknownSsrc)), num_protected_frames_(0), min_num_media_packets_(1), - keyframe_in_process_(false), + media_contains_keyframe_(false), fec_bitrate_(/*max_window_size_ms=*/1000, RateStatistics::kBpsScale) {} // Used by FlexFecSender, payload types are unused. @@ -89,7 +89,7 @@ UlpfecGenerator::UlpfecGenerator(std::unique_ptr fec, fec_(std::move(fec)), num_protected_frames_(0), min_num_media_packets_(1), - keyframe_in_process_(false), + media_contains_keyframe_(false), fec_bitrate_(/*max_window_size_ms=*/1000, RateStatistics::kBpsScale) {} UlpfecGenerator::~UlpfecGenerator() = default; @@ -111,7 +111,7 @@ void UlpfecGenerator::AddPacketAndGenerateFec(const RtpPacketToSend& packet) { RTC_DCHECK_RUNS_SERIALIZED(&race_checker_); RTC_DCHECK(generated_fec_packets_.empty()); - if (media_packets_.empty()) { + { MutexLock lock(&mutex_); if (pending_params_) { current_params_ = *pending_params_; @@ -123,13 +123,12 @@ void UlpfecGenerator::AddPacketAndGenerateFec(const RtpPacketToSend& packet) { min_num_media_packets_ = 1; } } - - keyframe_in_process_ = packet.is_key_frame(); } - RTC_DCHECK_EQ(packet.is_key_frame(), keyframe_in_process_); - bool complete_frame = false; - const bool marker_bit = packet.Marker(); + if (packet.is_key_frame()) { + media_contains_keyframe_ = true; + } + const bool complete_frame = packet.Marker(); if (media_packets_.size() < kUlpfecMaxMediaPackets) { // Our packet masks can only protect up to |kUlpfecMaxMediaPackets| packets. auto fec_packet = std::make_unique(); @@ -142,9 +141,8 @@ void UlpfecGenerator::AddPacketAndGenerateFec(const RtpPacketToSend& packet) { last_media_packet_ = packet; } - if (marker_bit) { + if (complete_frame) { ++num_protected_frames_; - complete_frame = true; } auto params = CurrentParams(); @@ -154,7 +152,7 @@ void UlpfecGenerator::AddPacketAndGenerateFec(const RtpPacketToSend& packet) { // less than |kMaxExcessOverhead|, and // (2) at least |min_num_media_packets_| media packets is reached. if (complete_frame && - (num_protected_frames_ == params.max_fec_frames || + (num_protected_frames_ >= params.max_fec_frames || (ExcessOverheadBelowMax() && MinimumMediaPacketsReached()))) { // We are not using Unequal Protection feature of the parity erasure code. constexpr int kNumImportantPackets = 0; @@ -190,8 +188,8 @@ bool UlpfecGenerator::MinimumMediaPacketsReached() const { const FecProtectionParams& UlpfecGenerator::CurrentParams() const { RTC_DCHECK_RUNS_SERIALIZED(&race_checker_); - return keyframe_in_process_ ? current_params_.keyframe_params - : current_params_.delta_params; + return media_contains_keyframe_ ? current_params_.keyframe_params + : current_params_.delta_params; } size_t UlpfecGenerator::MaxPacketOverhead() const { @@ -265,6 +263,7 @@ void UlpfecGenerator::ResetState() { last_media_packet_.reset(); generated_fec_packets_.clear(); num_protected_frames_ = 0; + media_contains_keyframe_ = false; } } // namespace webrtc diff --git a/modules/rtp_rtcp/source/ulpfec_generator.h b/modules/rtp_rtcp/source/ulpfec_generator.h index 32ddc6c4b9..934a1d5c38 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator.h +++ b/modules/rtp_rtcp/source/ulpfec_generator.h @@ -59,6 +59,9 @@ class UlpfecGenerator : public VideoFecGenerator { absl::optional GetRtpState() override { return absl::nullopt; } + // Currently used protection params. + const FecProtectionParams& CurrentParams() const; + private: struct Params { Params(); @@ -90,8 +93,6 @@ class UlpfecGenerator : public VideoFecGenerator { // (e.g. (2k,2m) vs (k,m)) are generally more effective at recovering losses. bool MinimumMediaPacketsReached() const; - const FecProtectionParams& CurrentParams() const; - void ResetState(); const int red_payload_type_; @@ -110,7 +111,7 @@ class UlpfecGenerator : public VideoFecGenerator { int num_protected_frames_ RTC_GUARDED_BY(race_checker_); int min_num_media_packets_ RTC_GUARDED_BY(race_checker_); Params current_params_ RTC_GUARDED_BY(race_checker_); - bool keyframe_in_process_ RTC_GUARDED_BY(race_checker_); + bool media_contains_keyframe_ RTC_GUARDED_BY(race_checker_); mutable Mutex mutex_; absl::optional pending_params_ RTC_GUARDED_BY(mutex_); diff --git a/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc b/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc index db005ddb49..c07e81d4fc 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc +++ b/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc @@ -217,4 +217,57 @@ TEST_F(UlpfecGeneratorTest, MixedMediaRtpHeaderLengths) { } } +TEST_F(UlpfecGeneratorTest, UpdatesProtectionParameters) { + const FecProtectionParams kKeyFrameParams = {25, /*max_fec_frames=*/2, + kFecMaskRandom}; + const FecProtectionParams kDeltaFrameParams = {25, /*max_fec_frames=*/5, + kFecMaskRandom}; + + ulpfec_generator_.SetProtectionParameters(kDeltaFrameParams, kKeyFrameParams); + + // No params applied yet. + EXPECT_EQ(ulpfec_generator_.CurrentParams().max_fec_frames, 0); + + // Helper function to add a single-packet frame market as either key-frame + // or delta-frame. + auto add_frame = [&](bool is_keyframe) { + packet_generator_.NewFrame(1); + std::unique_ptr packet = + packet_generator_.NextPacket(0, 10); + RtpPacketToSend rtp_packet(nullptr); + EXPECT_TRUE(rtp_packet.Parse(packet->data.data(), packet->data.size())); + rtp_packet.set_is_key_frame(is_keyframe); + ulpfec_generator_.AddPacketAndGenerateFec(rtp_packet); + }; + + // Add key-frame, keyframe params should apply, no FEC generated yet. + add_frame(true); + EXPECT_EQ(ulpfec_generator_.CurrentParams().max_fec_frames, 2); + EXPECT_TRUE(ulpfec_generator_.GetFecPackets().empty()); + + // Add delta-frame, generated FEC packet. Params will not be updated until + // next added packet though. + add_frame(false); + EXPECT_EQ(ulpfec_generator_.CurrentParams().max_fec_frames, 2); + EXPECT_FALSE(ulpfec_generator_.GetFecPackets().empty()); + + // Add delta-frame, now params get updated. + add_frame(false); + EXPECT_EQ(ulpfec_generator_.CurrentParams().max_fec_frames, 5); + EXPECT_TRUE(ulpfec_generator_.GetFecPackets().empty()); + + // Add yet another delta-frame. + add_frame(false); + EXPECT_EQ(ulpfec_generator_.CurrentParams().max_fec_frames, 5); + EXPECT_TRUE(ulpfec_generator_.GetFecPackets().empty()); + + // Add key-frame, params immediately switch to key-frame ones. The two + // buffered frames plus the key-frame is protected and fec emitted, + // even though the frame count is technically over the keyframe frame count + // threshold. + add_frame(true); + EXPECT_EQ(ulpfec_generator_.CurrentParams().max_fec_frames, 2); + EXPECT_FALSE(ulpfec_generator_.GetFecPackets().empty()); +} + } // namespace webrtc