Avoid toggling default receive streams in WebRtcVideoChannel2.

This CL removes |default_recv_ssrc_| from DefaultUnsignalledSsrcHandler
and replaces it with calls to a new member function
WebRtcVideoChannel2::GetDefaultReceiveStreamSsrc. The latter checks
the |default_stream_| member on the
WebRtcVideoChannel2::WebRtcVideoReceiveStreams to know which stream
is the current default stream.

This change removes duplicate state and fixes an issue where
incoming unsignaled SSRCs would compete for being the default
receive stream.

BUG=webrtc:7725

Review-Url: https://codereview.webrtc.org/2906893002
Cr-Commit-Position: refs/heads/master@{#18314}

webrtc/media/engine/webrtcvideoengine2.cc

@@ -428,14 +428,18 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::ConfigureVideoEncoderSettings(
 }
 
 DefaultUnsignalledSsrcHandler::DefaultUnsignalledSsrcHandler()
-    : default_recv_ssrc_(0), default_sink_(NULL) {}
+    : default_sink_(nullptr) {}
 
 UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc(
     WebRtcVideoChannel2* channel,
     uint32_t ssrc) {
-  if (default_recv_ssrc_ != 0) {  // Already one default stream, so replace it.
-    channel->RemoveRecvStream(default_recv_ssrc_);
-    default_recv_ssrc_ = 0;
+  rtc::Optional<uint32_t> default_recv_ssrc =
+      channel->GetDefaultReceiveStreamSsrc();
+
+  if (default_recv_ssrc) {
+    LOG(LS_INFO) << "Destroying old default receive stream for SSRC=" << ssrc
+                 << ".";
+    channel->RemoveRecvStream(*default_recv_ssrc);
   }
 
   StreamParams sp;
@@ -446,7 +450,6 @@ UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc(
   }
 
   channel->SetSink(ssrc, default_sink_);
-  default_recv_ssrc_ = ssrc;
   return kDeliverPacket;
 }
 
@@ -456,11 +459,13 @@ DefaultUnsignalledSsrcHandler::GetDefaultSink() const {
 }
 
 void DefaultUnsignalledSsrcHandler::SetDefaultSink(
-    VideoMediaChannel* channel,
+    WebRtcVideoChannel2* channel,
     rtc::VideoSinkInterface<webrtc::VideoFrame>* sink) {
   default_sink_ = sink;
-  if (default_recv_ssrc_ != 0) {
-    channel->SetSink(default_recv_ssrc_, default_sink_);
+  rtc::Optional<uint32_t> default_recv_ssrc =
+      channel->GetDefaultReceiveStreamSsrc();
+  if (default_recv_ssrc) {
+    channel->SetSink(*default_recv_ssrc, default_sink_);
   }
 }
 
@@ -1321,6 +1326,8 @@ bool WebRtcVideoChannel2::SetSink(
   LOG(LS_INFO) << "SetSink: ssrc:" << ssrc << " "
                << (sink ? "(ptr)" : "nullptr");
   if (ssrc == 0) {
+    // Do not hold |stream_crit_| here, since SetDefaultSink will call
+    // WebRtcVideoChannel2::GetDefaultReceiveStreamSsrc().
     default_unsignalled_ssrc_handler_.SetDefaultSink(this, sink);
     return true;
   }
@@ -1529,6 +1536,18 @@ void WebRtcVideoChannel2::SetInterface(NetworkInterface* iface) {
                           kVideoRtpBufferSize);
 }
 
+rtc::Optional<uint32_t> WebRtcVideoChannel2::GetDefaultReceiveStreamSsrc() {
+  rtc::CritScope stream_lock(&stream_crit_);
+  rtc::Optional<uint32_t> ssrc;
+  for (auto it = receive_streams_.begin(); it != receive_streams_.end(); ++it) {
+    if (it->second->IsDefaultStream()) {
+      ssrc.emplace(it->first);
+      break;
+    }
+  }
+  return ssrc;
+}
+
 bool WebRtcVideoChannel2::SendRtp(const uint8_t* data,
                                   size_t len,
                                   const webrtc::PacketOptions& options) {

webrtc/media/engine/webrtcvideoengine2.h

@@ -22,13 +22,14 @@
 #include "webrtc/base/asyncinvoker.h"
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/networkroute.h"
+#include "webrtc/base/optional.h"
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/base/thread_checker.h"
-#include "webrtc/media/base/videosinkinterface.h"
-#include "webrtc/media/base/videosourceinterface.h"
 #include "webrtc/call/call.h"
 #include "webrtc/call/flexfec_receive_stream.h"
 #include "webrtc/media/base/mediaengine.h"
+#include "webrtc/media/base/videosinkinterface.h"
+#include "webrtc/media/base/videosourceinterface.h"
 #include "webrtc/media/engine/webrtcvideodecoderfactory.h"
 #include "webrtc/media/engine/webrtcvideoencoderfactory.h"
 #include "webrtc/video_receive_stream.h"
@@ -81,15 +82,12 @@ class DefaultUnsignalledSsrcHandler : public UnsignalledSsrcHandler {
                            uint32_t ssrc) override;
 
   rtc::VideoSinkInterface<webrtc::VideoFrame>* GetDefaultSink() const;
-  void SetDefaultSink(VideoMediaChannel* channel,
+  void SetDefaultSink(WebRtcVideoChannel2* channel,
                       rtc::VideoSinkInterface<webrtc::VideoFrame>* sink);
 
-  uint32_t default_recv_ssrc() const { return default_recv_ssrc_; }
-
   virtual ~DefaultUnsignalledSsrcHandler() = default;
 
  private:
-  uint32_t default_recv_ssrc_;
   rtc::VideoSinkInterface<webrtc::VideoFrame>* default_sink_;
 };
 
@@ -177,6 +175,8 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
   // Implemented for VideoMediaChannelTest.
   bool sending() const { return sending_; }
 
+  rtc::Optional<uint32_t> GetDefaultReceiveStreamSsrc();
+
   // AdaptReason is used for expressing why a WebRtcVideoSendStream request
   // a lower input frame size than the currently configured camera input frame
   // size. There can be more than one reason OR:ed together.

webrtc/media/engine/webrtcvideoengine2_unittest.cc

@@ -3969,6 +3969,55 @@ TEST_F(WebRtcVideoChannel2Test, ReceiveDifferentUnsignaledSsrc) {
 #endif
 }
 
+// This test verifies that when a new default stream is created for a new
+// unsignaled SSRC, the new stream does not overwrite any old stream that had
+// been the default receive stream before being properly signaled.
+TEST_F(WebRtcVideoChannel2Test,
+       NewUnsignaledStreamDoesNotDestroyPreviouslyUnsignaledStream) {
+  cricket::VideoRecvParameters parameters;
+  parameters.codecs.push_back(GetEngineCodec("VP8"));
+  ASSERT_TRUE(channel_->SetRecvParameters(parameters));
+
+  // No streams signaled and no packets received, so we should not have any
+  // stream objects created yet.
+  EXPECT_EQ(0u, fake_call_->GetVideoReceiveStreams().size());
+
+  // Receive packet on an unsignaled SSRC.
+  uint8_t data[kMinRtpPacketLen];
+  cricket::RtpHeader rtp_header;
+  rtp_header.payload_type = GetEngineCodec("VP8").id;
+  rtp_header.seq_num = rtp_header.timestamp = 0;
+  rtp_header.ssrc = kSsrcs3[0];
+  cricket::SetRtpHeader(data, sizeof(data), rtp_header);
+  rtc::CopyOnWriteBuffer packet(data, sizeof(data));
+  rtc::PacketTime packet_time;
+  channel_->OnPacketReceived(&packet, packet_time);
+  // Default receive stream should be created.
+  ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size());
+  FakeVideoReceiveStream* recv_stream0 =
+      fake_call_->GetVideoReceiveStreams()[0];
+  EXPECT_EQ(kSsrcs3[0], recv_stream0->GetConfig().rtp.remote_ssrc);
+
+  // Signal the SSRC.
+  EXPECT_TRUE(
+      channel_->AddRecvStream(cricket::StreamParams::CreateLegacy(kSsrcs3[0])));
+  ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size());
+  recv_stream0 = fake_call_->GetVideoReceiveStreams()[0];
+  EXPECT_EQ(kSsrcs3[0], recv_stream0->GetConfig().rtp.remote_ssrc);
+
+  // Receive packet on a different unsignaled SSRC.
+  rtp_header.ssrc = kSsrcs3[1];
+  cricket::SetRtpHeader(data, sizeof(data), rtp_header);
+  packet.SetData(data, sizeof(data));
+  channel_->OnPacketReceived(&packet, packet_time);
+  // New default receive stream should be created, but old stream should remain.
+  ASSERT_EQ(2u, fake_call_->GetVideoReceiveStreams().size());
+  EXPECT_EQ(recv_stream0, fake_call_->GetVideoReceiveStreams()[0]);
+  FakeVideoReceiveStream* recv_stream1 =
+      fake_call_->GetVideoReceiveStreams()[1];
+  EXPECT_EQ(kSsrcs3[1], recv_stream1->GetConfig().rtp.remote_ssrc);
+}
+
 TEST_F(WebRtcVideoChannel2Test, CanSentMaxBitrateForExistingStream) {
   AddSendStream();