From 49456a5b332741e0130884fdbc14676ba5695fde Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 30 Jan 2018 09:56:23 +0100 Subject: [PATCH] Add hack to RtcpTransceiver to mitigate bug in RtcpReceiver of remote endpoint. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:8805 Change-Id: I540ff1d2503ba43723e82800b0bebd322f1af351 Reviewed-on: https://webrtc-review.googlesource.com/44481 Commit-Queue: Danil Chapovalov Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#21802} --- .../rtp_rtcp/source/rtcp_transceiver_config.h | 5 ++ .../rtp_rtcp/source/rtcp_transceiver_impl.cc | 20 +++++-- .../source/rtcp_transceiver_impl_unittest.cc | 57 +++++++++++++++++-- 3 files changed, 73 insertions(+), 9 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_config.h b/modules/rtp_rtcp/source/rtcp_transceiver_config.h index 4b08d89e0e..264b8b30b4 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_config.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver_config.h @@ -93,6 +93,11 @@ struct RtcpTransceiverConfig { // Estimate RTT as non-sender as described in // https://tools.ietf.org/html/rfc3611#section-4.4 and #section-4.5 bool non_sender_rtt_measurement = false; + // Copies LastSR/DelaySinceLastSR for previous report block to avoid + // triggering bug in older version of RtcpReceiver. + // TODO(bugs.webrtc.org/8805): Change to false by default then remove when + // all major webrtc clients updated with the fix in RtcpReceiver. + bool avoid_zero_last_sr_in_last_report_block = true; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc index 2ac1c6bbc4..99977ff8fd 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc @@ -402,15 +402,27 @@ std::vector RtcpTransceiverImpl::CreateReportBlocks( std::vector report_blocks = config_.receive_statistics->RtcpReportBlocks( rtcp::ReceiverReport::kMaxNumberOfReportBlocks); + uint32_t last_sr = 0; + uint32_t last_delay = 0; for (rtcp::ReportBlock& report_block : report_blocks) { auto it = remote_senders_.find(report_block.source_ssrc()); - if (it == remote_senders_.end() || !it->second.last_received_sender_report) + if (it == remote_senders_.end() || + !it->second.last_received_sender_report) { + if (config_.avoid_zero_last_sr_in_last_report_block && last_sr != 0) { + // Simulate behaviour of the RtcpSender to avoid hitting bug in + // RtcpReceiver. + report_block.SetLastSr(last_sr); + report_block.SetDelayLastSr(last_delay); + } continue; + } const SenderReportTimes& last_sender_report = *it->second.last_received_sender_report; - report_block.SetLastSr(CompactNtp(last_sender_report.remote_sent_time)); - report_block.SetDelayLastSr(SaturatedUsToCompactNtp( - now_us - last_sender_report.local_received_time_us)); + last_sr = CompactNtp(last_sender_report.remote_sent_time); + last_delay = SaturatedUsToCompactNtp( + now_us - last_sender_report.local_received_time_us); + report_block.SetLastSr(last_sr); + report_block.SetDelayLastSr(last_delay); } return report_blocks; } diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc index 77d8e0cd3e..5ab6858a88 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc @@ -569,6 +569,7 @@ TEST(RtcpTransceiverImplTest, RtcpTransceiverConfig config; config.schedule_periodic_compound_packets = false; + config.avoid_zero_last_sr_in_last_report_block = false; RtcpPacketParser rtcp_parser; RtcpParserTransport transport(&rtcp_parser); config.outgoing_transport = &transport; @@ -576,9 +577,9 @@ TEST(RtcpTransceiverImplTest, RtcpTransceiverImpl rtcp_transceiver(config); const NtpTime kRemoteNtp(0x9876543211); - // Receive SenderReport for RemoteSsrc2, but no report for RemoteSsrc1. + // Receive SenderReport for RemoteSsrc1, but no report for RemoteSsrc2. SenderReport sr; - sr.SetSenderSsrc(kRemoteSsrc2); + sr.SetSenderSsrc(kRemoteSsrc1); sr.SetNtp(kRemoteNtp); auto raw_packet = sr.Build(); rtcp_transceiver.ReceivePacket(raw_packet, /*now_us=*/0); @@ -593,11 +594,57 @@ TEST(RtcpTransceiverImplTest, // match result of ReceiveStatisticsProvider::RtcpReportBlocks callback, // but for simplicity of the test asume it is the same. ASSERT_EQ(report_blocks[0].source_ssrc(), kRemoteSsrc1); - // No matching Sender Report for kRemoteSsrc1, LastSR fields has to be 0. - EXPECT_EQ(report_blocks[0].last_sr(), 0u); + EXPECT_EQ(report_blocks[0].last_sr(), CompactNtp(kRemoteNtp)); ASSERT_EQ(report_blocks[1].source_ssrc(), kRemoteSsrc2); - EXPECT_EQ(report_blocks[1].last_sr(), CompactNtp(kRemoteNtp)); + // No matching Sender Report for kRemoteSsrc2, LastSR fields has to be 0. + EXPECT_EQ(report_blocks[1].last_sr(), 0u); +} + +TEST(RtcpTransceiverImplTest, AvoidLastReportBlockToHaveZeroLastSrField) { + const uint32_t kRemoteSsrc1 = 54321; + const uint32_t kRemoteSsrc2 = 54323; + MockReceiveStatisticsProvider receive_statistics; + std::vector statistics_report_blocks(2); + statistics_report_blocks[0].SetMediaSsrc(kRemoteSsrc1); + statistics_report_blocks[1].SetMediaSsrc(kRemoteSsrc2); + ON_CALL(receive_statistics, RtcpReportBlocks(_)) + .WillByDefault(Return(statistics_report_blocks)); + + RtcpTransceiverConfig config; + config.schedule_periodic_compound_packets = false; + config.avoid_zero_last_sr_in_last_report_block = true; + RtcpPacketParser rtcp_parser; + RtcpParserTransport transport(&rtcp_parser); + config.outgoing_transport = &transport; + config.receive_statistics = &receive_statistics; + RtcpTransceiverImpl rtcp_transceiver(config); + + const NtpTime kRemoteNtp(0x9876543211); + // Receive SenderReport for RemoteSsrc1, but no report for RemoteSsrc2. + SenderReport sr; + sr.SetSenderSsrc(kRemoteSsrc1); + sr.SetNtp(kRemoteNtp); + auto raw_packet = sr.Build(); + rtcp_transceiver.ReceivePacket(raw_packet, /*now_us=*/0); + + // Trigger sending ReceiverReport. + rtcp_transceiver.SendCompoundPacket(); + + EXPECT_GT(rtcp_parser.receiver_report()->num_packets(), 0); + const auto& report_blocks = rtcp_parser.receiver_report()->report_blocks(); + ASSERT_EQ(report_blocks.size(), 2u); + // RtcpTransceiverImpl doesn't guarantee order of the report blocks + // match result of ReceiveStatisticsProvider::RtcpReportBlocks callback, + // but for simplicity of the test asume it is the same. + ASSERT_EQ(report_blocks[0].source_ssrc(), kRemoteSsrc1); + EXPECT_NE(report_blocks[0].last_sr(), 0u); + + ASSERT_EQ(report_blocks[1].source_ssrc(), kRemoteSsrc2); + // No Sender Report for kRemoteSsrc2, use same LastSR as for kRemoteSsrc1 + EXPECT_EQ(report_blocks[1].last_sr(), report_blocks[0].last_sr()); + EXPECT_EQ(report_blocks[1].delay_since_last_sr(), + report_blocks[0].delay_since_last_sr()); } TEST(RtcpTransceiverImplTest,