From 11820502b840f34aa9fccb1c273cefbcc457c962 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Fri, 12 Jul 2019 08:33:22 +0000 Subject: [PATCH] Revert "Make new pacer padding more like old one" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit bb7727211c535f8a9dce27891941b52b6ea8e750. Reason for revert: Speculative revert (some perf test are failing) Original change's description: > Make new pacer padding more like old one > > The (currently unused) new pacer code path was implemented with what > was intended as a more careful padding strategy. > Unfortunately this doesn't work as well as expected due to the fact > that the padding budget cannot build up underuse. > > I made another CL that could fix that, but I think it adds complexity > for dubious gains. It also will make it more difficult to find any > potential regression when switching to the new path, should one occur. > See https://webrtc-review.googlesource.com/c/src/+/144563 > > Therefore, this CL makes the new code path choose RTX payload in the > same way as is currently done. > > Bug: webrtc:10633 > Change-Id: If2115d4fa7463add959faa77c63101286c27e0f5 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145202 > Reviewed-by: Stefan Holmer > Commit-Queue: Erik Språng > Cr-Commit-Position: refs/heads/master@{#28537} TBR=sprang@webrtc.org,stefan@webrtc.org Change-Id: I99b72858414e0a245da596d94694449da88fd626 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10633 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145324 Reviewed-by: Mirko Bonadei Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#28550} --- modules/rtp_rtcp/source/rtp_sender.cc | 5 ++++- modules/rtp_rtcp/source/rtp_sender_unittest.cc | 14 +++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 0ab938b610..76cc19c2fc 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -1007,11 +1007,14 @@ std::vector> RTPSender::GeneratePadding( std::vector> padding_packets; size_t bytes_left = target_size_bytes; if ((rtx_ & kRtxRedundantPayloads) != 0) { - while (bytes_left >= kMinPayloadPaddingBytes) { + while (bytes_left >= 0) { std::unique_ptr packet = packet_history_.GetPayloadPaddingPacket( [&](const RtpPacketToSend& packet) -> std::unique_ptr { + if (packet.payload_size() + kRtxHeaderSize > bytes_left) { + return nullptr; + } return BuildRtxPacket(packet); }); if (!packet) { diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index b5703363fb..d2761ea347 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -2549,9 +2549,6 @@ TEST_P(RtpSenderTest, TrySendPacketUpdatesStats) { } TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { - // Min requested size in order to use RTX payload. - const size_t kMinPaddingSize = 50; - rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); rtp_sender_->SetStorePacketsStatus(true, 1); @@ -2569,7 +2566,7 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { // Generated padding has large enough budget that the video packet should be // retransmitted as padding. std::vector> generated_packets = - rtp_sender_->GeneratePadding(kMinPaddingSize); + rtp_sender_->GeneratePadding(kPayloadPacketSize + kRtxHeaderSize); ASSERT_EQ(generated_packets.size(), 1u); auto& padding_packet = generated_packets.front(); EXPECT_EQ(padding_packet->packet_type(), RtpPacketToSend::Type::kPadding); @@ -2578,11 +2575,13 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { kPayloadPacketSize + kRtxHeaderSize); // Not enough budged for payload padding, use plain padding instead. - const size_t kPaddingBytesRequested = kMinPaddingSize - 1; + const size_t kPaddingBytesRequested = kPayloadPacketSize + kRtxHeaderSize - 1; + const size_t kExpectedNumPaddingPackets = + (kPaddingBytesRequested + kMaxPaddingSize - 1) / kMaxPaddingSize; size_t padding_bytes_generated = 0; generated_packets = rtp_sender_->GeneratePadding(kPaddingBytesRequested); - EXPECT_EQ(generated_packets.size(), 1u); + EXPECT_EQ(generated_packets.size(), kExpectedNumPaddingPackets); for (auto& packet : generated_packets) { EXPECT_EQ(packet->packet_type(), RtpPacketToSend::Type::kPadding); EXPECT_EQ(packet->Ssrc(), kRtxSsrc); @@ -2591,7 +2590,8 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { padding_bytes_generated += packet->padding_size(); } - EXPECT_EQ(padding_bytes_generated, kMaxPaddingSize); + EXPECT_EQ(padding_bytes_generated, + kExpectedNumPaddingPackets * kMaxPaddingSize); } TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) {