Remove packets from RtpPacketHistory if acked via TransportFeedback
If the receiver has indicated that a packet has been received, via a
TransportFeedback RTCP message, it is safe to remove it from the
RtpPacketHistory as we can be sure it won't be needed anymore.
This will reduce memory usage, reduce the risk of overflow in the
history at very high bitrates, and hopefully make payload based padding
a little more useful.
This is code stems partly from
https://webrtc-review.googlesource.com/c/src/+/134208
but without the RtpPacketHistory changes which were landed in
https://webrtc-review.googlesource.com/c/src/+/134307
Bug: webrtc:8975
Change-Id: Iea9d3d32bee5512473744e9ef3a18018567fc272
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/135160
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27868}
diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc
index 9416173..d0fe7b3 100644
--- a/call/rtp_video_sender_unittest.cc
+++ b/call/rtp_video_sender_unittest.cc
@@ -15,8 +15,13 @@
#include "api/task_queue/default_task_queue_factory.h"
#include "call/rtp_transport_controller_send.h"
#include "call/rtp_video_sender.h"
+#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
+#include "modules/rtp_rtcp/source/byte_io.h"
+#include "modules/rtp_rtcp/source/rtcp_packet/nack.h"
+#include "modules/rtp_rtcp/source/rtp_packet.h"
#include "modules/video_coding/fec_controller_default.h"
#include "modules/video_coding/include/video_codec_interface.h"
+#include "rtc_base/event.h"
#include "rtc_base/rate_limiter.h"
#include "test/field_trial.h"
#include "test/gmock.h"
@@ -27,6 +32,7 @@
#include "video/send_statistics_proxy.h"
using ::testing::_;
+using ::testing::Invoke;
using ::testing::NiceMock;
using ::testing::SaveArg;
using ::testing::Unused;
@@ -36,6 +42,8 @@
const int8_t kPayloadType = 96;
const uint32_t kSsrc1 = 12345;
const uint32_t kSsrc2 = 23456;
+const uint32_t kRtxSsrc1 = 34567;
+const uint32_t kRtxSsrc2 = 45678;
const int16_t kInitialPictureId1 = 222;
const int16_t kInitialPictureId2 = 44;
const int16_t kInitialTl0PicIdx1 = 99;
@@ -60,6 +68,7 @@
RtpSenderObservers observers;
observers.rtcp_rtt_stats = rtcp_rtt_stats;
observers.intra_frame_callback = intra_frame_callback;
+ observers.rtcp_loss_notification_observer = nullptr;
observers.rtcp_stats = rtcp_stats;
observers.rtp_stats = rtp_stats;
observers.bitrate_observer = bitrate_observer;
@@ -70,16 +79,43 @@
return observers;
}
+BitrateConstraints GetBitrateConfig() {
+ BitrateConstraints bitrate_config;
+ bitrate_config.min_bitrate_bps = 30000;
+ bitrate_config.start_bitrate_bps = 300000;
+ bitrate_config.max_bitrate_bps = 3000000;
+ return bitrate_config;
+}
+
+VideoSendStream::Config CreateVideoSendStreamConfig(
+ Transport* transport,
+ const std::vector<uint32_t>& ssrcs,
+ const std::vector<uint32_t>& rtx_ssrcs,
+ int payload_type) {
+ VideoSendStream::Config config(transport);
+ config.rtp.ssrcs = ssrcs;
+ config.rtp.rtx.ssrcs = rtx_ssrcs;
+ config.rtp.payload_type = payload_type;
+ config.rtp.rtx.payload_type = payload_type + 1;
+ config.rtp.nack.rtp_history_ms = 1000;
+ return config;
+}
+
class RtpVideoSenderTestFixture {
public:
RtpVideoSenderTestFixture(
const std::vector<uint32_t>& ssrcs,
+ const std::vector<uint32_t>& rtx_ssrcs,
int payload_type,
const std::map<uint32_t, RtpPayloadState>& suspended_payload_states,
FrameCountObserver* frame_count_observer)
: clock_(1000000),
- config_(&transport_),
+ config_(CreateVideoSendStreamConfig(&transport_,
+ ssrcs,
+ rtx_ssrcs,
+ payload_type)),
send_delay_stats_(&clock_),
+ bitrate_config_(GetBitrateConfig()),
task_queue_factory_(CreateDefaultTaskQueueFactory()),
transport_controller_(&clock_,
&event_log_,
@@ -94,10 +130,6 @@
config_,
VideoEncoderConfig::ContentType::kRealtimeVideo),
retransmission_rate_limiter_(&clock_, kRetransmitWindowSizeMs) {
- for (uint32_t ssrc : ssrcs) {
- config_.rtp.ssrcs.push_back(ssrc);
- }
- config_.rtp.payload_type = payload_type;
std::map<uint32_t, RtpState> suspended_ssrcs;
router_ = absl::make_unique<RtpVideoSender>(
&clock_, suspended_ssrcs, suspended_payload_states, config_.rtp,
@@ -111,14 +143,18 @@
}
RtpVideoSenderTestFixture(
const std::vector<uint32_t>& ssrcs,
+ const std::vector<uint32_t>& rtx_ssrcs,
int payload_type,
const std::map<uint32_t, RtpPayloadState>& suspended_payload_states)
: RtpVideoSenderTestFixture(ssrcs,
+ rtx_ssrcs,
payload_type,
suspended_payload_states,
/*frame_count_observer=*/nullptr) {}
RtpVideoSender* router() { return router_.get(); }
+ MockTransport& transport() { return transport_; }
+ SimulatedClock& clock() { return clock_; }
private:
NiceMock<MockTransport> transport_;
@@ -148,7 +184,7 @@
encoded_image.data()[0] = kPayload;
encoded_image.set_size(1);
- RtpVideoSenderTestFixture test({kSsrc1}, kPayloadType, {});
+ RtpVideoSenderTestFixture test({kSsrc1}, {kRtxSsrc1}, kPayloadType, {});
EXPECT_NE(
EncodedImageCallback::Result::OK,
test.router()->OnEncodedImage(encoded_image, nullptr, nullptr).error);
@@ -179,7 +215,8 @@
encoded_image_1.data()[0] = kPayload;
encoded_image_1.set_size(1);
- RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, kPayloadType, {});
+ RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2},
+ kPayloadType, {});
CodecSpecificInfo codec_info;
codec_info.codecType = kVideoCodecVP8;
@@ -226,7 +263,8 @@
EncodedImage encoded_image_2(encoded_image_1);
encoded_image_2.SetSpatialIndex(1);
- RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, kPayloadType, {});
+ RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2},
+ kPayloadType, {});
CodecSpecificInfo codec_info;
codec_info.codecType = kVideoCodecVP8;
@@ -256,7 +294,8 @@
}
TEST(RtpVideoSenderTest, CreateWithNoPreviousStates) {
- RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, kPayloadType, {});
+ RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2},
+ kPayloadType, {});
test.router()->SetActive(true);
std::map<uint32_t, RtpPayloadState> initial_states =
@@ -280,7 +319,8 @@
std::map<uint32_t, RtpPayloadState> states = {{kSsrc1, state1},
{kSsrc2, state2}};
- RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, kPayloadType, states);
+ RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2},
+ kPayloadType, states);
test.router()->SetActive(true);
std::map<uint32_t, RtpPayloadState> initial_states =
@@ -301,7 +341,8 @@
void(const FrameCounts& frame_counts, uint32_t ssrc));
} callback;
- RtpVideoSenderTestFixture test({kSsrc1}, kPayloadType, {}, &callback);
+ RtpVideoSenderTestFixture test({kSsrc1}, {kRtxSsrc1}, kPayloadType, {},
+ &callback);
constexpr uint8_t kPayload = 'a';
EncodedImage encoded_image;
@@ -346,4 +387,123 @@
EXPECT_EQ(1, frame_counts.delta_frames);
}
+// Integration test verifying that ack of packet via TransportFeedback means
+// that the packet is removed from RtpPacketHistory and won't be retranmistted
+// again.
+TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) {
+ const int64_t kTimeoutMs = 500;
+
+ RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2},
+ kPayloadType, {});
+ test.router()->SetActive(true);
+
+ constexpr uint8_t kPayload = 'a';
+ EncodedImage encoded_image;
+ encoded_image.SetTimestamp(1);
+ encoded_image.capture_time_ms_ = 2;
+ encoded_image._frameType = VideoFrameType::kVideoFrameKey;
+ encoded_image.Allocate(1);
+ encoded_image.data()[0] = kPayload;
+ encoded_image.set_size(1);
+
+ // Send two tiny images, mapping to two RTP packets. Capture sequence numbers.
+ rtc::Event event;
+ std::vector<uint16_t> rtp_sequence_numbers;
+ std::vector<uint16_t> transport_sequence_numbers;
+ EXPECT_CALL(test.transport(), SendRtp)
+ .Times(2)
+ .WillRepeatedly(
+ [&event, &rtp_sequence_numbers, &transport_sequence_numbers](
+ const uint8_t* packet, size_t length,
+ const PacketOptions& options) {
+ RtpPacket rtp_packet;
+ EXPECT_TRUE(rtp_packet.Parse(packet, length));
+ rtp_sequence_numbers.push_back(rtp_packet.SequenceNumber());
+ transport_sequence_numbers.push_back(options.packet_id);
+ if (transport_sequence_numbers.size() == 2) {
+ event.Set();
+ }
+ return true;
+ });
+ EXPECT_EQ(
+ EncodedImageCallback::Result::OK,
+ test.router()->OnEncodedImage(encoded_image, nullptr, nullptr).error);
+ encoded_image.SetTimestamp(2);
+ encoded_image.capture_time_ms_ = 3;
+ EXPECT_EQ(
+ EncodedImageCallback::Result::OK,
+ test.router()->OnEncodedImage(encoded_image, nullptr, nullptr).error);
+ test.clock().AdvanceTimeMilliseconds(33);
+
+ ASSERT_TRUE(event.Wait(kTimeoutMs));
+
+ // Construct a NACK message for requesting retransmission of both packet.
+ rtcp::Nack nack;
+ nack.SetMediaSsrc(kSsrc1);
+ nack.SetPacketIds(rtp_sequence_numbers);
+ rtc::Buffer nack_buffer = nack.Build();
+
+ std::vector<uint16_t> retransmitted_rtp_sequence_numbers;
+ EXPECT_CALL(test.transport(), SendRtp)
+ .Times(2)
+ .WillRepeatedly([&event, &retransmitted_rtp_sequence_numbers](
+ const uint8_t* packet, size_t length,
+ const PacketOptions& options) {
+ RtpPacket rtp_packet;
+ EXPECT_TRUE(rtp_packet.Parse(packet, length));
+ EXPECT_EQ(rtp_packet.Ssrc(), kRtxSsrc1);
+ // Capture the retransmitted sequence number from the RTX header.
+ rtc::ArrayView<const uint8_t> payload = rtp_packet.payload();
+ retransmitted_rtp_sequence_numbers.push_back(
+ ByteReader<uint16_t>::ReadBigEndian(payload.data()));
+ if (retransmitted_rtp_sequence_numbers.size() == 2) {
+ event.Set();
+ }
+ return true;
+ });
+ test.router()->DeliverRtcp(nack_buffer.data(), nack_buffer.size());
+ ASSERT_TRUE(event.Wait(kTimeoutMs));
+
+ // Verify that both packets were retransmitted.
+ EXPECT_EQ(retransmitted_rtp_sequence_numbers, rtp_sequence_numbers);
+
+ // Simulate transport feedback indicating fist packet received, next packet
+ // lost.
+ PacketFeedback received_packet_feedback(test.clock().TimeInMilliseconds(),
+ transport_sequence_numbers[0]);
+ received_packet_feedback.rtp_sequence_number = rtp_sequence_numbers[0];
+ received_packet_feedback.ssrc = kSsrc1;
+
+ PacketFeedback lost_packet_feedback(PacketFeedback::kNotReceived,
+ transport_sequence_numbers[1]);
+ lost_packet_feedback.rtp_sequence_number = rtp_sequence_numbers[1];
+ lost_packet_feedback.ssrc = kSsrc1;
+ std::vector<PacketFeedback> feedback_vector = {received_packet_feedback,
+ lost_packet_feedback};
+
+ test.router()->OnPacketFeedbackVector(feedback_vector);
+
+ // Advance time to make sure retransmission would be allowed and try again.
+ // This time the retransmission should not happen for the first packet since
+ // the history has been notified of the ack and removed the packet. The
+ // second packet, included in the feedback but not marked as received, should
+ // still be retransmitted.
+ test.clock().AdvanceTimeMilliseconds(33);
+ EXPECT_CALL(test.transport(), SendRtp)
+ .WillOnce([&event, &lost_packet_feedback](const uint8_t* packet,
+ size_t length,
+ const PacketOptions& options) {
+ RtpPacket rtp_packet;
+ EXPECT_TRUE(rtp_packet.Parse(packet, length));
+ EXPECT_EQ(rtp_packet.Ssrc(), kRtxSsrc1);
+ // Capture the retransmitted sequence number from the RTX header.
+ rtc::ArrayView<const uint8_t> payload = rtp_packet.payload();
+ EXPECT_EQ(lost_packet_feedback.rtp_sequence_number,
+ ByteReader<uint16_t>::ReadBigEndian(payload.data()));
+ event.Set();
+ return true;
+ });
+ test.router()->DeliverRtcp(nack_buffer.data(), nack_buffer.size());
+ ASSERT_TRUE(event.Wait(kTimeoutMs));
+}
} // namespace webrtc
