Detach LossNotificationController from RtpGenericFrameDescriptor
To allow to use the LossNotificationController with
an updated version of the frame descriptor extension
Bug: webrtc:10342
Change-Id: I5ac44dc5549dfcfc73bf81ad1e8eab8bd5dd136e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166166
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Elad Alon <eladalon@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30369}
diff --git a/modules/video_coding/loss_notification_controller_unittest.cc b/modules/video_coding/loss_notification_controller_unittest.cc
index 62ff889..9c4e715 100644
--- a/modules/video_coding/loss_notification_controller_unittest.cc
+++ b/modules/video_coding/loss_notification_controller_unittest.cc
@@ -10,9 +10,12 @@
#include "modules/video_coding/loss_notification_controller.h"
+#include <stdint.h>
+
#include <limits>
#include <string>
#include <tuple>
+#include <utility>
#include <vector>
#include "absl/types/optional.h"
@@ -24,7 +27,10 @@
// The information about an RTP packet that is relevant in these tests.
struct Packet {
uint16_t seq_num;
- RtpGenericFrameDescriptor descriptor;
+ bool first_in_frame;
+ bool is_keyframe;
+ int64_t frame_id;
+ std::vector<int64_t> frame_dependencies;
};
Packet CreatePacket(
@@ -33,21 +39,17 @@
uint16_t seq_num,
uint16_t frame_id,
bool is_key_frame,
- std::vector<uint16_t> ref_frame_ids = std::vector<uint16_t>()) {
- RtpGenericFrameDescriptor frame_descriptor;
- frame_descriptor.SetFirstPacketInSubFrame(first_in_frame);
- frame_descriptor.SetLastPacketInSubFrame(last_in_frame);
+ std::vector<int64_t> ref_frame_ids = std::vector<int64_t>()) {
+ Packet packet;
+ packet.seq_num = seq_num;
+ packet.first_in_frame = first_in_frame;
if (first_in_frame) {
- frame_descriptor.SetFrameId(frame_id);
- if (!is_key_frame) {
- for (uint16_t ref_frame_id : ref_frame_ids) {
- uint16_t fdiff = frame_id - ref_frame_id;
- EXPECT_TRUE(frame_descriptor.AddFrameDependencyDiff(fdiff));
- }
- }
+ packet.is_keyframe = is_key_frame;
+ packet.frame_id = frame_id;
+ RTC_DCHECK(!is_key_frame || ref_frame_ids.empty());
+ packet.frame_dependencies = std::move(ref_frame_ids);
}
-
- return Packet{seq_num, frame_descriptor};
+ return packet;
}
class PacketStreamCreator final {
@@ -55,7 +57,7 @@
PacketStreamCreator() : seq_num_(0), frame_id_(0), next_is_key_frame_(true) {}
Packet NextPacket() {
- std::vector<uint16_t> ref_frame_ids;
+ std::vector<int64_t> ref_frame_ids;
if (!next_is_key_frame_) {
ref_frame_ids.push_back(frame_id_ - 1);
}
@@ -70,7 +72,7 @@
private:
uint16_t seq_num_;
- uint16_t frame_id_;
+ int64_t frame_id_;
bool next_is_key_frame_;
};
} // namespace
@@ -112,25 +114,27 @@
EXPECT_FALSE(LastKeyFrameRequest());
EXPECT_FALSE(LastLossNotification());
- if (packet.descriptor.FirstPacketInSubFrame()) {
+ if (packet.first_in_frame) {
previous_first_packet_in_frame_ = packet;
+ LossNotificationController::FrameDetails frame;
+ frame.is_keyframe = packet.is_keyframe;
+ frame.frame_id = packet.frame_id;
+ frame.frame_dependencies = packet.frame_dependencies;
+ uut_.OnReceivedPacket(packet.seq_num, &frame);
+ } else {
+ uut_.OnReceivedPacket(packet.seq_num, nullptr);
}
-
- uut_.OnReceivedPacket(packet.seq_num, packet.descriptor);
}
void OnAssembledFrame(uint16_t first_seq_num,
- uint16_t frame_id,
+ int64_t frame_id,
bool discardable) {
EXPECT_FALSE(LastKeyFrameRequest());
EXPECT_FALSE(LastLossNotification());
ASSERT_TRUE(previous_first_packet_in_frame_);
- const RtpGenericFrameDescriptor& frame_descriptor =
- previous_first_packet_in_frame_->descriptor;
-
uut_.OnAssembledFrame(first_seq_num, frame_id, discardable,
- frame_descriptor.FrameDependenciesDiffs());
+ previous_first_packet_in_frame_->frame_dependencies);
}
void ExpectKeyFrameRequest() {
@@ -255,19 +259,6 @@
OnReceivedPacket(CreatePacket(first, last, ++seq_num, 1, false, {0}));
}
-// No key frame or loss notifications issued due to an innocuous wrap-around
-// of the frame ID.
-TEST_P(LossNotificationControllerTest, FrameIdWrapAround) {
- uint16_t frame_id = std::numeric_limits<uint16_t>::max();
- OnReceivedPacket(CreatePacket(true, true, 100, frame_id, true));
- OnAssembledFrame(100, frame_id, false);
- ++frame_id;
- const bool first = Bool<0>();
- const bool last = Bool<1>();
- OnReceivedPacket(CreatePacket(first, last, 100, frame_id, false,
- {static_cast<uint16_t>(frame_id - 1)}));
-}
-
TEST_F(LossNotificationControllerTest,
KeyFrameAfterPacketLossProducesNoLossNotifications) {
OnReceivedPacket(CreatePacket(true, true, 100, 1, true));
@@ -334,8 +325,7 @@
const auto key_frame_packet = packet_stream.NextPacket();
OnReceivedPacket(key_frame_packet);
- OnAssembledFrame(key_frame_packet.seq_num,
- key_frame_packet.descriptor.FrameId(), false);
+ OnAssembledFrame(key_frame_packet.seq_num, key_frame_packet.frame_id, false);
const bool gap = Bool<0>();
@@ -355,6 +345,27 @@
OnReceivedPacket(repeated_packet);
}
+TEST_F(LossNotificationControllerTest,
+ RecognizesDependencyAcrossIntraFrameThatIsNotAKeyframe) {
+ int last_seq_num = 1;
+ auto receive = [&](bool is_key_frame, int64_t frame_id,
+ std::vector<int64_t> ref_frame_ids) {
+ ++last_seq_num;
+ OnReceivedPacket(CreatePacket(
+ /*first_in_frame=*/true, /*last_in_frame=*/true, last_seq_num, frame_id,
+ is_key_frame, std::move(ref_frame_ids)));
+ OnAssembledFrame(last_seq_num, frame_id, /*discardable=*/false);
+ };
+ // 11 -- 13
+ // | |
+ // 10 12
+ receive(/*is_key_frame=*/true, /*frame_id=*/10, /*ref_frame_ids=*/{});
+ receive(/*is_key_frame=*/false, /*frame_id=*/11, /*ref_frame_ids=*/{10});
+ receive(/*is_key_frame=*/false, /*frame_id=*/12, /*ref_frame_ids=*/{});
+ receive(/*is_key_frame=*/false, /*frame_id=*/13, /*ref_frame_ids=*/{11, 12});
+ EXPECT_FALSE(LastLossNotification());
+}
+
class LossNotificationControllerTestDecodabilityFlag
: public LossNotificationControllerBaseTest {
protected:
@@ -376,7 +387,7 @@
void ReceivePacket(bool first_packet_in_frame,
bool last_packet_in_frame,
- const std::vector<uint16_t>& ref_frame_ids) {
+ const std::vector<int64_t>& ref_frame_ids) {
if (first_packet_in_frame) {
frame_id_ += 1;
}
@@ -397,10 +408,10 @@
// The tests intentionally never receive this, and can therefore always
// use this as an unsatisfied dependency.
- const uint16_t never_received_frame_id_ = 123;
+ const int64_t never_received_frame_id_ = 123;
uint16_t seq_num_;
- uint16_t frame_id_;
+ int64_t frame_id_;
};
TEST_F(LossNotificationControllerTestDecodabilityFlag,
@@ -408,7 +419,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {key_frame_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {key_frame_frame_id_};
ReceivePacket(true, true, ref_frame_ids);
const bool expected_decodability_flag = true;
@@ -421,7 +432,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {never_received_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {never_received_frame_id_};
ReceivePacket(true, true, ref_frame_ids);
const bool expected_decodability_flag = false;
@@ -434,7 +445,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {key_frame_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {key_frame_frame_id_};
ReceivePacket(true, false, ref_frame_ids);
const bool expected_decodability_flag = true;
@@ -447,7 +458,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {never_received_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {never_received_frame_id_};
ReceivePacket(true, false, ref_frame_ids);
const bool expected_decodability_flag = false;
@@ -460,7 +471,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {key_frame_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {key_frame_frame_id_};
ReceivePacket(false, false, ref_frame_ids);
const bool expected_decodability_flag = false;
@@ -473,7 +484,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {never_received_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {never_received_frame_id_};
ReceivePacket(false, false, ref_frame_ids);
const bool expected_decodability_flag = false;
@@ -488,7 +499,7 @@
// First packet in multi-packet frame. A loss notification is produced
// because of the gap in RTP sequence numbers.
- const std::vector<uint16_t> ref_frame_ids = {key_frame_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {key_frame_frame_id_};
ReceivePacket(true, false, ref_frame_ids);
const bool expected_decodability_flag_first = true;
ExpectLossNotification(key_frame_seq_num_, seq_num_,
@@ -510,7 +521,7 @@
// First packet in multi-packet frame. A loss notification is produced
// because of the gap in RTP sequence numbers. The frame is also recognized
// as having non-decodable dependencies.
- const std::vector<uint16_t> ref_frame_ids = {never_received_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {never_received_frame_id_};
ReceivePacket(true, false, ref_frame_ids);
const bool expected_decodability_flag_first = false;
ExpectLossNotification(key_frame_seq_num_, seq_num_,
@@ -529,7 +540,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {key_frame_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {key_frame_frame_id_};
ReceivePacket(false, true, ref_frame_ids);
const bool expected_decodability_flag = false;
@@ -542,7 +553,7 @@
ReceiveKeyFrame();
CreateGap();
- const std::vector<uint16_t> ref_frame_ids = {never_received_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {never_received_frame_id_};
ReceivePacket(false, true, ref_frame_ids);
const bool expected_decodability_flag = false;
@@ -557,7 +568,7 @@
// First packet in multi-packet frame. A loss notification is produced
// because of the gap in RTP sequence numbers.
- const std::vector<uint16_t> ref_frame_ids = {key_frame_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {key_frame_frame_id_};
ReceivePacket(true, false, ref_frame_ids);
const bool expected_decodability_flag_first = true;
ExpectLossNotification(key_frame_seq_num_, seq_num_,
@@ -579,7 +590,7 @@
// First packet in multi-packet frame. A loss notification is produced
// because of the gap in RTP sequence numbers. The frame is also recognized
// as having non-decodable dependencies.
- const std::vector<uint16_t> ref_frame_ids = {never_received_frame_id_};
+ const std::vector<int64_t> ref_frame_ids = {never_received_frame_id_};
ReceivePacket(true, false, ref_frame_ids);
const bool expected_decodability_flag_first = false;
ExpectLossNotification(key_frame_seq_num_, seq_num_,