Adding reduced size RTCP configuration down to the video stream level. Still waiting to turn on negotiation (in mediasession.cc) until we verify it's working as expected. BUG=webrtc:4868 Review URL: https://codereview.webrtc.org/1418123003 Cr-Commit-Position: refs/heads/master@{#10958}
diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index fceaaf4..bc5dc1d 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc
@@ -122,6 +122,7 @@ static const char kAttributeGroup[] = "group"; static const char kAttributeMid[] = "mid"; static const char kAttributeRtcpMux[] = "rtcp-mux"; +static const char kAttributeRtcpReducedSize[] = "rtcp-rsize"; static const char kAttributeSsrc[] = "ssrc"; static const char kSsrcAttributeCname[] = "cname"; static const char kAttributeExtmap[] = "extmap"; @@ -1400,6 +1401,13 @@ AddLine(os.str(), message); } + // RFC 5506 + // a=rtcp-rsize + if (media_desc->rtcp_reduced_size()) { + InitAttrLine(kAttributeRtcpReducedSize, &os); + AddLine(os.str(), message); + } + // RFC 4568 // a=crypto:<tag> <crypto-suite> <key-params> [<session-params>] for (std::vector<CryptoParams>::const_iterator it = @@ -2553,6 +2561,8 @@ // if (HasAttribute(line, kAttributeRtcpMux)) { media_desc->set_rtcp_mux(true); + } else if (HasAttribute(line, kAttributeRtcpReducedSize)) { + media_desc->set_rtcp_reduced_size(true); } else if (HasAttribute(line, kAttributeSsrcGroup)) { if (!ParseSsrcGroupAttribute(line, &ssrc_groups, error)) { return false;
diff --git a/talk/app/webrtc/webrtcsdp_unittest.cc b/talk/app/webrtc/webrtcsdp_unittest.cc index cb6a392..fb55e31 100644 --- a/talk/app/webrtc/webrtcsdp_unittest.cc +++ b/talk/app/webrtc/webrtcsdp_unittest.cc
@@ -153,6 +153,7 @@ "a=mid:audio_content_name\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" + "a=rtcp-rsize\r\n" "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32 " "dummy_session_params\r\n" @@ -220,6 +221,7 @@ "a=mid:audio_content_name\r\n" "a=sendrecv\r\n" "a=rtcp-mux\r\n" + "a=rtcp-rsize\r\n" "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32 " "dummy_session_params\r\n" @@ -704,6 +706,7 @@ AudioContentDescription* CreateAudioContentDescription() { AudioContentDescription* audio = new AudioContentDescription(); audio->set_rtcp_mux(true); + audio->set_rtcp_reduced_size(true); StreamParams audio_stream1; audio_stream1.id = kAudioTrackId1; audio_stream1.cname = kStream1Cname; @@ -735,6 +738,9 @@ // rtcp_mux EXPECT_EQ(cd1->rtcp_mux(), cd2->rtcp_mux()); + // rtcp_reduced_size + EXPECT_EQ(cd1->rtcp_reduced_size(), cd2->rtcp_reduced_size()); + // cryptos EXPECT_EQ(cd1->cryptos().size(), cd2->cryptos().size()); if (cd1->cryptos().size() != cd2->cryptos().size()) {
diff --git a/talk/media/base/mediachannel.h b/talk/media/base/mediachannel.h index 92d0fcb..fe223bb 100644 --- a/talk/media/base/mediachannel.h +++ b/talk/media/base/mediachannel.h
@@ -927,6 +927,10 @@ std::vector<DataReceiverInfo> receivers; }; +struct RtcpParameters { + bool reduced_size = false; +}; + template <class Codec> struct RtpParameters { virtual std::string ToString() const { @@ -941,6 +945,7 @@ std::vector<Codec> codecs; std::vector<RtpHeaderExtension> extensions; // TODO(pthatcher): Add streams. + RtcpParameters rtcp; }; template <class Codec, class Options>
diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index 3f9e8ce..6b699fe 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc
@@ -792,10 +792,20 @@ LOG(LS_INFO) << "SetSendParameters: " << params.ToString(); // TODO(pbos): Refactor this to only recreate the send streams once // instead of 4 times. - return (SetSendCodecs(params.codecs) && - SetSendRtpHeaderExtensions(params.extensions) && - SetMaxSendBandwidth(params.max_bandwidth_bps) && - SetOptions(params.options)); + if (!SetSendCodecs(params.codecs) || + !SetSendRtpHeaderExtensions(params.extensions) || + !SetMaxSendBandwidth(params.max_bandwidth_bps) || + !SetOptions(params.options)) { + return false; + } + if (send_params_.rtcp.reduced_size != params.rtcp.reduced_size) { + rtc::CritScope stream_lock(&stream_crit_); + for (auto& kv : send_streams_) { + kv.second->SetSendParameters(params); + } + } + send_params_ = params; + return true; } bool WebRtcVideoChannel2::SetRecvParameters(const VideoRecvParameters& params) { @@ -803,8 +813,18 @@ LOG(LS_INFO) << "SetRecvParameters: " << params.ToString(); // TODO(pbos): Refactor this to only recreate the recv streams once // instead of twice. - return (SetRecvCodecs(params.codecs) && - SetRecvRtpHeaderExtensions(params.extensions)); + if (!SetRecvCodecs(params.codecs) || + !SetRecvRtpHeaderExtensions(params.extensions)) { + return false; + } + if (recv_params_.rtcp.reduced_size != params.rtcp.reduced_size) { + rtc::CritScope stream_lock(&stream_crit_); + for (auto& kv : receive_streams_) { + kv.second->SetRecvParameters(params); + } + } + recv_params_ = params; + return true; } std::string WebRtcVideoChannel2::CodecSettingsVectorToString( @@ -1025,15 +1045,10 @@ webrtc::VideoSendStream::Config config(this); config.overuse_callback = this; - WebRtcVideoSendStream* stream = - new WebRtcVideoSendStream(call_, - sp, - config, - external_encoder_factory_, - options_, - bitrate_config_.max_bitrate_bps, - send_codec_, - send_rtp_extensions_); + WebRtcVideoSendStream* stream = new WebRtcVideoSendStream( + call_, sp, config, external_encoder_factory_, options_, + bitrate_config_.max_bitrate_bps, send_codec_, send_rtp_extensions_, + send_params_); uint32_t ssrc = sp.first_ssrc(); RTC_DCHECK(ssrc != 0); @@ -1175,6 +1190,9 @@ config->rtp.local_ssrc = rtcp_receiver_report_ssrc_; config->rtp.extensions = recv_rtp_extensions_; + config->rtp.rtcp_mode = recv_params_.rtcp.reduced_size + ? webrtc::RtcpMode::kReducedSize + : webrtc::RtcpMode::kCompound; // TODO(pbos): This protection is against setting the same local ssrc as // remote which is not permitted by the lower-level API. RTCP requires a @@ -1661,7 +1679,10 @@ const VideoOptions& options, int max_bitrate_bps, const rtc::Optional<VideoCodecSettings>& codec_settings, - const std::vector<webrtc::RtpExtension>& rtp_extensions) + const std::vector<webrtc::RtpExtension>& rtp_extensions, + // TODO(deadbeef): Don't duplicate information between send_params, + // rtp_extensions, options, etc. + const VideoSendParameters& send_params) : ssrcs_(sp.ssrcs), ssrc_groups_(sp.ssrc_groups), call_(call), @@ -1682,6 +1703,9 @@ ¶meters_.config.rtp.rtx.ssrcs); parameters_.config.rtp.c_name = sp.cname; parameters_.config.rtp.extensions = rtp_extensions; + parameters_.config.rtp.rtcp_mode = send_params.rtcp.reduced_size + ? webrtc::RtcpMode::kReducedSize + : webrtc::RtcpMode::kCompound; if (codec_settings) { SetCodec(*codec_settings); @@ -1998,6 +2022,18 @@ } } +void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSendParameters( + const VideoSendParameters& send_params) { + rtc::CritScope cs(&lock_); + parameters_.config.rtp.rtcp_mode = send_params.rtcp.reduced_size + ? webrtc::RtcpMode::kReducedSize + : webrtc::RtcpMode::kCompound; + if (stream_ != nullptr) { + LOG(LS_INFO) << "RecreateWebRtcStream (send) because of SetSendParameters"; + RecreateWebRtcStream(); + } +} + webrtc::VideoEncoderConfig WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoderConfig( const Dimensions& dimensions, @@ -2435,6 +2471,15 @@ RecreateWebRtcStream(); } +void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRecvParameters( + const VideoRecvParameters& recv_params) { + config_.rtp.rtcp_mode = recv_params.rtcp.reduced_size + ? webrtc::RtcpMode::kReducedSize + : webrtc::RtcpMode::kCompound; + LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetRecvParameters"; + RecreateWebRtcStream(); +} + void WebRtcVideoChannel2::WebRtcVideoReceiveStream::RecreateWebRtcStream() { if (stream_ != NULL) { call_->DestroyVideoReceiveStream(stream_);
diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index e6e26e7..155790c 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h
@@ -249,13 +249,17 @@ const VideoOptions& options, int max_bitrate_bps, const rtc::Optional<VideoCodecSettings>& codec_settings, - const std::vector<webrtc::RtpExtension>& rtp_extensions); + const std::vector<webrtc::RtpExtension>& rtp_extensions, + const VideoSendParameters& send_params); ~WebRtcVideoSendStream(); void SetOptions(const VideoOptions& options); void SetCodec(const VideoCodecSettings& codec); void SetRtpExtensions( const std::vector<webrtc::RtpExtension>& rtp_extensions); + // TODO(deadbeef): Move logic from SetCodec/SetRtpExtensions/etc. + // into this method. Currently this method only sets the RTCP mode. + void SetSendParameters(const VideoSendParameters& send_params); void InputFrame(VideoCapturer* capturer, const VideoFrame* frame); bool SetCapturer(VideoCapturer* capturer); @@ -405,6 +409,9 @@ bool transport_cc_enabled); void SetRecvCodecs(const std::vector<VideoCodecSettings>& recv_codecs); void SetRtpExtensions(const std::vector<webrtc::RtpExtension>& extensions); + // TODO(deadbeef): Move logic from SetRecvCodecs/SetRtpExtensions/etc. + // into this method. Currently this method only sets the RTCP mode. + void SetRecvParameters(const VideoRecvParameters& recv_params); void RenderFrame(const webrtc::VideoFrame& frame, int time_to_render_ms) override; @@ -525,6 +532,10 @@ std::vector<webrtc::RtpExtension> recv_rtp_extensions_; webrtc::Call::Config::BitrateConfig bitrate_config_; VideoOptions options_; + // TODO(deadbeef): Don't duplicate information between + // send_params/recv_params, rtp_extensions, options, etc. + VideoSendParameters send_params_; + VideoRecvParameters recv_params_; }; } // namespace cricket
diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 1de2754..24e028e 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
@@ -2418,6 +2418,42 @@ channel_->SetInterface(NULL); } +// This test verifies that the RTCP reduced size mode is properly applied to +// send video streams. +TEST_F(WebRtcVideoChannel2Test, TestSetSendRtcpReducedSize) { + // Create stream, expecting that default mode is "compound". + FakeVideoSendStream* stream1 = AddSendStream(); + EXPECT_EQ(webrtc::RtcpMode::kCompound, stream1->GetConfig().rtp.rtcp_mode); + + // Now enable reduced size mode. + send_parameters_.rtcp.reduced_size = true; + EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); + stream1 = fake_call_->GetVideoSendStreams()[0]; + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, stream1->GetConfig().rtp.rtcp_mode); + + // Create a new stream and ensure it picks up the reduced size mode. + FakeVideoSendStream* stream2 = AddSendStream(); + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, stream2->GetConfig().rtp.rtcp_mode); +} + +// This test verifies that the RTCP reduced size mode is properly applied to +// receive video streams. +TEST_F(WebRtcVideoChannel2Test, TestSetRecvRtcpReducedSize) { + // Create stream, expecting that default mode is "compound". + FakeVideoReceiveStream* stream1 = AddRecvStream(); + 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_)); + stream1 = fake_call_->GetVideoReceiveStreams()[0]; + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, stream1->GetConfig().rtp.rtcp_mode); + + // Create a new stream and ensure it picks up the reduced size mode. + FakeVideoReceiveStream* stream2 = AddRecvStream(); + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, stream2->GetConfig().rtp.rtcp_mode); +} + TEST_F(WebRtcVideoChannel2Test, OnReadyToSendSignalsNetworkState) { EXPECT_EQ(webrtc::kNetworkUp, fake_call_->GetNetworkState());
diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc index f4c0e81..588a036 100644 --- a/talk/session/media/channel.cc +++ b/talk/session/media/channel.cc
@@ -152,6 +152,7 @@ if (desc->rtp_header_extensions_set()) { params->extensions = desc->rtp_header_extensions(); } + params->rtcp.reduced_size = desc->rtcp_reduced_size(); } template <class Codec, class Options>
diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc index 0e9730c..8db37d0 100644 --- a/talk/session/media/mediasession.cc +++ b/talk/session/media/mediasession.cc
@@ -760,6 +760,11 @@ 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); + // } offer->set_multistream(options.is_muc); offer->set_rtp_header_extensions(rtp_extensions); @@ -1038,6 +1043,11 @@ 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 (sdes_policy != SEC_DISABLED) { CryptoParams crypto;
diff --git a/talk/session/media/mediasession.h b/talk/session/media/mediasession.h index 25ea017..8a7f4cc 100644 --- a/talk/session/media/mediasession.h +++ b/talk/session/media/mediasession.h
@@ -167,17 +167,7 @@ // "content" (as used in XEP-0166) descriptions for voice and video. class MediaContentDescription : public ContentDescription { public: - MediaContentDescription() - : rtcp_mux_(false), - bandwidth_(kAutoBandwidth), - crypto_required_(CT_NONE), - rtp_header_extensions_set_(false), - multistream_(false), - conference_mode_(false), - partial_(false), - buffered_mode_latency_(kBufferedModeDisabled), - direction_(MD_SENDRECV) { - } + MediaContentDescription() {} virtual MediaType type() const = 0; virtual bool has_codecs() const = 0; @@ -195,6 +185,11 @@ bool rtcp_mux() const { return rtcp_mux_; } void set_rtcp_mux(bool mux) { rtcp_mux_ = mux; } + bool rtcp_reduced_size() const { return rtcp_reduced_size_; } + void set_rtcp_reduced_size(bool reduced_size) { + rtcp_reduced_size_ = reduced_size; + } + int bandwidth() const { return bandwidth_; } void set_bandwidth(int bandwidth) { bandwidth_ = bandwidth; } @@ -291,19 +286,20 @@ int buffered_mode_latency() const { return buffered_mode_latency_; } protected: - bool rtcp_mux_; - int bandwidth_; + bool rtcp_mux_ = false; + bool rtcp_reduced_size_ = false; + int bandwidth_ = kAutoBandwidth; std::string protocol_; std::vector<CryptoParams> cryptos_; - CryptoType crypto_required_; + CryptoType crypto_required_ = CT_NONE; std::vector<RtpHeaderExtension> rtp_header_extensions_; - bool rtp_header_extensions_set_; - bool multistream_; + bool rtp_header_extensions_set_ = false; + bool multistream_ = false; StreamParamsVec streams_; - bool conference_mode_; - bool partial_; - int buffered_mode_latency_; - MediaContentDirection direction_; + bool conference_mode_ = false; + bool partial_ = false; + int buffered_mode_latency_ = kBufferedModeDisabled; + MediaContentDirection direction_ = MD_SENDRECV; }; template <class C>
diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index d73ef9c..656d551 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc
@@ -516,7 +516,7 @@ // When it goes down, disable RTCP afterwards. This ensures that any packets // sent due to the network state changed will not be dropped. if (state == kNetworkUp) - vie_channel_->SetRTCPMode(RtcpMode::kCompound); + vie_channel_->SetRTCPMode(config_.rtp.rtcp_mode); vie_encoder_->SetNetworkTransmissionState(state == kNetworkUp); if (state == kNetworkDown) vie_channel_->SetRTCPMode(RtcpMode::kOff);
diff --git a/webrtc/video_send_stream.h b/webrtc/video_send_stream.h index 0c0af80..d0e0c93 100644 --- a/webrtc/video_send_stream.h +++ b/webrtc/video_send_stream.h
@@ -100,6 +100,9 @@ std::vector<uint32_t> ssrcs; + // See RtcpMode for description. + RtcpMode rtcp_mode = RtcpMode::kCompound; + // Max RTP packet size delivered to send transport from VideoEngine. size_t max_packet_size = kDefaultMaxPacketSize;