From d6f4c25eedcfd502920f1b2a24744badd9da80be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Thu, 26 Mar 2015 16:23:04 +0100 Subject: [PATCH] Reject streams reusing simulcast or RTX SSRCs. BUG=1788, chromium:470122, chromium:470856 R=stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/42919004 Cr-Commit-Position: refs/heads/master@{#8868} --- talk/media/webrtc/webrtcvideoengine2.cc | 94 ++++++++++++++----- talk/media/webrtc/webrtcvideoengine2.h | 17 +++- .../webrtc/webrtcvideoengine2_unittest.cc | 53 +++++++++++ 3 files changed, 141 insertions(+), 23 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index af73702c1..fec80678b 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -839,20 +839,41 @@ bool WebRtcVideoChannel2::SetSend(bool send) { return true; } +bool WebRtcVideoChannel2::ValidateSendSsrcAvailability( + const StreamParams& sp) const { + for (uint32_t ssrc: sp.ssrcs) { + if (send_ssrcs_.find(ssrc) != send_ssrcs_.end()) { + LOG(LS_ERROR) << "Send stream with SSRC '" << ssrc << "' already exists."; + return false; + } + } + return true; +} + +bool WebRtcVideoChannel2::ValidateReceiveSsrcAvailability( + const StreamParams& sp) const { + for (uint32_t ssrc: sp.ssrcs) { + if (receive_ssrcs_.find(ssrc) != receive_ssrcs_.end()) { + LOG(LS_ERROR) << "Receive stream with SSRC '" << ssrc + << "' already exists."; + return false; + } + } + return true; +} + bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) { LOG(LS_INFO) << "AddSendStream: " << sp.ToString(); if (!ValidateStreamParams(sp)) return false; - uint32 ssrc = sp.first_ssrc(); - assert(ssrc != 0); - // TODO(pbos): Make sure none of sp.ssrcs are used, not just the identifying - // ssrc. rtc::CritScope stream_lock(&stream_crit_); - if (send_streams_.find(ssrc) != send_streams_.end()) { - LOG(LS_ERROR) << "Send stream with SSRC '" << ssrc << "' already exists."; + + if (!ValidateSendSsrcAvailability(sp)) return false; - } + + for (uint32 used_ssrc : sp.ssrcs) + send_ssrcs_.insert(used_ssrc); WebRtcVideoSendStream* stream = new WebRtcVideoSendStream(call_.get(), @@ -862,6 +883,8 @@ bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) { sp, send_rtp_extensions_); + uint32 ssrc = sp.first_ssrc(); + assert(ssrc != 0); send_streams_[ssrc] = stream; if (rtcp_receiver_report_ssrc_ == kDefaultRtcpReceiverReportSsrc) { @@ -899,6 +922,9 @@ bool WebRtcVideoChannel2::RemoveSendStream(uint32 ssrc) { return false; } + for (uint32 old_ssrc : it->second->GetSsrcs()) + send_ssrcs_.erase(old_ssrc); + removed_stream = it->second; send_streams_.erase(it); } @@ -912,6 +938,13 @@ bool WebRtcVideoChannel2::RemoveSendStream(uint32 ssrc) { return true; } +void WebRtcVideoChannel2::DeleteReceiveStream( + WebRtcVideoChannel2::WebRtcVideoReceiveStream* stream) { + for (uint32 old_ssrc : stream->GetSsrcs()) + receive_ssrcs_.erase(old_ssrc); + delete stream; +} + bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) { return AddRecvStream(sp, false); } @@ -926,21 +959,25 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp, uint32 ssrc = sp.first_ssrc(); assert(ssrc != 0); // TODO(pbos): Is this ever valid? - // TODO(pbos): Check if any of the SSRCs overlap. rtc::CritScope stream_lock(&stream_crit_); - { - auto it = receive_streams_.find(ssrc); - if (it != receive_streams_.end()) { - if (default_stream || !it->second->IsDefaultStream()) { - LOG(LS_ERROR) << "Receive stream for SSRC '" << ssrc - << "' already exists."; - return false; - } - delete it->second; - receive_streams_.erase(it); + // Remove running stream if this was a default stream. + auto prev_stream = receive_streams_.find(ssrc); + if (prev_stream != receive_streams_.end()) { + if (default_stream || !prev_stream->second->IsDefaultStream()) { + LOG(LS_ERROR) << "Receive stream for SSRC '" << ssrc + << "' already exists."; + return false; } + DeleteReceiveStream(prev_stream->second); + receive_streams_.erase(prev_stream); } + if (!ValidateReceiveSsrcAvailability(sp)) + return false; + + for (uint32 used_ssrc : sp.ssrcs) + receive_ssrcs_.insert(used_ssrc); + webrtc::VideoReceiveStream::Config config; ConfigureReceiverRtp(&config, sp); @@ -954,9 +991,9 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp, config.audio_channel_id = voice_channel_id_; } - receive_streams_[ssrc] = - new WebRtcVideoReceiveStream(call_.get(), external_decoder_factory_, - default_stream, config, recv_codecs_); + receive_streams_[ssrc] = new WebRtcVideoReceiveStream( + call_.get(), sp.ssrcs, external_decoder_factory_, default_stream, config, + recv_codecs_); return true; } @@ -1013,7 +1050,7 @@ bool WebRtcVideoChannel2::RemoveRecvStream(uint32 ssrc) { LOG(LS_ERROR) << "Stream not found for ssrc: " << ssrc; return false; } - delete stream->second; + DeleteReceiveStream(stream->second); receive_streams_.erase(stream); return true; @@ -1370,6 +1407,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream( const StreamParams& sp, const std::vector& rtp_extensions) : call_(call), + ssrcs_(sp.ssrcs), external_encoder_factory_(external_encoder_factory), stream_(NULL), parameters_(webrtc::VideoSendStream::Config(), options, codec_settings), @@ -1534,6 +1572,11 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::DisconnectCapturer() { return true; } +const std::vector& +WebRtcVideoChannel2::WebRtcVideoSendStream::GetSsrcs() const { + return ssrcs_; +} + void WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions( const VideoOptions& options) { rtc::CritScope cs(&lock_); @@ -1903,11 +1946,13 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::RecreateWebRtcStream() { WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( webrtc::Call* call, + const std::vector& ssrcs, WebRtcVideoDecoderFactory* external_decoder_factory, bool default_stream, const webrtc::VideoReceiveStream::Config& config, const std::vector& recv_codecs) : call_(call), + ssrcs_(ssrcs), stream_(NULL), default_stream_(default_stream), config_(config), @@ -1927,6 +1972,11 @@ WebRtcVideoChannel2::WebRtcVideoReceiveStream::~WebRtcVideoReceiveStream() { ClearDecoders(&allocated_decoders_); } +const std::vector& +WebRtcVideoChannel2::WebRtcVideoReceiveStream::GetSsrcs() const { + return ssrcs_; +} + WebRtcVideoChannel2::WebRtcVideoReceiveStream::AllocatedDecoder WebRtcVideoChannel2::WebRtcVideoReceiveStream::CreateOrReuseVideoDecoder( std::vector* old_decoders, diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index 2ccad0442..bf1a0706d 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -238,9 +238,16 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, bool GetRenderer(uint32 ssrc, VideoRenderer** renderer); private: + class WebRtcVideoReceiveStream; void ConfigureReceiverRtp(webrtc::VideoReceiveStream::Config* config, const StreamParams& sp) const; bool CodecIsExternallySupported(const std::string& name) const; + bool ValidateSendSsrcAvailability(const StreamParams& sp) const + EXCLUSIVE_LOCKS_REQUIRED(stream_crit_); + bool ValidateReceiveSsrcAvailability(const StreamParams& sp) const + EXCLUSIVE_LOCKS_REQUIRED(stream_crit_); + void DeleteReceiveStream(WebRtcVideoReceiveStream* stream) + EXCLUSIVE_LOCKS_REQUIRED(stream_crit_); struct VideoCodecSettings { VideoCodecSettings(); @@ -263,8 +270,8 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, const Settable& codec_settings, const StreamParams& sp, const std::vector& rtp_extensions); - ~WebRtcVideoSendStream(); + void SetOptions(const VideoOptions& options); void SetCodec(const VideoCodecSettings& codec); void SetRtpExtensions( @@ -279,6 +286,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, void Start(); void Stop(); + const std::vector& GetSsrcs() const; VideoSenderInfo GetVideoSenderInfo(); void FillBandwidthEstimationInfo(BandwidthEstimationInfo* bwe_info); @@ -360,6 +368,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, void SetDimensions(int width, int height, bool is_screencast) EXCLUSIVE_LOCKS_REQUIRED(lock_); + const std::vector ssrcs_; webrtc::Call* const call_; WebRtcVideoEncoderFactory* const external_encoder_factory_ GUARDED_BY(lock_); @@ -385,12 +394,15 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, public: WebRtcVideoReceiveStream( webrtc::Call*, + const std::vector& ssrcs, WebRtcVideoDecoderFactory* external_decoder_factory, bool default_stream, const webrtc::VideoReceiveStream::Config& config, const std::vector& recv_codecs); ~WebRtcVideoReceiveStream(); + const std::vector& GetSsrcs() const; + void SetRecvCodecs(const std::vector& recv_codecs); void SetRtpExtensions(const std::vector& extensions); @@ -424,6 +436,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, void ClearDecoders(std::vector* allocated_decoders); webrtc::Call* const call_; + const std::vector ssrcs_; webrtc::VideoReceiveStream* stream_; const bool default_stream_; @@ -481,6 +494,8 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, GUARDED_BY(stream_crit_); std::map receive_streams_ GUARDED_BY(stream_crit_); + std::set send_ssrcs_ GUARDED_BY(stream_crit_); + std::set receive_ssrcs_ GUARDED_BY(stream_crit_); Settable send_codec_; std::vector send_rtp_extensions_; diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index e7560f18f..732287934 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -2367,6 +2367,59 @@ TEST_F(WebRtcVideoChannel2Test, RejectsAddingStreamsWithMissingSsrcsForRtx) { EXPECT_FALSE(channel_->AddRecvStream(sp)); } +TEST_F(WebRtcVideoChannel2Test, RejectsAddingStreamsWithOverlappingRtxSsrcs) { + EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs())); + + const std::vector ssrcs = MAKE_VECTOR(kSsrcs1); + const std::vector rtx_ssrcs = MAKE_VECTOR(kRtxSsrcs1); + + StreamParams sp = + cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs); + + EXPECT_TRUE(channel_->AddSendStream(sp)); + EXPECT_TRUE(channel_->AddRecvStream(sp)); + + // The RTX SSRC is already used in previous streams, using it should fail. + sp = cricket::StreamParams::CreateLegacy(rtx_ssrcs[0]); + EXPECT_FALSE(channel_->AddSendStream(sp)); + EXPECT_FALSE(channel_->AddRecvStream(sp)); + + // After removing the original stream this should be fine to add (makes sure + // that RTX ssrcs are not forever taken). + EXPECT_TRUE(channel_->RemoveSendStream(ssrcs[0])); + EXPECT_TRUE(channel_->RemoveRecvStream(ssrcs[0])); + EXPECT_TRUE(channel_->AddSendStream(sp)); + EXPECT_TRUE(channel_->AddRecvStream(sp)); +} + +TEST_F(WebRtcVideoChannel2Test, + RejectsAddingStreamsWithOverlappingSimulcastSsrcs) { + static const uint32 kFirstStreamSsrcs[] = {1, 2, 3}; + static const uint32 kOverlappingStreamSsrcs[] = {4, 3, 5}; + EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs())); + + const std::vector ssrcs = MAKE_VECTOR(kSsrcs3); + + StreamParams sp = + cricket::CreateSimStreamParams("cname", MAKE_VECTOR(kFirstStreamSsrcs)); + + EXPECT_TRUE(channel_->AddSendStream(sp)); + EXPECT_TRUE(channel_->AddRecvStream(sp)); + + // One of the SSRCs is already used in previous streams, using it should fail. + sp = cricket::CreateSimStreamParams("cname", + MAKE_VECTOR(kOverlappingStreamSsrcs)); + EXPECT_FALSE(channel_->AddSendStream(sp)); + EXPECT_FALSE(channel_->AddRecvStream(sp)); + + // After removing the original stream this should be fine to add (makes sure + // that RTX ssrcs are not forever taken). + EXPECT_TRUE(channel_->RemoveSendStream(ssrcs[0])); + EXPECT_TRUE(channel_->RemoveRecvStream(ssrcs[0])); + EXPECT_TRUE(channel_->AddSendStream(sp)); + EXPECT_TRUE(channel_->AddRecvStream(sp)); +} + class WebRtcVideoEngine2SimulcastTest : public testing::Test { public: WebRtcVideoEngine2SimulcastTest() : engine_(nullptr) {}