zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

RtpSender::GeneratePadding() fixes for new PacedSender code path

Browse Source 
This CL fixes two things related to the (not yet active) new
PacedSender code path:

1. Make sure BWE header extensions are properly populated for all
   padding packets.
2. When generating padding, don't hold the RtpSender critsect when
   accessing the RtpPacketHistory as this may lead to a lock order
   inversion.

Bug: webrtc:10633
Change-Id: I8650fbf5dafddbeae61837d2137338163e1c48ce
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145723
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28613}
This commit is contained in:
Erik Språng
2019-07-15 20:33:40 +02:00
committed by Commit Bot
parent d70d80d882
commit 0f6191d227
2 changed files with 76 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@ -812,8 +812,13 @@ bool RTPSender::TrySendPacket(RtpPacketToSend* packet,
// the FEC.
int64_t now_ms = clock_->TimeInMilliseconds();
int64_t diff_ms = now_ms - packet->capture_time_ms();
packet->SetExtension<TransmissionOffset>(kTimestampTicksPerMs * diff_ms);
packet->SetExtension<AbsoluteSendTime>(AbsoluteSendTime::MsTo24Bits(now_ms));
if (packet->IsExtensionReserved<TransmissionOffset>()) {
packet->SetExtension<TransmissionOffset>(kTimestampTicksPerMs * diff_ms);
}
if (packet->IsExtensionReserved<AbsoluteSendTime>()) {
packet->SetExtension<AbsoluteSendTime>(
AbsoluteSendTime::MsTo24Bits(now_ms));
}
if (packet->HasExtension<VideoTimingExtension>()) {
if (populate_network2_timestamp_) {
@ -999,14 +1004,10 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
// them and puts them in the pacer queue. Since this should incur
// low overhead, keep the lock for the scope of the method in order
// to make the code more readable.
rtc::CritScope lock(&send_critsect_);
if (!sending_media_) {
return {};
}
std::vector<std::unique_ptr<RtpPacketToSend>> padding_packets;
size_t bytes_left = target_size_bytes;
if ((rtx_ & kRtxRedundantPayloads) != 0) {
if (SupportsRtxPayloadPadding()) {
while (bytes_left >= kMinPayloadPaddingBytes) {
std::unique_ptr<RtpPacketToSend> packet =
packet_history_.GetPayloadPaddingPacket(
@ -1024,6 +1025,11 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
}
}
rtc::CritScope lock(&send_critsect_);
if (!sending_media_) {
return {};
}
size_t padding_bytes_in_packet;
const size_t max_payload_size = max_packet_size_ - RtpHeaderLength();
if (audio_configured_) {
@ -1092,6 +1098,13 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
if (rtp_header_extension_map_.IsRegistered(TransportSequenceNumber::kId)) {
padding_packet->ReserveExtension<TransportSequenceNumber>();
}
if (rtp_header_extension_map_.IsRegistered(TransmissionOffset::kId)) {
padding_packet->ReserveExtension<TransmissionOffset>();
}
if (rtp_header_extension_map_.IsRegistered(AbsoluteSendTime::kId)) {
padding_packet->ReserveExtension<AbsoluteSendTime>();
}
padding_packet->SetPadding(padding_bytes_in_packet);
bytes_left -= std::min(bytes_left, padding_bytes_in_packet);
padding_packets.push_back(std::move(padding_packet));

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

@ -2556,6 +2556,16 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload);
rtp_sender_->SetStorePacketsStatus(true, 1);
ASSERT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransmissionTimeOffset,
kTransmissionTimeOffsetExtensionId));
ASSERT_EQ(
0, rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionAbsoluteSendTime,
kAbsoluteSendTimeExtensionId));
ASSERT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransportSequenceNumber,
kTransportSequenceNumberExtensionId));
const size_t kPayloadPacketSize = 1234;
std::unique_ptr<RtpPacketToSend> packet =
BuildRtpPacket(kPayload, true, 0, fake_clock_.TimeInMilliseconds());
@ -2564,6 +2574,7 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
packet->set_packet_type(RtpPacketToSend::Type::kVideo);
// Send a dummy video packet so it ends up in the packet history.
EXPECT_CALL(send_packet_observer_, OnSendPacket).Times(1);
EXPECT_TRUE(rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()));
// Generated padding has large enough budget that the video packet should be
@ -2576,6 +2587,18 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
EXPECT_EQ(padding_packet->Ssrc(), kRtxSsrc);
EXPECT_EQ(padding_packet->payload_size(),
kPayloadPacketSize + kRtxHeaderSize);
EXPECT_TRUE(padding_packet->IsExtensionReserved<TransportSequenceNumber>());
EXPECT_TRUE(padding_packet->IsExtensionReserved<AbsoluteSendTime>());
EXPECT_TRUE(padding_packet->IsExtensionReserved<TransmissionOffset>());
// Verify all header extensions are received.
EXPECT_TRUE(
rtp_sender_->TrySendPacket(padding_packet.get(), PacedPacketInfo()));
webrtc::RTPHeader rtp_header;
transport_.last_sent_packet().GetHeader(&rtp_header);
EXPECT_TRUE(rtp_header.extension.hasAbsoluteSendTime);
EXPECT_TRUE(rtp_header.extension.hasTransmissionTimeOffset);
EXPECT_TRUE(rtp_header.extension.hasTransportSequenceNumber);
// Not enough budged for payload padding, use plain padding instead.
const size_t kPaddingBytesRequested = kMinPaddingSize - 1;
@ -2589,6 +2612,18 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
EXPECT_EQ(packet->payload_size(), 0u);
EXPECT_GT(packet->padding_size(), 0u);
padding_bytes_generated += packet->padding_size();
EXPECT_TRUE(packet->IsExtensionReserved<TransportSequenceNumber>());
EXPECT_TRUE(packet->IsExtensionReserved<AbsoluteSendTime>());
EXPECT_TRUE(packet->IsExtensionReserved<TransmissionOffset>());
// Verify all header extensions are received.
EXPECT_TRUE(rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()));
webrtc::RTPHeader rtp_header;
transport_.last_sent_packet().GetHeader(&rtp_header);
EXPECT_TRUE(rtp_header.extension.hasAbsoluteSendTime);
EXPECT_TRUE(rtp_header.extension.hasTransmissionTimeOffset);
EXPECT_TRUE(rtp_header.extension.hasTransportSequenceNumber);
}
EXPECT_EQ(padding_bytes_generated, kMaxPaddingSize);
@ -2596,6 +2631,15 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) {
TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) {
rtp_sender_->SetStorePacketsStatus(true, 1);
ASSERT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransmissionTimeOffset,
kTransmissionTimeOffsetExtensionId));
ASSERT_EQ(
0, rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionAbsoluteSendTime,
kAbsoluteSendTimeExtensionId));
ASSERT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransportSequenceNumber,
kTransportSequenceNumberExtensionId));
const size_t kPayloadPacketSize = 1234;
// Send a dummy video packet so it ends up in the packet history. Since we
@ -2605,6 +2649,7 @@ TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) {
packet->set_allow_retransmission(true);
packet->SetPayloadSize(kPayloadPacketSize);
packet->set_packet_type(RtpPacketToSend::Type::kVideo);
EXPECT_CALL(send_packet_observer_, OnSendPacket).Times(1);
EXPECT_TRUE(rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()));
// Payload padding not available without RTX, only generate plain padding on
@ -2625,6 +2670,17 @@ TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) {
EXPECT_EQ(packet->payload_size(), 0u);
EXPECT_GT(packet->padding_size(), 0u);
padding_bytes_generated += packet->padding_size();
EXPECT_TRUE(packet->IsExtensionReserved<TransportSequenceNumber>());
EXPECT_TRUE(packet->IsExtensionReserved<AbsoluteSendTime>());
EXPECT_TRUE(packet->IsExtensionReserved<TransmissionOffset>());
// Verify all header extensions are received.
EXPECT_TRUE(rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()));
webrtc::RTPHeader rtp_header;
transport_.last_sent_packet().GetHeader(&rtp_header);
EXPECT_TRUE(rtp_header.extension.hasAbsoluteSendTime);
EXPECT_TRUE(rtp_header.extension.hasTransmissionTimeOffset);
EXPECT_TRUE(rtp_header.extension.hasTransportSequenceNumber);
}
EXPECT_EQ(padding_bytes_generated,