From a7e3bcebaedce13d5b5187982a50206152a1349e Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Fri, 12 Jul 2019 17:35:56 +0000 Subject: [PATCH] Reland "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 11820502b840f34aa9fccb1c273cefbcc457c962. Reason for revert: The culprit was https://webrtc-review.googlesource.com/c/src/+/133169. Original change's description: > Revert "Make new pacer padding more like old one" > > 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} TBR=mbonadei@webrtc.org,sprang@webrtc.org,stefan@webrtc.org Change-Id: I1fff79d75dc888921c6fbfc7f3980395a67e1c1a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10633 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145403 Reviewed-by: Mirko Bonadei Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#28565} --- modules/rtp_rtcp/source/rtp_sender.cc | 5 +---- modules/rtp_rtcp/source/rtp_sender_unittest.cc | 14 +++++++------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 76cc19c2fc..0ab938b610 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -1007,14 +1007,11 @@ std::vector> RTPSender::GeneratePadding( std::vector> padding_packets; size_t bytes_left = target_size_bytes; if ((rtx_ & kRtxRedundantPayloads) != 0) { - while (bytes_left >= 0) { + while (bytes_left >= kMinPayloadPaddingBytes) { 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 d2761ea347..b5703363fb 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -2549,6 +2549,9 @@ 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); @@ -2566,7 +2569,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(kPayloadPacketSize + kRtxHeaderSize); + rtp_sender_->GeneratePadding(kMinPaddingSize); ASSERT_EQ(generated_packets.size(), 1u); auto& padding_packet = generated_packets.front(); EXPECT_EQ(padding_packet->packet_type(), RtpPacketToSend::Type::kPadding); @@ -2575,13 +2578,11 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { kPayloadPacketSize + kRtxHeaderSize); // Not enough budged for payload padding, use plain padding instead. - const size_t kPaddingBytesRequested = kPayloadPacketSize + kRtxHeaderSize - 1; - const size_t kExpectedNumPaddingPackets = - (kPaddingBytesRequested + kMaxPaddingSize - 1) / kMaxPaddingSize; + const size_t kPaddingBytesRequested = kMinPaddingSize - 1; size_t padding_bytes_generated = 0; generated_packets = rtp_sender_->GeneratePadding(kPaddingBytesRequested); - EXPECT_EQ(generated_packets.size(), kExpectedNumPaddingPackets); + EXPECT_EQ(generated_packets.size(), 1u); for (auto& packet : generated_packets) { EXPECT_EQ(packet->packet_type(), RtpPacketToSend::Type::kPadding); EXPECT_EQ(packet->Ssrc(), kRtxSsrc); @@ -2590,8 +2591,7 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { padding_bytes_generated += packet->padding_size(); } - EXPECT_EQ(padding_bytes_generated, - kExpectedNumPaddingPackets * kMaxPaddingSize); + EXPECT_EQ(padding_bytes_generated, kMaxPaddingSize); } TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) {