admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove the non-useful rtx payload padding option

Browse Source 
This CL removes the field trial left in place as a kill-switch in case
there were any regressions related to selecting payload padding based
on the likelihood of being useful instead of matching size.

It also removes the functionality that was only enabled with the
kill-switch active.

The feature has been default-on since June 23rd 2019:
https://webrtc.googlesource.com/src.git/+/214f54365ec210db76218a35ead66c9ce23e068e

Since we have not observed any issues, let's clean this code up.

Bug: webrtc:8975
Change-Id: I7f49fe354227b3f6566a250332e56b6d70fe2f09
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145821
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28616}
This commit is contained in:
Erik Språng 2019-07-16 21:09:14 +02:00 committed by Commit Bot
parent 76c89daac5
commit 21f2fc9c73
6 changed files with 2 additions and 307 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
modules/rtp_rtcp/source/rtp_packet_history.cc
View File

@ -21,16 +21,6 @@
#include "system_wrappers/include/clock.h"
namespace webrtc {
namespace {
// Utility function to get the absolute difference in size between the provided
// target size and the size of packet.
size_t SizeDiff(size_t packet_size, size_t size) {
if (packet_size > size) {
return packet_size - size;
}
return size - packet_size;
}
} // namespace
constexpr size_t RtpPacketHistory::kMaxCapacity;
constexpr int64_t RtpPacketHistory::kMinPacketDurationMs;
@ -150,16 +140,6 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr<RtpPacketToSend> packet,
: 0));
RTC_DCHECK(it.second) << "Failed to insert packet in history.";
StoredPacket& stored_packet = it.first->second;
if (stored_packet.packet_) {
// It is an error if this happen. But it can happen if the sequence numbers
// for some reason restart without that the history has been reset.
auto size_iterator = packet_size_.find(stored_packet.packet_->size());
if (size_iterator != packet_size_.end() &&
size_iterator->second == stored_packet.packet_->SequenceNumber()) {
packet_size_.erase(size_iterator);
}
}
if (stored_packet.packet_->capture_time_ms() <= 0) {
stored_packet.packet_->set_capture_time_ms(now_ms);
}
@ -170,7 +150,6 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr<RtpPacketToSend> packet,
// Store the sequence number of the last send packet with this size.
if (type != StorageType::kDontRetransmit) {
packet_size_[stored_packet.packet_->size()] = rtp_seq_no;
auto it = padding_priority_.insert(&stored_packet);
RTC_DCHECK(it.second) << "Failed to insert packet into prio set.";
}
@ -317,45 +296,6 @@ bool RtpPacketHistory::VerifyRtt(const RtpPacketHistory::StoredPacket& packet,
return true;
}
std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetBestFittingPacket(
size_t packet_length) const {
rtc::CritScope cs(&lock_);
if (packet_size_.empty()) {
return nullptr;
}
auto size_iter_upper = packet_size_.upper_bound(packet_length);
auto size_iter_lower = size_iter_upper;
if (size_iter_upper == packet_size_.end()) {
--size_iter_upper;
}
if (size_iter_lower != packet_size_.begin()) {
--size_iter_lower;
}
const size_t upper_bound_diff =
SizeDiff(size_iter_upper->first, packet_length);
const size_t lower_bound_diff =
SizeDiff(size_iter_lower->first, packet_length);
const uint16_t seq_no = upper_bound_diff < lower_bound_diff
? size_iter_upper->second
: size_iter_lower->second;
auto history_it = packet_history_.find(seq_no);
if (history_it == packet_history_.end()) {
RTC_LOG(LS_ERROR) << "Can't find packet in history with seq_no" << seq_no;
RTC_DCHECK(false);
return nullptr;
}
if (!history_it->second.packet_) {
RTC_LOG(LS_ERROR) << "Packet pointer is null in history for seq_no"
<< seq_no;
RTC_DCHECK(false);
return nullptr;
}
RtpPacketToSend* best_packet = history_it->second.packet_.get();
return absl::make_unique<RtpPacketToSend>(*best_packet);
}
std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket() {
// Default implementation always just returns a copy of the packet.
return GetPayloadPaddingPacket([](const RtpPacketToSend& packet) {
@ -423,7 +363,6 @@ bool RtpPacketHistory::SetPendingTransmission(uint16_t sequence_number) {
void RtpPacketHistory::Reset() {
packet_history_.clear();
packet_size_.clear();
padding_priority_.clear();
start_seqno_.reset();
}
@ -508,12 +447,6 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::RemovePacket(
}
}
auto size_iterator = packet_size_.find(rtp_packet->size());
if (size_iterator != packet_size_.end() &&
size_iterator->second == rtp_packet->SequenceNumber()) {
packet_size_.erase(size_iterator);
}
return rtp_packet;
}

7
modules/rtp_rtcp/source/rtp_packet_history.h
View File

@ -108,11 +108,6 @@ class RtpPacketHistory {
// current state for packet, and never updates internal state.
absl::optional<PacketState> GetPacketState(uint16_t sequence_number) const;
// Get the packet (if any) from the history, with size closest to
// |packet_size|. The exact size of the packet is not guaranteed.
std::unique_ptr<RtpPacketToSend> GetBestFittingPacket(
size_t packet_size) const;
// Get the packet (if any) from the history, that is deemed most likely to
// the remote side. This is calculated from heuristics such as packet age
// and times retransmitted. Updated the send time of the packet, so is not
@ -203,8 +198,6 @@ class RtpPacketHistory {
// Map from rtp sequence numbers to stored packet.
std::map<uint16_t, StoredPacket> packet_history_ RTC_GUARDED_BY(lock_);
// Map from packet size to sequence number.
std::map<size_t, uint16_t> packet_size_ RTC_GUARDED_BY(lock_);
// Total number of packets with StorageType::kAllowsRetransmission inserted.
uint64_t retransmittable_packets_inserted_ RTC_GUARDED_BY(lock_);

110
modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
View File

@ -471,116 +471,6 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCullingHighRtt) {
EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum));
}
TEST_F(RtpPacketHistoryTest, GetBestFittingPacket) {
const size_t kTargetSize = 500;
hist_.SetStorePacketsStatus(StorageMode::kStore, 10);
// Add three packets of various sizes.
std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
packet->SetPayloadSize(kTargetSize);
const size_t target_packet_size = packet->size();
hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
packet = CreateRtpPacket(To16u(kStartSeqNum + 1));
packet->SetPayloadSize(kTargetSize - 1);
hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
packet = CreateRtpPacket(To16u(kStartSeqNum + 2));
packet->SetPayloadSize(kTargetSize + 1);
hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
EXPECT_EQ(target_packet_size,
hist_.GetBestFittingPacket(target_packet_size)->size());
}
TEST_F(RtpPacketHistoryTest,
GetBestFittingPacketReturnsNextPacketWhenBestPacketHasBeenCulled) {
hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
packet->SetPayloadSize(50);
const size_t target_packet_size = packet->size();
hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
packet = hist_.GetBestFittingPacket(target_packet_size + 2);
ASSERT_THAT(packet, ::testing::NotNull());
// Send the packet and advance time past where packet expires.
ASSERT_THAT(hist_.GetPacketAndSetSendTime(kStartSeqNum),
::testing::NotNull());
fake_clock_.AdvanceTimeMilliseconds(
RtpPacketHistory::kPacketCullingDelayFactor *
RtpPacketHistory::kMinPacketDurationMs);
packet = CreateRtpPacket(To16u(kStartSeqNum + 1));
packet->SetPayloadSize(100);
hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
ASSERT_FALSE(hist_.GetPacketState(kStartSeqNum));
auto best_packet = hist_.GetBestFittingPacket(target_packet_size + 2);
ASSERT_THAT(best_packet, ::testing::NotNull());
EXPECT_EQ(best_packet->SequenceNumber(), To16u(kStartSeqNum + 1));
}
TEST_F(RtpPacketHistoryTest, GetBestFittingPacketReturnLastPacketWhenSameSize) {
const size_t kTargetSize = 500;
hist_.SetStorePacketsStatus(StorageMode::kStore, 10);
// Add two packets of same size.
std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
packet->SetPayloadSize(kTargetSize);
hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
packet = CreateRtpPacket(To16u(kStartSeqNum + 1));
packet->SetPayloadSize(kTargetSize);
hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
auto best_packet = hist_.GetBestFittingPacket(123);
ASSERT_THAT(best_packet, ::testing::NotNull());
EXPECT_EQ(best_packet->SequenceNumber(), To16u(kStartSeqNum + 1));
}
TEST_F(RtpPacketHistoryTest,
GetBestFittingPacketReturnsPacketWithSmallestDiff) {
const size_t kTargetSize = 500;
hist_.SetStorePacketsStatus(StorageMode::kStore, 10);
// Add two packets of very different size.
std::unique_ptr<RtpPacketToSend> small_packet = CreateRtpPacket(kStartSeqNum);
small_packet->SetPayloadSize(kTargetSize);
hist_.PutRtpPacket(std::move(small_packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
auto large_packet = CreateRtpPacket(To16u(kStartSeqNum + 1));
large_packet->SetPayloadSize(kTargetSize * 2);
hist_.PutRtpPacket(std::move(large_packet), kAllowRetransmission,
fake_clock_.TimeInMilliseconds());
ASSERT_THAT(hist_.GetBestFittingPacket(kTargetSize), ::testing::NotNull());
EXPECT_EQ(hist_.GetBestFittingPacket(kTargetSize)->SequenceNumber(),
kStartSeqNum);
ASSERT_THAT(hist_.GetBestFittingPacket(kTargetSize * 2),
::testing::NotNull());
EXPECT_EQ(hist_.GetBestFittingPacket(kTargetSize * 2)->SequenceNumber(),
To16u(kStartSeqNum + 1));
}
TEST_F(RtpPacketHistoryTest,
GetBestFittingPacketIgnoresNoneRetransmitablePackets) {
hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
packet->SetPayloadSize(50);
hist_.PutRtpPacket(std::move(packet), kDontRetransmit,
fake_clock_.TimeInMilliseconds());
EXPECT_THAT(hist_.GetBestFittingPacket(50), ::testing::IsNull());
EXPECT_THAT(hist_.GetPacketAndSetSendTime(kStartSeqNum),
::testing::NotNull());
}
TEST_F(RtpPacketHistoryTest, CullWithAcks) {
const int64_t kPacketLifetime = RtpPacketHistory::kMinPacketDurationMs *
RtpPacketHistory::kPacketCullingDelayFactor;

14
modules/rtp_rtcp/source/rtp_sender.cc
View File

@ -201,9 +201,6 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config)
legacy_packet_history_storage_mode_(
IsEnabled("WebRTC-UseRtpPacketHistoryLegacyStorageMode",
config.field_trials)),
payload_padding_prefer_useful_packets_(
!IsDisabled("WebRTC-PayloadPadding-UseMostUsefulPacket",
config.field_trials)),
pacer_legacy_packet_referencing_(
!IsDisabled("WebRTC-Pacer-LegacyPacketReferencing",
config.field_trials)) {
@ -293,9 +290,6 @@ RTPSender::RTPSender(
legacy_packet_history_storage_mode_(
field_trials.Lookup("WebRTC-UseRtpPacketHistoryLegacyStorageMode")
.find("Enabled") == 0),
payload_padding_prefer_useful_packets_(
field_trials.Lookup("WebRTC-PayloadPadding-UseMostUsefulPacket")
.find("Disabled") != 0),
pacer_legacy_packet_referencing_(
field_trials.Lookup("WebRTC-Pacer-LegacyPacketReferencing")
.find("Disabled") != 0) {
@ -441,12 +435,8 @@ size_t RTPSender::TrySendRedundantPayloads(size_t bytes_to_send,
int bytes_left = static_cast<int>(bytes_to_send);
while (bytes_left >= kMinPayloadPaddingBytes) {
std::unique_ptr<RtpPacketToSend> packet;
if (payload_padding_prefer_useful_packets_) {
packet = packet_history_.GetPayloadPaddingPacket();
} else {
packet = packet_history_.GetBestFittingPacket(bytes_left);
}
std::unique_ptr<RtpPacketToSend> packet =
packet_history_.GetPayloadPaddingPacket();
if (!packet)
break;

6
modules/rtp_rtcp/source/rtp_sender.h
View File

@ -320,12 +320,6 @@ class RTPSender {
const bool send_side_bwe_with_overhead_;
const bool legacy_packet_history_storage_mode_;
// Set by field trial "WebRTC-PayloadPadding-UseMostUsefulPacket". If set
// to "Enabled" this field will be true and
// packet_history_.GetPayloadPaddingPacket() will be called instead of
// packet_history_.GetBestFittingPacket() in TrySendRedundantPayloads().
const bool payload_padding_prefer_useful_packets_;
// If true, PacedSender should only reference packets as in legacy mode.
// If false, PacedSender may have direct ownership of RtpPacketToSend objects.
// Defaults to true, will be changed to default false soon.

105
modules/rtp_rtcp/source/rtp_sender_unittest.cc
View File

@ -1229,8 +1229,6 @@ TEST_P(RtpSenderTest, OnSendPacketNotUpdatedWithoutSeqNumAllocator) {
EXPECT_EQ(1, transport_.packets_sent());
}
// TODO(bugs.webrtc.org/8975): Remove this test when non-useful padding is
// removed.
TEST_P(RtpSenderTest, SendRedundantPayloads) {
if (!GetParam().pacer_references_packets) {
// If PacedSender owns the RTP packets, GeneratePadding() family of methods
@ -1238,109 +1236,6 @@ TEST_P(RtpSenderTest, SendRedundantPayloads) {
return;
}
test::ScopedFieldTrials field_trials(
"WebRTC-PayloadPadding-UseMostUsefulPacket/Disabled/");
MockTransport transport;
RtpRtcp::Configuration config;
config.clock = &fake_clock_;
config.outgoing_transport = &transport;
config.paced_sender = &mock_paced_sender_;
config.media_send_ssrc = kSsrc;
config.rtx_send_ssrc = kRtxSsrc;
config.event_log = &mock_rtc_event_log_;
config.retransmission_rate_limiter = &retransmission_rate_limiter_;
rtp_sender_ = absl::make_unique<RTPSender>(config);
rtp_sender_->SetSequenceNumber(kSeqNum);
rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload);
uint16_t seq_num = kSeqNum;
rtp_sender_->SetStorePacketsStatus(true, 10);
int32_t rtp_header_len = kRtpHeaderSize;
EXPECT_EQ(
0, rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionAbsoluteSendTime,
kAbsoluteSendTimeExtensionId));
rtp_header_len += 4; // 4 bytes extension.
rtp_header_len += 4; // 4 extra bytes common to all extension headers.
rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads);
const size_t kNumPayloadSizes = 10;
const size_t kPayloadSizes[kNumPayloadSizes] = {500, 550, 600, 650, 700,
750, 800, 850, 900, 950};
// Expect all packets go through the pacer.
EXPECT_CALL(mock_rtc_event_log_,
LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing)))
.Times(kNumPayloadSizes);
// Send 10 packets of increasing size.
for (size_t i = 0; i < kNumPayloadSizes; ++i) {
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
EXPECT_CALL(transport, SendRtp(_, _, _)).WillOnce(::testing::Return(true));
if (GetParam().pacer_references_packets) {
EXPECT_CALL(mock_paced_sender_, InsertPacket(_, kSsrc, seq_num, _, _, _));
SendPacket(capture_time_ms, kPayloadSizes[i]);
rtp_sender_->TimeToSendPacket(kSsrc, seq_num,
fake_clock_.TimeInMilliseconds(), false,
PacedPacketInfo());
} else {
EXPECT_CALL(
mock_paced_sender_,
EnqueuePacket(AllOf(
Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc)),
Pointee(Property(&RtpPacketToSend::SequenceNumber, seq_num)))));
auto packet = SendPacket(capture_time_ms, kPayloadSizes[i]);
packet->set_packet_type(RtpPacketToSend::Type::kVideo);
rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo());
}
++seq_num;
fake_clock_.AdvanceTimeMilliseconds(33);
}
EXPECT_CALL(mock_rtc_event_log_,
LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing)))
.Times(AtLeast(4));
// The amount of padding to send it too small to send a payload packet.
EXPECT_CALL(transport, SendRtp(_, kMaxPaddingSize + rtp_header_len, _))
.WillOnce(Return(true));
EXPECT_EQ(kMaxPaddingSize,
rtp_sender_->TimeToSendPadding(49, PacedPacketInfo()));
PacketOptions options;
EXPECT_CALL(transport,
SendRtp(_, kPayloadSizes[0] + rtp_header_len + kRtxHeaderSize, _))
.WillOnce(DoAll(SaveArg<2>(&options), 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(Return(true));
options.is_retransmit = false;
EXPECT_CALL(transport, SendRtp(_, kMaxPaddingSize + rtp_header_len, _))
.WillOnce(DoAll(SaveArg<2>(&options), Return(true)));
EXPECT_EQ(kPayloadSizes[kNumPayloadSizes - 1] + kMaxPaddingSize,
rtp_sender_->TimeToSendPadding(999, PacedPacketInfo()));
EXPECT_FALSE(options.is_retransmit);
}
TEST_P(RtpSenderTest, SendRedundantPayloadsUsefulPadding) {
if (!GetParam().pacer_references_packets) {
// If PacedSender owns the RTP packets, GeneratePadding() family of methods
// will be called instead and this test makes no sense.
return;
}
test::ScopedFieldTrials field_trials(
"WebRTC-PayloadPadding-UseMostUsefulPacket/Enabled/");
MockTransport transport;
RtpRtcp::Configuration config;
config.clock = &fake_clock_;