From 26bc6695cd910e116b48491bc76428e25f46472e Mon Sep 17 00:00:00 2001 From: Petter Strandmark Date: Tue, 29 May 2018 08:43:35 +0200 Subject: [PATCH] Pass packet retransmission information in PacketOptions. bugs.webrtc.org/8439 introduces application data that could e.g. contain timestamps. We would like to take different actions for this data depending on whether this is the first time a packet is being sent. Bug: webrtc:8906 Change-Id: Ib370d76beec2960d961bf44391930faa4b193479 Reviewed-on: https://webrtc-review.googlesource.com/77643 Reviewed-by: Danil Chapovalov Reviewed-by: Stefan Holmer Reviewed-by: Niels Moller Commit-Queue: Petter Strandmark Cr-Commit-Position: refs/heads/master@{#23426} --- api/call/transport.h | 2 ++ modules/rtp_rtcp/source/rtp_sender.cc | 7 ++++ .../rtp_rtcp/source/rtp_sender_unittest.cc | 34 +++++++++++++++---- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/api/call/transport.h b/api/call/transport.h index df101fcf05..5cb6b3007c 100644 --- a/api/call/transport.h +++ b/api/call/transport.h @@ -29,6 +29,8 @@ struct PacketOptions { // Additional data bound to the RTP packet for use in application code, // outside of WebRTC. std::vector application_data; + // Whether this is a retransmission of an earlier packet. + bool is_retransmit = false; }; class Transport { diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 11e96b54a8..c4ae627fc9 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -594,6 +594,8 @@ size_t RTPSender::SendPadData(size_t bytes, padding_packet.SetExtension( AbsoluteSendTime::MsTo24Bits(now_ms)); PacketOptions options; + // Padding packets are never retransmissions. + options.is_retransmit = false; bool has_transport_seq_num = UpdateTransportSequenceNumber(&padding_packet, &options.packet_id); padding_packet.SetPadding(padding_bytes_in_packet, &random_); @@ -811,6 +813,10 @@ bool RTPSender::PrepareAndSendPacket(std::unique_ptr packet, } PacketOptions options; + // If we are sending over RTX, it also means this is a retransmission. + // E.g. RTPSender::TrySendRedundantPayloads calls PrepareAndSendPacket with + // send_over_rtx = true but is_retransmit = false. + options.is_retransmit = is_retransmit || send_over_rtx; if (UpdateTransportSequenceNumber(packet_to_send, &options.packet_id)) { AddPacketToTransportFeedback(options.packet_id, *packet_to_send, pacing_info); @@ -946,6 +952,7 @@ bool RTPSender::SendToNetwork(std::unique_ptr packet, } PacketOptions options; + options.is_retransmit = false; if (UpdateTransportSequenceNumber(packet.get(), &options.packet_id)) { AddPacketToTransportFeedback(options.packet_id, *packet.get(), PacedPacketInfo()); diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 282e44aece..65f2a17d02 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -71,7 +71,7 @@ uint64_t ConvertMsToAbsSendTime(int64_t time_ms) { class LoopbackTransportTest : public webrtc::Transport { public: - LoopbackTransportTest() : total_bytes_sent_(0), last_packet_id_(-1) { + LoopbackTransportTest() : total_bytes_sent_(0) { receivers_extensions_.Register(kRtpExtensionTransmissionTimeOffset, kTransmissionTimeOffsetExtensionId); receivers_extensions_.Register(kRtpExtensionAbsoluteSendTime, @@ -90,7 +90,7 @@ class LoopbackTransportTest : public webrtc::Transport { bool SendRtp(const uint8_t* data, size_t len, const PacketOptions& options) override { - last_packet_id_ = options.packet_id; + last_options_ = options; total_bytes_sent_ += len; sent_packets_.push_back(RtpPacketReceived(&receivers_extensions_)); EXPECT_TRUE(sent_packets_.back().Parse(data, len)); @@ -101,7 +101,7 @@ class LoopbackTransportTest : public webrtc::Transport { int packets_sent() { return sent_packets_.size(); } size_t total_bytes_sent_; - int last_packet_id_; + PacketOptions last_options_; std::vector sent_packets_; private: @@ -467,7 +467,19 @@ TEST_P(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) { uint16_t transport_seq_no; ASSERT_TRUE(packet.GetExtension(&transport_seq_no)); EXPECT_EQ(kTransportSequenceNumber, transport_seq_no); - EXPECT_EQ(transport_.last_packet_id_, transport_seq_no); + EXPECT_EQ(transport_.last_options_.packet_id, transport_seq_no); +} + +TEST_P(RtpSenderTestWithoutPacer, PacketOptionsNoRetransmission) { + rtp_sender_.reset(new RTPSender( + false, &fake_clock_, &transport_, nullptr, nullptr, &seq_num_allocator_, + &feedback_observer_, nullptr, nullptr, nullptr, &mock_rtc_event_log_, + &send_packet_observer_, &retransmission_rate_limiter_, nullptr, false)); + rtp_sender_->SetSSRC(kSsrc); + + SendGenericPayload(); + + EXPECT_FALSE(transport_.last_options_.is_retransmit); } TEST_P(RtpSenderTestWithoutPacer, NoAllocationIfNotRegistered) { @@ -520,7 +532,7 @@ TEST_P(RtpSenderTest, SendsPacketsWithTransportSequenceNumber) { uint16_t transport_seq_no; EXPECT_TRUE(packet.GetExtension(&transport_seq_no)); EXPECT_EQ(kTransportSequenceNumber, transport_seq_no); - EXPECT_EQ(transport_.last_packet_id_, transport_seq_no); + EXPECT_EQ(transport_.last_options_.packet_id, transport_seq_no); } TEST_P(RtpSenderTest, WritesPacerExitToTimingExtension) { @@ -844,6 +856,7 @@ TEST_P(RtpSenderTest, OnSendPacketNotUpdatedForRetransmits) { fake_clock_.TimeInMilliseconds(), kIsRetransmit, PacedPacketInfo()); EXPECT_EQ(1, transport_.packets_sent()); + EXPECT_TRUE(transport_.last_options_.is_retransmit); } TEST_P(RtpSenderTest, OnSendPacketNotUpdatedWithoutSeqNumAllocator) { @@ -925,20 +938,27 @@ TEST_P(RtpSenderTest, SendRedundantPayloads) { EXPECT_EQ(kMaxPaddingSize, rtp_sender_->TimeToSendPadding(49, PacedPacketInfo())); + PacketOptions options; EXPECT_CALL(transport, SendRtp(_, kPayloadSizes[0] + rtp_header_len + kRtxHeaderSize, _)) - .WillOnce(testing::Return(true)); + .WillOnce(testing::DoAll(testing::SaveArg<2>(&options), + testing::Return(true))); EXPECT_EQ(kPayloadSizes[0], rtp_sender_->TimeToSendPadding(500, PacedPacketInfo())); + EXPECT_TRUE(options.is_retransmit); EXPECT_CALL(transport, SendRtp(_, kPayloadSizes[kNumPayloadSizes - 1] + rtp_header_len + kRtxHeaderSize, _)) .WillOnce(testing::Return(true)); + + options.is_retransmit = false; EXPECT_CALL(transport, SendRtp(_, kMaxPaddingSize + rtp_header_len, _)) - .WillOnce(testing::Return(true)); + .WillOnce(testing::DoAll(testing::SaveArg<2>(&options), + testing::Return(true))); EXPECT_EQ(kPayloadSizes[kNumPayloadSizes - 1] + kMaxPaddingSize, rtp_sender_->TimeToSendPadding(999, PacedPacketInfo())); + EXPECT_FALSE(options.is_retransmit); } TEST_P(RtpSenderTestWithoutPacer, SendGenericVideo) {