Update how FEC handles protection parameters for key vs delta frames.
This CL:
1) Updates RtpSenderVideo to actually populate the is_key_frame field
properly.
2) Updates UlpfecGenerator to:
* Allow updating the protection parameters before adding any packet.
* Apply keyframe protection parameter when at least one buffered
media packet to be protected belongs to a keyframe.
Updating the parameters in the middle of a frame is allowed, at that
point they only determine how many _complete_ frames are needed in order
to trigger FEC generation. Only that requirement is met, will the
protection parameters (e.g. FEC rate and mask type) actually be applied.
This means that delta-frames adjecent to a key-frame (either ahead of
or after) may be protected in the same way as the key-frame itself.
Bug: webrtc:11340
Change-Id: Ieb84d0ae46de01c17b4ef72251a4cb37814569da
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/195620
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Ying Wang <yinwa@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32787}
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index 38f2d10..807d63d 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -77,6 +77,7 @@
using ::testing::_;
using ::testing::AllOf;
+using ::testing::AtLeast;
using ::testing::Contains;
using ::testing::Each;
using ::testing::ElementsAreArray;
@@ -2818,6 +2819,53 @@
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<VideoCodecType> 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 7a75973..2499d35 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -649,6 +649,8 @@
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<VideoTimingExtension>()) {
diff --git a/modules/rtp_rtcp/source/ulpfec_generator.cc b/modules/rtp_rtcp/source/ulpfec_generator.cc
index 76d1bb5..4873693 100644
--- a/modules/rtp_rtcp/source/ulpfec_generator.cc
+++ b/modules/rtp_rtcp/source/ulpfec_generator.cc
@@ -77,7 +77,7 @@
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 @@
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 @@
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 @@
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<ForwardErrorCorrection::Packet>();
@@ -142,9 +141,8 @@
last_media_packet_ = packet;
}
- if (marker_bit) {
+ if (complete_frame) {
++num_protected_frames_;
- complete_frame = true;
}
auto params = CurrentParams();
@@ -154,7 +152,7 @@
// 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 @@
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 @@
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 32ddc6c..934a1d5 100644
--- a/modules/rtp_rtcp/source/ulpfec_generator.h
+++ b/modules/rtp_rtcp/source/ulpfec_generator.h
@@ -59,6 +59,9 @@
absl::optional<RtpState> GetRtpState() override { return absl::nullopt; }
+ // Currently used protection params.
+ const FecProtectionParams& CurrentParams() const;
+
private:
struct Params {
Params();
@@ -90,8 +93,6 @@
// (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 @@
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<Params> 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 db005dd..c07e81d 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, 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<AugmentedPacket> 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
