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

@@ -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_);
-  {
+  // Remove running stream if this was a default stream.
-    auto it = receive_streams_.find(ssrc);
+  auto prev_stream = receive_streams_.find(ssrc);
-    if (it != receive_streams_.end()) {
+  if (prev_stream != receive_streams_.end()) {
-      if (default_stream || !it->second->IsDefaultStream()) {
+    if (default_stream || !prev_stream->second->IsDefaultStream()) {
       LOG(LS_ERROR) << "Receive stream for SSRC '" << ssrc
                     << "' already exists.";
       return false;
     }
-      delete it->second;
+    DeleteReceiveStream(prev_stream->second);
-      receive_streams_.erase(it);
+    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] =
+  receive_streams_[ssrc] = new WebRtcVideoReceiveStream(
-      new WebRtcVideoReceiveStream(call_.get(), external_decoder_factory_,
+      call_.get(), sp.ssrcs, external_decoder_factory_, default_stream, config,
-                                   default_stream, config, recv_codecs_);
+      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<webrtc::RtpExtension>& 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<uint32>&
+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<uint32>& ssrcs,
     WebRtcVideoDecoderFactory* external_decoder_factory,
     bool default_stream,
     const webrtc::VideoReceiveStream::Config& config,
     const std::vector<VideoCodecSettings>& 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<uint32>&
+WebRtcVideoChannel2::WebRtcVideoReceiveStream::GetSsrcs() const {
+  return ssrcs_;
+}
+
 WebRtcVideoChannel2::WebRtcVideoReceiveStream::AllocatedDecoder
 WebRtcVideoChannel2::WebRtcVideoReceiveStream::CreateOrReuseVideoDecoder(
     std::vector<AllocatedDecoder>* old_decoders,

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<VideoCodecSettings>& codec_settings,
         const StreamParams& sp,
         const std::vector<webrtc::RtpExtension>& 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<uint32>& 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<uint32> 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<uint32>& ssrcs,
         WebRtcVideoDecoderFactory* external_decoder_factory,
         bool default_stream,
         const webrtc::VideoReceiveStream::Config& config,
         const std::vector<VideoCodecSettings>& recv_codecs);
     ~WebRtcVideoReceiveStream();
 
+    const std::vector<uint32>& GetSsrcs() const;
+
     void SetRecvCodecs(const std::vector<VideoCodecSettings>& recv_codecs);
     void SetRtpExtensions(const std::vector<webrtc::RtpExtension>& extensions);
 
@@ -424,6 +436,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler,
     void ClearDecoders(std::vector<AllocatedDecoder>* allocated_decoders);
 
     webrtc::Call* const call_;
+    const std::vector<uint32> ssrcs_;
 
     webrtc::VideoReceiveStream* stream_;
     const bool default_stream_;
@@ -481,6 +494,8 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler,
       GUARDED_BY(stream_crit_);
   std::map<uint32, WebRtcVideoReceiveStream*> receive_streams_
       GUARDED_BY(stream_crit_);
+  std::set<uint32> send_ssrcs_ GUARDED_BY(stream_crit_);
+  std::set<uint32> receive_ssrcs_ GUARDED_BY(stream_crit_);
 
   Settable<VideoCodecSettings> send_codec_;
   std::vector<webrtc::RtpExtension> send_rtp_extensions_;

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<uint32> ssrcs = MAKE_VECTOR(kSsrcs1);
+  const std::vector<uint32> 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<uint32> 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) {}