From a57711c941b89cd1b0f666f35b053bb5b796ced4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 24 Jul 2019 10:47:20 +0200 Subject: [PATCH] Fix issue with TransmissionOffset using new pacer code path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Stefan Holmer Cr-Commit-Position: refs/heads/master@{#28657} --- modules/rtp_rtcp/source/rtp_packet_history.cc | 3 - .../source/rtp_packet_history_unittest.cc | 16 ---- modules/rtp_rtcp/source/rtp_sender.cc | 7 ++ .../rtp_rtcp/source/rtp_sender_unittest.cc | 88 +++++++++++++++++++ 4 files changed, 95 insertions(+), 19 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 0dd322916e..253e9862be 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -142,9 +142,6 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, : 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 25c1f868f7..97e8bc3976 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 @@ TEST_F(RtpPacketHistoryTest, GetRtpPacket) { 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 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 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 66d6b71984..870cc2a869 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -1098,6 +1098,10 @@ bool RTPSender::SendToNetwork(std::unique_ptr packet, 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 @@ std::unique_ptr RTPSender::BuildRtxPacket( // 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 75c418fc97..ec12780756 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, SupportsPadding) { } } +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(); + 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(); + packet->AllocatePayload(sizeof(kPayloadData)); + + std::unique_ptr packet_to_pace; + EXPECT_CALL(mock_paced_sender_, EnqueuePacket) + .WillOnce([&](std::unique_ptr 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 transmission_time_extension = + transport_.sent_packets_.back().GetExtension(); + 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 rtx_packet_to_pace; + EXPECT_CALL(mock_paced_sender_, EnqueuePacket) + .WillOnce([&](std::unique_ptr 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(); + 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},