From fe7a80c38c2cc023e5cfd96e879c98ffac68888b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Thu, 23 Apr 2015 17:53:17 +0200 Subject: [PATCH] Prevent sender RTCP signals for receive-only channels. Since RTCP packets are delivered to both senders and receivers that correspond the receivers currently log that NACKed packets are missing, since they have no direct connection to the sending side or the RTP packet history. Also preventing triggering on SR requests and PLI/FIR. BUG= R=asapersson@webrtc.org, stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/45249004 Cr-Commit-Position: refs/heads/master@{#9071} --- webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h | 1 + .../source/rtcp_format_remb_unittest.cc | 2 +- .../modules/rtp_rtcp/source/rtcp_receiver.cc | 21 ++++++++++++------- .../modules/rtp_rtcp/source/rtcp_receiver.h | 4 +++- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 4 ++-- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 4 ++-- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 4 +++- webrtc/video_engine/vie_channel.cc | 4 +++- 8 files changed, 29 insertions(+), 15 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h b/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h index 1623554eac..fb47a2bb0f 100644 --- a/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h @@ -54,6 +54,7 @@ class RtpRtcp : public Module { */ int32_t id; bool audio; + bool receiver_only; Clock* clock; ReceiveStatistics* receive_statistics; Transport* outgoing_transport; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc index 3c1a9a3fc1..ea50ffa8dd 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_format_remb_unittest.cc @@ -96,7 +96,7 @@ void RtcpFormatRembTest::SetUp() { dummy_rtp_rtcp_impl_ = new ModuleRtpRtcpImpl(configuration); rtcp_sender_ = new RTCPSender(0, false, system_clock_, receive_statistics_.get(), NULL); - rtcp_receiver_ = new RTCPReceiver(0, system_clock_, NULL, NULL, NULL, + rtcp_receiver_ = new RTCPReceiver(0, system_clock_, false, NULL, NULL, NULL, dummy_rtp_rtcp_impl_); test_transport_ = new TestTransport(rtcp_receiver_); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc index 02f1fa350a..f50ce1782b 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -10,11 +10,12 @@ #include "webrtc/modules/rtp_rtcp/source/rtcp_receiver.h" -#include //assert -#include //memset +#include +#include #include +#include "webrtc/base/checks.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h" #include "webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h" #include "webrtc/system_wrappers/interface/critical_section_wrapper.h" @@ -31,12 +32,14 @@ const int kRrTimeoutIntervals = 3; RTCPReceiver::RTCPReceiver( int32_t id, Clock* clock, + bool receiver_only, RtcpPacketTypeCounterObserver* packet_type_counter_observer, RtcpBandwidthObserver* rtcp_bandwidth_observer, RtcpIntraFrameObserver* rtcp_intra_frame_observer, ModuleRtpRtcpImpl* owner) : TMMBRHelp(), _clock(clock), + receiver_only_(receiver_only), _method(kRtcpOff), _lastReceived(0), _rtpRtcp(*owner), @@ -781,7 +784,7 @@ void RTCPReceiver::HandleSDESChunk(RTCPUtility::RTCPParserV2& rtcpParser) { void RTCPReceiver::HandleNACK(RTCPUtility::RTCPParserV2& rtcpParser, RTCPPacketInformation& rtcpPacketInformation) { const RTCPUtility::RTCPPacket& rtcpPacket = rtcpParser.Packet(); - if (main_ssrc_ != rtcpPacket.NACK.MediaSSRC) { + if (receiver_only_ || main_ssrc_ != rtcpPacket.NACK.MediaSSRC) { // Not to us. rtcpParser.Iterate(); return; @@ -1311,16 +1314,18 @@ void RTCPReceiver::TriggerCallbacksFromRTCPPacket( // Might trigger a OnReceivedBandwidthEstimateUpdate. UpdateTMMBR(); } - unsigned int local_ssrc = 0; + unsigned int local_ssrc; { // We don't want to hold this critsect when triggering the callbacks below. CriticalSectionScoped lock(_criticalSectionRTCPReceiver); local_ssrc = main_ssrc_; } - if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpSrReq) { + if (!receiver_only_ && + rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpSrReq) { _rtpRtcp.OnRequestSendReport(); } - if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpNack) { + if (!receiver_only_ && + rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpNack) { if (rtcpPacketInformation.nackSequenceNumbers.size() > 0) { LOG(LS_VERBOSE) << "Incoming NACK length: " << rtcpPacketInformation.nackSequenceNumbers.size(); @@ -1333,6 +1338,7 @@ void RTCPReceiver::TriggerCallbacksFromRTCPPacket( // report can generate several RTCP packets, based on number relayed/mixed // a send report block should go out to all receivers. if (_cbRtcpIntraFrameObserver) { + DCHECK(!receiver_only_); if ((rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpPli) || (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpFir)) { if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpPli) { @@ -1354,6 +1360,7 @@ void RTCPReceiver::TriggerCallbacksFromRTCPPacket( } } if (_cbRtcpBandwidthObserver) { + DCHECK(!receiver_only_); if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpRemb) { LOG(LS_VERBOSE) << "Incoming REMB: " << rtcpPacketInformation.receiverEstimatedMaxBitrate; @@ -1371,7 +1378,7 @@ void RTCPReceiver::TriggerCallbacksFromRTCPPacket( } } - { + if (!receiver_only_) { CriticalSectionScoped cs(_criticalSectionFeedbacks); if (stats_callback_) { for (ReportBlockList::const_iterator it = diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h index 2086afcae1..569af90130 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h @@ -31,6 +31,7 @@ class RTCPReceiver : public TMMBRHelp public: RTCPReceiver(int32_t id, Clock* clock, + bool receiver_only, RtcpPacketTypeCounterObserver* packet_type_counter_observer, RtcpBandwidthObserver* rtcp_bandwidth_observer, RtcpIntraFrameObserver* rtcp_intra_frame_observer, @@ -231,7 +232,8 @@ protected: uint32_t remote_ssrc, uint32_t source_ssrc) const EXCLUSIVE_LOCKS_REQUIRED(_criticalSectionRTCPReceiver); - Clock* _clock; + Clock* const _clock; + const bool receiver_only_; RTCPMethod _method; int64_t _lastReceived; ModuleRtpRtcpImpl& _rtpRtcp; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index f2ea04be5c..1082b3020b 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -84,8 +84,8 @@ class RtcpReceiverTest : public ::testing::Test { configuration.outgoing_transport = test_transport_; configuration.remote_bitrate_estimator = remote_bitrate_estimator_.get(); rtp_rtcp_impl_ = new ModuleRtpRtcpImpl(configuration); - rtcp_receiver_ = new RTCPReceiver(0, &system_clock_, NULL, NULL, NULL, - rtp_rtcp_impl_); + rtcp_receiver_ = new RTCPReceiver(0, &system_clock_, false, NULL, NULL, + NULL, rtp_rtcp_impl_); test_transport_->SetRTCPReceiver(rtcp_receiver_); } ~RtcpReceiverTest() { diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index a35b7c382f..6def724537 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -303,8 +303,8 @@ class RtcpSenderTest : public ::testing::Test { 0, &clock_, test_transport_, NULL, rtp_payload_registry_.get())); rtcp_sender_ = new RTCPSender(0, false, &clock_, receive_statistics_.get(), NULL); - rtcp_receiver_ = new RTCPReceiver(0, &clock_, NULL, NULL, NULL, - rtp_rtcp_impl_); + rtcp_receiver_ = + new RTCPReceiver(0, &clock_, false, NULL, NULL, NULL, rtp_rtcp_impl_); test_transport_->SetRTCPReceiver(rtcp_receiver_); // Initialize EXPECT_EQ(0, rtcp_sender_->RegisterSendTransport(test_transport_)); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 93b5f545b0..611e0c090e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -29,6 +29,7 @@ namespace webrtc { RtpRtcp::Configuration::Configuration() : id(-1), audio(false), + receiver_only(false), clock(NULL), receive_statistics(NullObjectReceiveStatistics()), outgoing_transport(NULL), @@ -74,6 +75,7 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) configuration.rtcp_packet_type_counter_observer), rtcp_receiver_(configuration.id, configuration.clock, + configuration.receiver_only, configuration.rtcp_packet_type_counter_observer, configuration.bandwidth_callback, configuration.intra_frame_callback, @@ -85,7 +87,7 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) last_process_time_(configuration.clock->TimeInMilliseconds()), last_bitrate_process_time_(configuration.clock->TimeInMilliseconds()), last_rtt_process_time_(configuration.clock->TimeInMilliseconds()), - packet_overhead_(28), // IPV4 UDP. + packet_overhead_(28), // IPV4 UDP. padding_index_(static_cast(-1)), // Start padding at first child. nack_method_(kNackOff), nack_last_time_sent_full_(0), diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index 557f14d9a5..0dbefc32c0 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -1851,8 +1851,10 @@ RtpRtcp::Configuration ViEChannel::CreateRtpRtcpConfiguration() { RtpRtcp::Configuration configuration; configuration.id = ViEModuleId(engine_id_, channel_id_); configuration.audio = false; + configuration.receiver_only = !sender_; configuration.outgoing_transport = &vie_sender_; - configuration.intra_frame_callback = intra_frame_observer_; + if (sender_) + configuration.intra_frame_callback = intra_frame_observer_; configuration.rtt_stats = rtt_stats_; configuration.rtcp_packet_type_counter_observer = &rtcp_packet_type_counter_observer_;