From fb2aceded614a34311f15396a27fdfacc36bf384 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 6 Jan 2017 23:05:37 -0800 Subject: [PATCH] Add video send SSRC to RtpParameters, and don't allow changing SSRC. With this, RtpSender and RtpReceiver will always return an SSRC if one is available. Also, attempts to change the SSRC with SetParameters will fail, rather than silently doing nothing. BUG=webrtc:6888 Review-Url: https://codereview.webrtc.org/2567333004 Cr-Commit-Position: refs/heads/master@{#15939} --- webrtc/media/engine/webrtcvideoengine2.cc | 10 ++++++ .../engine/webrtcvideoengine2_unittest.cc | 31 +++++++++++++---- webrtc/media/engine/webrtcvoiceengine.cc | 33 +++++++++---------- .../engine/webrtcvoiceengine_unittest.cc | 18 ++++++---- 4 files changed, 62 insertions(+), 30 deletions(-) diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 129eb3288a..335ee3a691 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -1560,6 +1560,11 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream( sp.GetPrimarySsrcs(¶meters_.config.rtp.ssrcs); + // ValidateStreamParams should prevent this from happening. + RTC_CHECK(!parameters_.config.rtp.ssrcs.empty()); + rtp_parameters_.encodings[0].ssrc = + rtc::Optional(parameters_.config.rtp.ssrcs[0]); + // RTX. sp.GetFidSsrcs(parameters_.config.rtp.ssrcs, ¶meters_.config.rtp.rtx.ssrcs); @@ -1862,11 +1867,16 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::GetRtpParameters() const { bool WebRtcVideoChannel2::WebRtcVideoSendStream::ValidateRtpParameters( const webrtc::RtpParameters& rtp_parameters) { + RTC_DCHECK_RUN_ON(&thread_checker_); if (rtp_parameters.encodings.size() != 1) { LOG(LS_ERROR) << "Attempted to set RtpParameters without exactly one encoding"; return false; } + if (rtp_parameters.encodings[0].ssrc != rtp_parameters_.encodings[0].ssrc) { + LOG(LS_ERROR) << "Attempted to set RtpParameters with modified SSRC"; + return false; + } return true; } diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index b8495b0546..5944883e1c 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -3661,16 +3661,21 @@ TEST_F(WebRtcVideoChannel2Test, // for each encoding individually. AddSendStream(); - // Setting RtpParameters with no encoding is expected to fail. webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(last_ssrc_); - parameters.encodings.clear(); - EXPECT_FALSE(channel_->SetRtpSendParameters(last_ssrc_, parameters)); - // Setting RtpParameters with exactly one encoding should succeed. - parameters.encodings.push_back(webrtc::RtpEncodingParameters()); - EXPECT_TRUE(channel_->SetRtpSendParameters(last_ssrc_, parameters)); // Two or more encodings should result in failure. parameters.encodings.push_back(webrtc::RtpEncodingParameters()); EXPECT_FALSE(channel_->SetRtpSendParameters(last_ssrc_, parameters)); + // Zero encodings should also fail. + parameters.encodings.clear(); + EXPECT_FALSE(channel_->SetRtpSendParameters(last_ssrc_, parameters)); +} + +// Changing the SSRC through RtpParameters is not allowed. +TEST_F(WebRtcVideoChannel2Test, CannotSetSsrcInRtpSendParameters) { + AddSendStream(); + webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(last_ssrc_); + parameters.encodings[0].ssrc = rtc::Optional(0xdeadbeef); + EXPECT_FALSE(channel_->SetRtpSendParameters(last_ssrc_, parameters)); } // Test that a stream will not be sending if its encoding is made inactive @@ -3753,6 +3758,18 @@ TEST_F(WebRtcVideoChannel2Test, GetRtpSendParametersCodecs) { rtp_parameters.codecs[1]); } +// Test that RtpParameters for send stream has one encoding and it has +// the correct SSRC. +TEST_F(WebRtcVideoChannel2Test, GetRtpSendParametersSsrc) { + AddSendStream(); + + webrtc::RtpParameters rtp_parameters = + channel_->GetRtpSendParameters(last_ssrc_); + ASSERT_EQ(1u, rtp_parameters.encodings.size()); + EXPECT_EQ(rtc::Optional(last_ssrc_), + rtp_parameters.encodings[0].ssrc); +} + // Test that if we set/get parameters multiple times, we get the same results. TEST_F(WebRtcVideoChannel2Test, SetAndGetRtpSendParameters) { AddSendStream(); @@ -3826,7 +3843,7 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_GetRtpReceiveFmtpSprop) { // Test that RtpParameters for receive stream has one encoding and it has // the correct SSRC. -TEST_F(WebRtcVideoChannel2Test, RtpEncodingParametersSsrcIsSet) { +TEST_F(WebRtcVideoChannel2Test, GetRtpReceiveParametersSsrc) { AddRecvStream(); webrtc::RtpParameters rtp_parameters = diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc index a36f328705..9871d3a21f 100644 --- a/webrtc/media/engine/webrtcvoiceengine.cc +++ b/webrtc/media/engine/webrtcvoiceengine.cc @@ -1384,8 +1384,23 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream return rtp_parameters_; } + bool ValidateRtpParameters(const webrtc::RtpParameters& rtp_parameters) { + if (rtp_parameters.encodings.size() != 1) { + LOG(LS_ERROR) + << "Attempted to set RtpParameters without exactly one encoding"; + return false; + } + if (rtp_parameters.encodings[0].ssrc != rtp_parameters_.encodings[0].ssrc) { + LOG(LS_ERROR) << "Attempted to set RtpParameters with modified SSRC"; + return false; + } + return true; + } + bool SetRtpParameters(const webrtc::RtpParameters& parameters) { - RTC_CHECK_EQ(1UL, parameters.encodings.size()); + if (!ValidateRtpParameters(parameters)) { + return false; + } auto send_rate = ComputeSendBitrate(max_send_bitrate_bps_, parameters.encodings[0].max_bitrate_bps, send_codec_spec_.codec_inst); @@ -1704,9 +1719,6 @@ bool WebRtcVoiceMediaChannel::SetRtpSendParameters( uint32_t ssrc, const webrtc::RtpParameters& parameters) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - if (!ValidateRtpParameters(parameters)) { - return false; - } auto it = send_streams_.find(ssrc); if (it == send_streams_.end()) { LOG(LS_WARNING) << "Attempting to set RTP send parameters for stream " @@ -1760,9 +1772,6 @@ bool WebRtcVoiceMediaChannel::SetRtpReceiveParameters( uint32_t ssrc, const webrtc::RtpParameters& parameters) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - if (!ValidateRtpParameters(parameters)) { - return false; - } auto it = recv_streams_.find(ssrc); if (it == recv_streams_.end()) { LOG(LS_WARNING) << "Attempting to set RTP receive parameters for stream " @@ -1779,16 +1788,6 @@ bool WebRtcVoiceMediaChannel::SetRtpReceiveParameters( return true; } -bool WebRtcVoiceMediaChannel::ValidateRtpParameters( - const webrtc::RtpParameters& rtp_parameters) { - if (rtp_parameters.encodings.size() != 1) { - LOG(LS_ERROR) - << "Attempted to set RtpParameters without exactly one encoding"; - return false; - } - return true; -} - bool WebRtcVoiceMediaChannel::SetOptions(const AudioOptions& options) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); LOG(LS_INFO) << "Setting voice channel options: " diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc index 3ca7ec8b05..0cfbeab42b 100644 --- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc +++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc @@ -1039,15 +1039,21 @@ TEST_F(WebRtcVoiceEngineTestFake, // for each encoding individually. EXPECT_TRUE(SetupSendStream()); - // Setting RtpParameters with no encoding is expected to fail. - webrtc::RtpParameters parameters; - EXPECT_FALSE(channel_->SetRtpSendParameters(kSsrc1, parameters)); - // Setting RtpParameters with exactly one encoding should succeed. - parameters.encodings.push_back(webrtc::RtpEncodingParameters()); - EXPECT_TRUE(channel_->SetRtpSendParameters(kSsrc1, parameters)); + webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(kSsrc1); // Two or more encodings should result in failure. parameters.encodings.push_back(webrtc::RtpEncodingParameters()); EXPECT_FALSE(channel_->SetRtpSendParameters(kSsrc1, parameters)); + // Zero encodings should also fail. + parameters.encodings.clear(); + EXPECT_FALSE(channel_->SetRtpSendParameters(kSsrc1, parameters)); +} + +// Changing the SSRC through RtpParameters is not allowed. +TEST_F(WebRtcVoiceEngineTestFake, CannotSetSsrcInRtpSendParameters) { + EXPECT_TRUE(SetupSendStream()); + webrtc::RtpParameters parameters = channel_->GetRtpSendParameters(kSsrc1); + parameters.encodings[0].ssrc = rtc::Optional(0xdeadbeef); + EXPECT_FALSE(channel_->SetRtpSendParameters(kSsrc1, parameters)); } // Test that a stream will not be sending if its encoding is made