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

Fix potential crash if nack is being processed when media gets disabled

Browse Source 
This is a race that can happen if a nack arrives before media is
disabled, but the packet is not processed until after the disabling
is complete.

Bug: webrtc:10633, b/138636698
Change-Id: Ic90462b815163ab58c324e5cdb95c8d199c0b772
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147277
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28718}
This commit is contained in:
Erik Språng 2019-07-30 22:03:49 +02:00 committed by Commit Bot
parent f034b86463
commit 60ffc31ae1
2 changed files with 56 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -620,8 +620,10 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id) {
retransmit_packet =
absl::make_unique<RtpPacketToSend>(stored_packet);
}
retransmit_packet->set_retransmitted_sequence_number(
stored_packet.SequenceNumber());
if (retransmit_packet) {
retransmit_packet->set_retransmitted_sequence_number(
stored_packet.SequenceNumber());
}
return retransmit_packet;
});
if (!packet) {

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

@ -278,6 +278,7 @@ class RtpSenderTest : public ::testing::TestWithParam<TestConfig> {
int64_t capture_time_ms) {
auto packet = rtp_sender_->AllocatePacket();
packet->SetPayloadType(payload_type);
packet->set_packet_type(RtpPacketToSend::Type::kVideo);
packet->SetMarker(marker_bit);
packet->SetTimestamp(timestamp);
packet->set_capture_time_ms(capture_time_ms);
@ -294,8 +295,7 @@ class RtpSenderTest : public ::testing::TestWithParam<TestConfig> {
// Packet should be stored in a send bucket.
EXPECT_TRUE(rtp_sender_->SendToNetwork(
absl::make_unique<RtpPacketToSend>(*packet), kAllowRetransmission,
RtpPacketSender::kNormalPriority));
absl::make_unique<RtpPacketToSend>(*packet), kAllowRetransmission));
return packet;
}
@ -2943,6 +2943,56 @@ TEST_P(RtpSenderTestWithoutPacer, ClearHistoryOnSequenceNumberCange) {
EXPECT_EQ(rtp_sender_->ReSendPacket(packet_seqence_number), 0);
}
TEST_P(RtpSenderTest, IgnoresNackAfterDisablingMedia) {
const int64_t kRtt = 10;
rtp_sender_->SetSendingMediaStatus(true);
rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads);
rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload);
rtp_sender_->SetStorePacketsStatus(true, 10);
rtp_sender_->SetRtt(kRtt);
// Send a packet so it is in the packet history.
if (GetParam().pacer_references_packets) {
EXPECT_CALL(mock_paced_sender_, InsertPacket);
SendGenericPacket();
rtp_sender_->TimeToSendPacket(kSsrc, kSeqNum,
fake_clock_.TimeInMilliseconds(), false,
PacedPacketInfo());
ASSERT_EQ(1u, transport_.sent_packets_.size());
// Disable media sending and try to retransmit the packet, it should be put
// in the pacer queue.
rtp_sender_->SetSendingMediaStatus(false);
fake_clock_.AdvanceTimeMilliseconds(kRtt);
EXPECT_CALL(mock_paced_sender_, InsertPacket);
EXPECT_GT(rtp_sender_->ReSendPacket(kSeqNum), 0);
// Time to send the retransmission. It should fail and the send packet
// counter should not increase.
rtp_sender_->TimeToSendPacket(kSsrc, kSeqNum,
fake_clock_.TimeInMilliseconds(), true,
PacedPacketInfo());
ASSERT_EQ(1u, transport_.sent_packets_.size());
} else {
std::unique_ptr<RtpPacketToSend> packet_to_pace;
EXPECT_CALL(mock_paced_sender_, EnqueuePacket)
.WillOnce([&](std::unique_ptr<RtpPacketToSend> packet) {
packet_to_pace = std::move(packet);
});
SendGenericPacket();
rtp_sender_->TrySendPacket(packet_to_pace.get(), PacedPacketInfo());
ASSERT_EQ(1u, transport_.sent_packets_.size());
// Disable media sending and try to retransmit the packet, it should fail.
rtp_sender_->SetSendingMediaStatus(false);
fake_clock_.AdvanceTimeMilliseconds(kRtt);
EXPECT_LT(rtp_sender_->ReSendPacket(kSeqNum), 0);
}
}
INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead,
RtpSenderTest,
::testing::Values(TestConfig{false, false},