Fix issue with TransmissionOffset using new pacer code path
This CL fixes two issues related to the TransmissionOffset header
extension and the new (not yet active) pacer mode.
Previously capture time (if unset) would be populated when put into the
packet history before entering the pacer. Since the pacer now owns the
packets, this does not occur until packet is actually sent, if at all.
Capture has really nothing to do with the packet history, this should
be set by the RtpSender pre-pacing instead.
Furthermore, for retransmissions the old path would take the capture
time from the original packet, build the RTX-wrapped retransmission and
set the toffset extension of the RTX packet using that captured capture
time. Since RTX packets are now fully built before the pacer, this does
not work, and we need to transfer the capture time from the original to
the RTX packet instead.
Bug: webrtc:10633
Change-Id: I031e8b6cc4ab20fb094dbd46720829b78951e7f9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146218
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28657}
diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc
index 0dd3229..253e986 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history.cc
+++ b/modules/rtp_rtcp/source/rtp_packet_history.cc
@@ -142,9 +142,6 @@
: 0));
RTC_DCHECK(it.second) << "Failed to insert packet in history.";
StoredPacket& stored_packet = it.first->second;
- if (stored_packet.packet_->capture_time_ms() <= 0) {
- stored_packet.packet_->set_capture_time_ms(now_ms);
- }
if (!start_seqno_) {
start_seqno_ = rtp_seq_no;
diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
index 25c1f86..97e8bc3 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
@@ -137,22 +137,6 @@
EXPECT_EQ(capture_time_ms, packet_out->capture_time_ms());
}
-TEST_F(RtpPacketHistoryTest, NoCaptureTime) {
- hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
- fake_clock_.AdvanceTimeMilliseconds(1);
- int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
- std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
- packet->set_capture_time_ms(-1);
- rtc::CopyOnWriteBuffer buffer = packet->Buffer();
- hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, absl::nullopt);
-
- std::unique_ptr<RtpPacketToSend> packet_out =
- hist_.GetPacketAndSetSendTime(kStartSeqNum);
- EXPECT_TRUE(packet_out);
- EXPECT_EQ(buffer, packet_out->Buffer());
- EXPECT_EQ(capture_time_ms, packet_out->capture_time_ms());
-}
-
TEST_F(RtpPacketHistoryTest, DontRetransmit) {
hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc
index 66d6b71..870cc2a 100644
--- a/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -1098,6 +1098,10 @@
auto packet_type = packet->packet_type();
RTC_CHECK(packet_type) << "Packet type must be set before sending.";
+ if (packet->capture_time_ms() <= 0) {
+ packet->set_capture_time_ms(now_ms);
+ }
+
if (pacer_legacy_packet_referencing_) {
// If |pacer_reference_packets_| then pacer needs to find the packet in
// the history when it is time to send, so move packet there.
@@ -1574,6 +1578,9 @@
// Add original application data.
rtx_packet->set_application_data(packet.application_data());
+ // Copy capture time so e.g. TransmissionOffset is correctly set.
+ rtx_packet->set_capture_time_ms(packet.capture_time_ms());
+
return rtx_packet;
}
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index 75c418f..ec12780 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -2620,6 +2620,94 @@
}
}
+TEST_P(RtpSenderTest, SetsCaptureTimeAndPopulatesTransmissionOffset) {
+ rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionTransmissionTimeOffset,
+ kTransmissionTimeOffsetExtensionId);
+
+ rtp_sender_->SetSendingMediaStatus(true);
+ rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads);
+ rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload);
+ rtp_sender_->SetStorePacketsStatus(true, 10);
+
+ const int64_t kMissingCaptureTimeMs = 0;
+ const uint32_t kTimestampTicksPerMs = 90;
+ const int64_t kOffsetMs = 10;
+
+ if (GetParam().pacer_references_packets) {
+ EXPECT_CALL(mock_paced_sender_, InsertPacket);
+
+ auto packet =
+ BuildRtpPacket(kPayload, kMarkerBit, fake_clock_.TimeInMilliseconds(),
+ kMissingCaptureTimeMs);
+ packet->set_packet_type(RtpPacketToSend::Type::kVideo);
+ packet->ReserveExtension<TransmissionOffset>();
+ packet->AllocatePayload(sizeof(kPayloadData));
+ EXPECT_TRUE(
+ rtp_sender_->SendToNetwork(std::move(packet), kAllowRetransmission));
+
+ fake_clock_.AdvanceTimeMilliseconds(kOffsetMs);
+
+ rtp_sender_->TimeToSendPacket(kSsrc, kSeqNum,
+ fake_clock_.TimeInMilliseconds(), false,
+ PacedPacketInfo());
+ } else {
+ auto packet =
+ BuildRtpPacket(kPayload, kMarkerBit, fake_clock_.TimeInMilliseconds(),
+ kMissingCaptureTimeMs);
+ packet->set_packet_type(RtpPacketToSend::Type::kVideo);
+ packet->ReserveExtension<TransmissionOffset>();
+ packet->AllocatePayload(sizeof(kPayloadData));
+
+ std::unique_ptr<RtpPacketToSend> packet_to_pace;
+ EXPECT_CALL(mock_paced_sender_, EnqueuePacket)
+ .WillOnce([&](std::unique_ptr<RtpPacketToSend> packet) {
+ EXPECT_GT(packet->capture_time_ms(), 0);
+ packet_to_pace = std::move(packet);
+ });
+
+ EXPECT_TRUE(
+ rtp_sender_->SendToNetwork(std::move(packet), kAllowRetransmission));
+
+ fake_clock_.AdvanceTimeMilliseconds(kOffsetMs);
+
+ rtp_sender_->TrySendPacket(packet_to_pace.get(), PacedPacketInfo());
+ }
+
+ EXPECT_EQ(1, transport_.packets_sent());
+ absl::optional<int32_t> transmission_time_extension =
+ transport_.sent_packets_.back().GetExtension<TransmissionOffset>();
+ ASSERT_TRUE(transmission_time_extension.has_value());
+ EXPECT_EQ(*transmission_time_extension, kOffsetMs * kTimestampTicksPerMs);
+
+ // Retransmit packet. The RTX packet should get the same capture time as the
+ // original packet, so offset is delta from original packet to now.
+ fake_clock_.AdvanceTimeMilliseconds(kOffsetMs);
+
+ if (GetParam().pacer_references_packets) {
+ EXPECT_CALL(mock_paced_sender_, InsertPacket);
+ EXPECT_GT(rtp_sender_->ReSendPacket(kSeqNum), 0);
+ rtp_sender_->TimeToSendPacket(kSsrc, kSeqNum,
+ fake_clock_.TimeInMilliseconds(), true,
+ PacedPacketInfo());
+ } else {
+ std::unique_ptr<RtpPacketToSend> rtx_packet_to_pace;
+ EXPECT_CALL(mock_paced_sender_, EnqueuePacket)
+ .WillOnce([&](std::unique_ptr<RtpPacketToSend> packet) {
+ EXPECT_GT(packet->capture_time_ms(), 0);
+ rtx_packet_to_pace = std::move(packet);
+ });
+
+ EXPECT_GT(rtp_sender_->ReSendPacket(kSeqNum), 0);
+ rtp_sender_->TrySendPacket(rtx_packet_to_pace.get(), PacedPacketInfo());
+ }
+
+ EXPECT_EQ(2, transport_.packets_sent());
+ transmission_time_extension =
+ transport_.sent_packets_.back().GetExtension<TransmissionOffset>();
+ ASSERT_TRUE(transmission_time_extension.has_value());
+ EXPECT_EQ(*transmission_time_extension, 2 * kOffsetMs * kTimestampTicksPerMs);
+}
+
INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead,
RtpSenderTest,
::testing::Values(TestConfig{false, false},