From 5f0b83b7fb6de95f33f20656e969e0280d72c315 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Fri, 18 Mar 2016 15:02:07 -0700 Subject: [PATCH] Enabling rtcp-rsize negotiation and fixing some issues with it. Sending of reduced size RTCP packets should be enabled only if it's enabled in the send parameters (which corresponds to the remote description). Since the RTCPReceiver's RtcpMode isn't used at all, I removed it to ease confusion. BUG=webrtc:4868 R=pbos@webrtc.org, pthatcher@google.com, pthatcher@webrtc.org, stefan@webrtc.org Review URL: https://codereview.webrtc.org/1713493003 . Cr-Commit-Position: refs/heads/master@{#12057} --- webrtc/media/base/mediachannel.h | 6 +++ webrtc/media/engine/webrtcvideoengine2.cc | 39 +++++++++---------- webrtc/media/engine/webrtcvideoengine2.h | 5 ++- .../engine/webrtcvideoengine2_unittest.cc | 6 ++- .../modules/rtp_rtcp/source/rtcp_receiver.cc | 11 ------ .../modules/rtp_rtcp/source/rtcp_receiver.h | 4 -- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 6 +-- webrtc/pc/mediasession.cc | 16 +++----- webrtc/video/video_send_stream.cc | 3 ++ 9 files changed, 42 insertions(+), 54 deletions(-) diff --git a/webrtc/media/base/mediachannel.h b/webrtc/media/base/mediachannel.h index f92a732632..ed565ee5b9 100644 --- a/webrtc/media/base/mediachannel.h +++ b/webrtc/media/base/mediachannel.h @@ -837,6 +837,8 @@ struct RtpParameters { RtcpParameters rtcp; }; +// TODO(deadbeef): Rename to RtpSenderParameters, since they're intended to +// encapsulate all the parameters needed for an RtpSender. template struct RtpSendParameters : RtpParameters { std::string ToString() const override { @@ -934,6 +936,8 @@ class VoiceMediaChannel : public MediaChannel { std::unique_ptr sink) = 0; }; +// TODO(deadbeef): Rename to VideoSenderParameters, since they're intended to +// encapsulate all the parameters needed for a video RtpSender. struct VideoSendParameters : RtpSendParameters { // Use conference mode? This flag comes from the remote // description's SDP line 'a=x-google-flag:conference', copied over @@ -944,6 +948,8 @@ struct VideoSendParameters : RtpSendParameters { bool conference_mode = false; }; +// TODO(deadbeef): Rename to VideoReceiverParameters, since they're intended to +// encapsulate all the parameters needed for a video RtpReceiver. struct VideoRecvParameters : RtpParameters { }; diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index e34056d905..83f81f0f42 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -802,16 +802,18 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) { for (auto& kv : send_streams_) { kv.second->SetSendParameters(changed_params); } - if (changed_params.codec) { - // Update receive feedback parameters from new codec. + if (changed_params.codec || changed_params.rtcp_mode) { + // Update receive feedback parameters from new codec or RTCP mode. LOG(LS_INFO) << "SetFeedbackOptions on all the receive streams because the send " - "codec has changed."; + "codec or RTCP mode has changed."; for (auto& kv : receive_streams_) { RTC_DCHECK(kv.second != nullptr); - kv.second->SetFeedbackParameters(HasNack(send_codec_->codec), - HasRemb(send_codec_->codec), - HasTransportCc(send_codec_->codec)); + kv.second->SetFeedbackParameters( + HasNack(send_codec_->codec), HasRemb(send_codec_->codec), + HasTransportCc(send_codec_->codec), + params.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize + : webrtc::RtcpMode::kCompound); } } } @@ -882,13 +884,6 @@ bool WebRtcVideoChannel2::GetChangedRecvParameters( rtc::Optional>(filtered_extensions); } - // Handle RTCP mode. - if (params.rtcp.reduced_size != recv_params_.rtcp.reduced_size) { - changed_params->rtcp_mode = rtc::Optional( - params.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize - : webrtc::RtcpMode::kCompound); - } - return true; } @@ -1159,7 +1154,12 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp( config->rtp.local_ssrc = rtcp_receiver_report_ssrc_; config->rtp.extensions = recv_rtp_extensions_; - config->rtp.rtcp_mode = recv_params_.rtcp.reduced_size + // Whether or not the receive stream sends reduced size RTCP is determined + // by the send params. + // TODO(deadbeef): Once we change "send_params" to "sender_params" and + // "recv_params" to "receiver_params", we should get this out of + // receiver_params_. + config->rtp.rtcp_mode = send_params_.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize : webrtc::RtcpMode::kCompound; @@ -2294,11 +2294,13 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetLocalSsrc( void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFeedbackParameters( bool nack_enabled, bool remb_enabled, - bool transport_cc_enabled) { + bool transport_cc_enabled, + webrtc::RtcpMode rtcp_mode) { int nack_history_ms = nack_enabled ? kNackHistoryMs : 0; if (config_.rtp.nack.rtp_history_ms == nack_history_ms && config_.rtp.remb == remb_enabled && - config_.rtp.transport_cc == transport_cc_enabled) { + config_.rtp.transport_cc == transport_cc_enabled && + config_.rtp.rtcp_mode == rtcp_mode) { LOG(LS_INFO) << "Ignoring call to SetFeedbackParameters because parameters are " "unchanged; nack=" @@ -2309,6 +2311,7 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFeedbackParameters( config_.rtp.remb = remb_enabled; config_.rtp.nack.rtp_history_ms = nack_history_ms; config_.rtp.transport_cc = transport_cc_enabled; + config_.rtp.rtcp_mode = rtcp_mode; LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetFeedbackParameters; nack=" << nack_enabled << ", remb=" << remb_enabled @@ -2328,10 +2331,6 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRecvParameters( config_.rtp.extensions = *params.rtp_header_extensions; needs_recreation = true; } - if (params.rtcp_mode) { - config_.rtp.rtcp_mode = *params.rtcp_mode; - needs_recreation = true; - } if (needs_recreation) { LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetRecvParameters"; RecreateWebRtcStream(); diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index 850bcaef63..7ffd85343b 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -200,7 +200,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { // These optionals are unset if not changed. rtc::Optional> codec_settings; rtc::Optional> rtp_header_extensions; - rtc::Optional rtcp_mode; }; bool GetChangedSendParameters(const VideoSendParameters& params, @@ -411,9 +410,11 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { const std::vector& GetSsrcs() const; void SetLocalSsrc(uint32_t local_ssrc); + // TODO(deadbeef): Move these feedback parameters into the recv parameters. void SetFeedbackParameters(bool nack_enabled, bool remb_enabled, - bool transport_cc_enabled); + bool transport_cc_enabled, + webrtc::RtcpMode rtcp_mode); void SetRecvParameters(const ChangedRecvParameters& recv_params); void RenderFrame(const webrtc::VideoFrame& frame, diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index b44eb9907d..64c691ff8d 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2575,8 +2575,10 @@ TEST_F(WebRtcVideoChannel2Test, TestSetRecvRtcpReducedSize) { EXPECT_EQ(webrtc::RtcpMode::kCompound, stream1->GetConfig().rtp.rtcp_mode); // Now enable reduced size mode. - recv_parameters_.rtcp.reduced_size = true; - EXPECT_TRUE(channel_->SetRecvParameters(recv_parameters_)); + // TODO(deadbeef): Once "recv_parameters" becomes "receiver_parameters", + // the reduced_size flag should come from that. + send_parameters_.rtcp.reduced_size = true; + EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); stream1 = fake_call_->GetVideoReceiveStreams()[0]; EXPECT_EQ(webrtc::RtcpMode::kReducedSize, stream1->GetConfig().rtp.rtcp_mode); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc index 0873254f63..1bfa7cdebc 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -47,7 +47,6 @@ RTCPReceiver::RTCPReceiver( : TMMBRHelp(), _clock(clock), receiver_only_(receiver_only), - _method(RtcpMode::kOff), _lastReceived(0), _rtpRtcp(*owner), _criticalSectionFeedbacks( @@ -103,16 +102,6 @@ RTCPReceiver::~RTCPReceiver() { } } -RtcpMode RTCPReceiver::Status() const { - CriticalSectionScoped lock(_criticalSectionRTCPReceiver); - return _method; -} - -void RTCPReceiver::SetRTCPStatus(RtcpMode method) { - CriticalSectionScoped lock(_criticalSectionRTCPReceiver); - _method = method; -} - int64_t RTCPReceiver::LastReceived() { CriticalSectionScoped lock(_criticalSectionRTCPReceiver); return _lastReceived; diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h index 1930670064..475ab1e26f 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h @@ -38,9 +38,6 @@ public: ModuleRtpRtcpImpl* owner); virtual ~RTCPReceiver(); - RtcpMode Status() const; - void SetRTCPStatus(RtcpMode method); - int64_t LastReceived(); int64_t LastReceivedReceiverReport() const; @@ -267,7 +264,6 @@ protected: Clock* const _clock; const bool receiver_only_; - RtcpMode _method; int64_t _lastReceived; ModuleRtpRtcpImpl& _rtpRtcp; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index ef720bef52..581aa5111a 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -497,16 +497,12 @@ int32_t ModuleRtpRtcpImpl::SetMaxTransferUnit(const uint16_t mtu) { } RtcpMode ModuleRtpRtcpImpl::RTCP() const { - if (rtcp_sender_.Status() != RtcpMode::kOff) { - return rtcp_receiver_.Status(); - } - return RtcpMode::kOff; + return rtcp_sender_.Status(); } // Configure RTCP status i.e on/off. void ModuleRtpRtcpImpl::SetRTCPStatus(const RtcpMode method) { rtcp_sender_.SetRTCPStatus(method); - rtcp_receiver_.SetRTCPStatus(method); } int32_t ModuleRtpRtcpImpl::SetCNAME(const char* c_name) { diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc index 6a98f7d076..a9d1b95c22 100644 --- a/webrtc/pc/mediasession.cc +++ b/webrtc/pc/mediasession.cc @@ -755,11 +755,9 @@ static bool CreateMediaContentOffer( offer->set_crypto_required(CT_SDES); } offer->set_rtcp_mux(options.rtcp_mux_enabled); - // TODO(deadbeef): Once we're sure this works correctly, enable it in - // CreateOffer. - // if (offer->type() == cricket::MEDIA_TYPE_VIDEO) { - // offer->set_rtcp_reduced_size(true); - // } + if (offer->type() == cricket::MEDIA_TYPE_VIDEO) { + offer->set_rtcp_reduced_size(true); + } offer->set_multistream(options.is_muc); offer->set_rtp_header_extensions(rtp_extensions); @@ -1053,11 +1051,9 @@ static bool CreateMediaContentAnswer( answer->set_rtp_header_extensions(negotiated_rtp_extensions); answer->set_rtcp_mux(options.rtcp_mux_enabled && offer->rtcp_mux()); - // TODO(deadbeef): Once we're sure this works correctly, enable it in - // CreateAnswer. - // if (answer->type() == cricket::MEDIA_TYPE_VIDEO) { - // answer->set_rtcp_reduced_size(offer->rtcp_reduced_size()); - // } + if (answer->type() == cricket::MEDIA_TYPE_VIDEO) { + answer->set_rtcp_reduced_size(offer->rtcp_reduced_size()); + } if (sdes_policy != SEC_DISABLED) { CryptoParams crypto; diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 21a22dc914..6682396fcd 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -69,6 +69,9 @@ std::string VideoSendStream::Config::Rtp::ToString() const { ss << ", "; } ss << ']'; + ss << ", rtcp_mode: " + << (rtcp_mode == RtcpMode::kCompound ? "RtcpMode::kCompound" + : "RtcpMode::kReducedSize"); ss << ", max_packet_size: " << max_packet_size; ss << ", extensions: ["; for (size_t i = 0; i < extensions.size(); ++i) {