From 4af95849f5c7f59785fd657b6744112df042d85a Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Fri, 6 Apr 2018 11:09:46 -0700 Subject: [PATCH] Always include the MID RTP header extension on every packet when configured This removes the optimization that would stop sending the MID RTP header extension when an RTCP report block is received. The old implementation was not flexible enough for the API, and making those changes is too involved at this time as we need this to work now to unblock other work. Bug: webrtc:4050 Change-Id: I099f8e9047a40993d93bcda9164eb82fdf810387 Reviewed-on: https://webrtc-review.googlesource.com/67192 Reviewed-by: Danil Chapovalov Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#22776} --- modules/rtp_rtcp/BUILD.gn | 3 - modules/rtp_rtcp/source/mid_oracle.cc | 32 --------- modules/rtp_rtcp/source/mid_oracle.h | 58 ---------------- .../rtp_rtcp/source/mid_oracle_unittest.cc | 68 ------------------- modules/rtp_rtcp/source/rtp_sender.cc | 47 ++----------- modules/rtp_rtcp/source/rtp_sender.h | 5 +- 6 files changed, 7 insertions(+), 206 deletions(-) delete mode 100644 modules/rtp_rtcp/source/mid_oracle.cc delete mode 100644 modules/rtp_rtcp/source/mid_oracle.h delete mode 100644 modules/rtp_rtcp/source/mid_oracle_unittest.cc diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index b817e1214e..5ef9e14ade 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -123,8 +123,6 @@ rtc_static_library("rtp_rtcp") { "source/forward_error_correction.h", "source/forward_error_correction_internal.cc", "source/forward_error_correction_internal.h", - "source/mid_oracle.cc", - "source/mid_oracle.h", "source/packet_loss_stats.cc", "source/packet_loss_stats.h", "source/playout_delay_oracle.cc", @@ -337,7 +335,6 @@ if (rtc_include_tests) { "source/flexfec_header_reader_writer_unittest.cc", "source/flexfec_receiver_unittest.cc", "source/flexfec_sender_unittest.cc", - "source/mid_oracle_unittest.cc", "source/nack_rtx_unittest.cc", "source/packet_loss_stats_unittest.cc", "source/playout_delay_oracle_unittest.cc", diff --git a/modules/rtp_rtcp/source/mid_oracle.cc b/modules/rtp_rtcp/source/mid_oracle.cc deleted file mode 100644 index 6d91ad5e3b..0000000000 --- a/modules/rtp_rtcp/source/mid_oracle.cc +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright (c) 2018 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/rtp_rtcp/source/mid_oracle.h" - -namespace webrtc { - -MidOracle::MidOracle(const std::string& mid) : mid_(mid) {} - -MidOracle::~MidOracle() = default; - -void MidOracle::OnReceivedRtcpReportBlocks( - const ReportBlockList& report_blocks) { - if (!send_mid_) { - return; - } - for (const RTCPReportBlock& report_block : report_blocks) { - if (report_block.source_ssrc == ssrc_) { - send_mid_ = false; - break; - } - } -} - -} // namespace webrtc diff --git a/modules/rtp_rtcp/source/mid_oracle.h b/modules/rtp_rtcp/source/mid_oracle.h deleted file mode 100644 index 541ee27753..0000000000 --- a/modules/rtp_rtcp/source/mid_oracle.h +++ /dev/null @@ -1,58 +0,0 @@ -/* - * Copyright (c) 2018 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef MODULES_RTP_RTCP_SOURCE_MID_ORACLE_H_ -#define MODULES_RTP_RTCP_SOURCE_MID_ORACLE_H_ - -#include -#include - -#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "rtc_base/constructormagic.h" - -namespace webrtc { - -// The MidOracle instructs an RTPSender to send the MID header extension on a -// new SSRC stream until it receives an RTCP report block for that stream (which -// implies that the remote side is able to demultiplex it and can remember the -// MID --> SSRC mapping). -class MidOracle { - public: - explicit MidOracle(const std::string& mid); - ~MidOracle(); - - // MID value to put into the header extension. - const std::string& mid() const { return mid_; } - - // True if the MID header extension should be included on the next outgoing - // packet. - bool send_mid() const { return send_mid_; } - - // Change the RTP stream SSRC. This will cause MIDs to be included until an - // RTCP report block lists this SSRC as received. - void SetSsrc(uint32_t ssrc) { - ssrc_ = ssrc; - send_mid_ = true; - } - - // Feedback to decide when to stop sending the MID header extension. - void OnReceivedRtcpReportBlocks(const ReportBlockList& report_blocks); - - private: - const std::string mid_; - bool send_mid_ = false; - uint32_t ssrc_ = 0; - - RTC_DISALLOW_COPY_AND_ASSIGN(MidOracle); -}; - -} // namespace webrtc - -#endif // MODULES_RTP_RTCP_SOURCE_MID_ORACLE_H_ diff --git a/modules/rtp_rtcp/source/mid_oracle_unittest.cc b/modules/rtp_rtcp/source/mid_oracle_unittest.cc deleted file mode 100644 index 06e79307e3..0000000000 --- a/modules/rtp_rtcp/source/mid_oracle_unittest.cc +++ /dev/null @@ -1,68 +0,0 @@ -/* - * Copyright (c) 2018 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/rtp_rtcp/source/mid_oracle.h" - -#include "rtc_base/logging.h" -#include "test/gtest.h" - -namespace { - -using ::webrtc::RTCPReportBlock; -using ::webrtc::MidOracle; - -RTCPReportBlock ReportBlockWithSourceSsrc(uint32_t ssrc) { - RTCPReportBlock report_block; - report_block.source_ssrc = ssrc; - return report_block; -} - -TEST(MidOracleTest, DoNotSendMidInitially) { - MidOracle mid_oracle("mid"); - EXPECT_FALSE(mid_oracle.send_mid()); -} - -TEST(MidOracleTest, SendMidOnceSsrcSet) { - MidOracle mid_oracle("mid"); - mid_oracle.SetSsrc(52); - EXPECT_TRUE(mid_oracle.send_mid()); -} - -TEST(MidOracleTest, IgnoreReportBlockWithUnknownSourceSsrc) { - MidOracle mid_oracle("mid"); - mid_oracle.SetSsrc(52); - mid_oracle.OnReceivedRtcpReportBlocks({ReportBlockWithSourceSsrc(63)}); - EXPECT_TRUE(mid_oracle.send_mid()); -} - -TEST(MidOracleTest, StopSendingMidAfterReceivingRtcpReportWithKnownSourceSsrc) { - constexpr uint32_t kSsrc = 52; - - MidOracle mid_oracle("mid"); - mid_oracle.SetSsrc(kSsrc); - mid_oracle.OnReceivedRtcpReportBlocks({ReportBlockWithSourceSsrc(kSsrc)}); - - EXPECT_FALSE(mid_oracle.send_mid()); -} - -TEST(MidOracleTest, RestartSendingMidWhenSsrcChanges) { - constexpr uint32_t kInitialSsrc = 52; - constexpr uint32_t kChangedSsrc = 63; - - MidOracle mid_oracle("mid"); - mid_oracle.SetSsrc(kInitialSsrc); - mid_oracle.OnReceivedRtcpReportBlocks( - {ReportBlockWithSourceSsrc(kInitialSsrc)}); - mid_oracle.SetSsrc(kChangedSsrc); - - EXPECT_TRUE(mid_oracle.send_mid()); -} - -} // namespace diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index aa133e882f..8db6791522 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -329,9 +329,6 @@ int RTPSender::RtxStatus() const { void RTPSender::SetRtxSsrc(uint32_t ssrc) { rtc::CritScope lock(&send_critsect_); ssrc_rtx_.emplace(ssrc); - if (mid_oracle_rtx_) { - mid_oracle_rtx_->SetSsrc(ssrc); - } } uint32_t RTPSender::RtxSsrc() const { @@ -731,16 +728,6 @@ void RTPSender::OnReceivedNack( void RTPSender::OnReceivedRtcpReportBlocks( const ReportBlockList& report_blocks) { playout_delay_oracle_.OnReceivedRtcpReportBlocks(report_blocks); - - { - rtc::CritScope lock(&send_critsect_); - if (mid_oracle_) { - mid_oracle_->OnReceivedRtcpReportBlocks(report_blocks); - } - if (mid_oracle_rtx_) { - mid_oracle_rtx_->OnReceivedRtcpReportBlocks(report_blocks); - } - } } // Called from pacer when we can send the packet. @@ -1087,9 +1074,9 @@ std::unique_ptr RTPSender::AllocatePacket() const { packet->SetExtension( playout_delay_oracle_.playout_delay()); } - if (mid_oracle_ && mid_oracle_->send_mid()) { + if (!mid_.empty()) { // This is a no-op if the MID header extension is not registered. - packet->SetExtension(mid_oracle_->mid()); + packet->SetExtension(mid_); } return packet; } @@ -1163,9 +1150,6 @@ void RTPSender::SetSSRC(uint32_t ssrc) { if (!sequence_number_forced_) { sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber); } - if (mid_oracle_) { - mid_oracle_->SetSsrc(ssrc); - } } uint32_t RTPSender::SSRC() const { @@ -1177,28 +1161,7 @@ uint32_t RTPSender::SSRC() const { void RTPSender::SetMid(const std::string& mid) { // This is configured via the API. rtc::CritScope lock(&send_critsect_); - - // Cannot change MID once sending. - RTC_DCHECK(!sending_media_); - - // Cannot change the MID if it is already set. - if (mid_oracle_) { - RTC_DCHECK_EQ(mid_oracle_->mid(), mid); - return; - } - if (mid_oracle_rtx_) { - RTC_DCHECK_EQ(mid_oracle_rtx_->mid(), mid); - return; - } - - mid_oracle_ = rtc::MakeUnique(mid); - if (ssrc_) { - mid_oracle_->SetSsrc(*ssrc_); - } - mid_oracle_rtx_ = rtc::MakeUnique(mid); - if (ssrc_rtx_) { - mid_oracle_rtx_->SetSsrc(*ssrc_rtx_); - } + mid_ = mid; } rtc::Optional RTPSender::FlexfecSsrc() const { @@ -1286,9 +1249,9 @@ std::unique_ptr RTPSender::BuildRtxPacket( rtx_packet->SetSsrc(*ssrc_rtx_); // Possibly include the MID header extension. - if (mid_oracle_rtx_ && mid_oracle_rtx_->send_mid()) { + if (!mid_.empty()) { // This is a no-op if the MID header extension is not registered. - rtx_packet->SetExtension(mid_oracle_rtx_->mid()); + rtx_packet->SetExtension(mid_); } } diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index c0af3400c6..4b4b9c616f 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -24,7 +24,6 @@ #include "modules/rtp_rtcp/include/flexfec_sender.h" #include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "modules/rtp_rtcp/source/mid_oracle.h" #include "modules/rtp_rtcp/source/playout_delay_oracle.h" #include "modules/rtp_rtcp/source/rtp_packet_history.h" #include "modules/rtp_rtcp/source/rtp_rtcp_config.h" @@ -321,7 +320,8 @@ class RTPSender { // Must be explicitly set by the application, use of rtc::Optional // only to keep track of correct use. rtc::Optional ssrc_ RTC_GUARDED_BY(send_critsect_); - std::unique_ptr mid_oracle_ RTC_GUARDED_BY(send_critsect_); + // MID value to send in the MID header extension. + std::string mid_ RTC_GUARDED_BY(send_critsect_); uint32_t last_rtp_timestamp_ RTC_GUARDED_BY(send_critsect_); int64_t capture_time_ms_ RTC_GUARDED_BY(send_critsect_); int64_t last_timestamp_time_ms_ RTC_GUARDED_BY(send_critsect_); @@ -330,7 +330,6 @@ class RTPSender { std::vector csrcs_ RTC_GUARDED_BY(send_critsect_); int rtx_ RTC_GUARDED_BY(send_critsect_); rtc::Optional ssrc_rtx_ RTC_GUARDED_BY(send_critsect_); - std::unique_ptr mid_oracle_rtx_ RTC_GUARDED_BY(send_critsect_); // Mapping rtx_payload_type_map_[associated] = rtx. std::map rtx_payload_type_map_ RTC_GUARDED_BY(send_critsect_); size_t rtp_overhead_bytes_per_packet_ RTC_GUARDED_BY(send_critsect_);