Support N unsignaled audio streams

webrtc/media/engine/webrtcvoiceengine_unittest.cc

Index: webrtc/media/engine/webrtcvoiceengine_unittest.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc
index c55389ae7361d9beaa4e8a1469a732019836e943..f3856cc14182c024cc6411adfa65811141101445 100644
--- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc
+++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc
@@ -2740,9 +2740,8 @@ TEST_F(WebRtcVoiceEngineTestFake, RecvWithMultipleStreams) {
   EXPECT_TRUE(channel_->RemoveRecvStream(ssrc1));
 }
 
-// Test that receiving on an unsignalled stream works (default channel will be
-// created).
-TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignalled) {
+// Test that receiving on an unsignaled stream works (a stream is created).
+TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaled) {
   EXPECT_TRUE(SetupChannel());
   EXPECT_EQ(0, call_.GetAudioReceiveStreams().size());
 
@@ -2753,37 +2752,57 @@ TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignalled) {
                                                 sizeof(kPcmuFrame)));
 }
 
-// Test that receiving on an unsignalled stream works (default channel will be
-// created), and that packets will be forwarded to the default channel
-// regardless of their SSRCs.
-TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignalledWithSsrcSwitch) {
+// Test that receiving N unsignaled stream works (streams will be created), and
+// that packets are forwarded to them all.
+TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaledWithSsrcSwitch) {
   EXPECT_TRUE(SetupChannel());
   unsigned char packet[sizeof(kPcmuFrame)];
   memcpy(packet, kPcmuFrame, sizeof(kPcmuFrame));
 
-  // Note that ssrc = 0 is not supported.
-  uint32_t ssrc = 1;
-  for (; ssrc < 10; ++ssrc) {
+  constexpr uint32_t kMaxUnsignaledCount = 50;
+
+  // Note that SSRC = 0 is not supported.
+  for (uint32_t ssrc = 1; ssrc < (1 + kMaxUnsignaledCount); ++ssrc) {
     rtc::SetBE32(&packet[8], ssrc);
     DeliverPacket(packet, sizeof(packet));
 
-    // Verify we only have one default stream.
-    EXPECT_EQ(1, call_.GetAudioReceiveStreams().size());
+    // Verify we have one new stream for each loop iteration.
+    EXPECT_EQ(ssrc, call_.GetAudioReceiveStreams().size());
     EXPECT_EQ(1, GetRecvStream(ssrc).received_packets());
     EXPECT_TRUE(GetRecvStream(ssrc).VerifyLastPacket(packet, sizeof(packet)));
   }
 
-  // Sending the same ssrc again should not create a new stream.
-  --ssrc;
+  // Send on the same SSRCs again should not create new streams.

[Taylor Brandstetter, 2017/02/17 08:36:15]

nit: "Sending"

[the sun, 2017/02/17 10:10:57]

Done.

+  for (uint32_t ssrc = 1; ssrc < (1 + kMaxUnsignaledCount); ++ssrc) {
+    rtc::SetBE32(&packet[8], ssrc);
+    DeliverPacket(packet, sizeof(packet));
+
+    EXPECT_EQ(kMaxUnsignaledCount, call_.GetAudioReceiveStreams().size());
+    EXPECT_EQ(2, GetRecvStream(ssrc).received_packets());
+    EXPECT_TRUE(GetRecvStream(ssrc).VerifyLastPacket(packet, sizeof(packet)));
+  }
+
+  // Send on another SSRC, the oldest unsignaled stream (SSRC=1) is replaced.
+  constexpr uint32_t kAnotherSsrc = 667;
+  rtc::SetBE32(&packet[8], kAnotherSsrc);
   DeliverPacket(packet, sizeof(packet));
-  EXPECT_EQ(1, call_.GetAudioReceiveStreams().size());
-  EXPECT_EQ(2, GetRecvStream(ssrc).received_packets());
-  EXPECT_TRUE(GetRecvStream(ssrc).VerifyLastPacket(packet, sizeof(packet)));
+
+  const auto& streams = call_.GetAudioReceiveStreams();
+  EXPECT_EQ(kMaxUnsignaledCount, streams.size());
+  size_t i = 0;
+  for (uint32_t ssrc = 2; ssrc < (1 + kMaxUnsignaledCount); ++ssrc, ++i) {
+    EXPECT_EQ(ssrc, streams[i]->GetConfig().rtp.remote_ssrc);
+    EXPECT_EQ(2, streams[i]->received_packets());
+  }
+  EXPECT_EQ(kAnotherSsrc, streams[i]->GetConfig().rtp.remote_ssrc);
+  EXPECT_EQ(1, streams[i]->received_packets());
+  ++i;
+  EXPECT_EQ(kMaxUnsignaledCount, i);

[Taylor Brandstetter, 2017/02/17 08:36:15]

What's the purpose of the final ++i and EXPECT_EQ? Is it a just a sanity check
that every stream was counted? If so could you leave a comment saying that?

[the sun, 2017/02/17 10:10:57]

Yes, basically to "complete" the loop

 }
 
 // Test that a default channel is created even after a signalled stream has been
 // added, and that this stream will get any packets for unknown SSRCs.
-TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignalledAfterSignalled) {
+TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaledAfterSignalled) {

[Taylor Brandstetter, 2017/02/17 08:36:15]

nit: This uses both "Unsignaled" and "Unsignalled"

[the sun, 2017/02/17 10:10:57]

Done.

   EXPECT_TRUE(SetupChannel());
   unsigned char packet[sizeof(kPcmuFrame)];
   memcpy(packet, kPcmuFrame, sizeof(kPcmuFrame));
@@ -3291,14 +3310,34 @@ TEST_F(WebRtcVoiceEngineTestFake, SetOutputVolume) {
   EXPECT_DOUBLE_EQ(3, GetRecvStream(kSsrc2).gain());
 }
 
-TEST_F(WebRtcVoiceEngineTestFake, SetOutputVolumeDefaultRecvStream) {
+TEST_F(WebRtcVoiceEngineTestFake, SetOutputVolumeUnsignaledRecvStream) {
   EXPECT_TRUE(SetupChannel());
+
+  unsigned char pcmuFrame2[sizeof(kPcmuFrame)];
+  memcpy(pcmuFrame2, kPcmuFrame, sizeof(kPcmuFrame));
+  rtc::SetBE32(&pcmuFrame2[8], kSsrc4);
+
+  // Spawn an unsignaled stream by sending a packet - gain should be 1.
+  DeliverPacket(pcmuFrame2, sizeof(pcmuFrame2));
+  EXPECT_DOUBLE_EQ(1, GetRecvStream(kSsrc4).gain());
+
+  // Should remember the volume "2" which will be set on new unsignaled streams,
+  // and also set the gain to 2 on existing unsignaled streams.
   EXPECT_TRUE(channel_->SetOutputVolume(0, 2));
+  EXPECT_DOUBLE_EQ(2, GetRecvStream(kSsrc4).gain());
+
+  // Spawn an unsignaled stream by sending a packet - gain should be 2.
   DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));

[Taylor Brandstetter, 2017/02/17 08:36:14]

nit: May be more readable if "kPcmuFrame2" was delivered after "kPcmuFrame"

[the sun, 2017/02/17 10:10:57]

Done.

   EXPECT_DOUBLE_EQ(2, GetRecvStream(1).gain());
+
+  // Setting gain with SSRC=0 should affect all unsignaled streams.
   EXPECT_TRUE(channel_->SetOutputVolume(0, 3));
+  EXPECT_DOUBLE_EQ(3, GetRecvStream(kSsrc4).gain());
   EXPECT_DOUBLE_EQ(3, GetRecvStream(1).gain());
+
+  // Setting gain on an individual stream affects only that.
   EXPECT_TRUE(channel_->SetOutputVolume(1, 4));
+  EXPECT_DOUBLE_EQ(3, GetRecvStream(kSsrc4).gain());
   EXPECT_DOUBLE_EQ(4, GetRecvStream(1).gain());
 }
 
@@ -3451,7 +3490,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetRawAudioSink) {
   EXPECT_EQ(nullptr, GetRecvStream(kSsrc1).sink());
 }
 
-TEST_F(WebRtcVoiceEngineTestFake, SetRawAudioSinkDefaultRecvStream) {
+TEST_F(WebRtcVoiceEngineTestFake, SetRawAudioSinkUnsignaledRecvStream) {
   EXPECT_TRUE(SetupChannel());
   std::unique_ptr<FakeAudioSink> fake_sink_1(new FakeAudioSink());
   std::unique_ptr<FakeAudioSink> fake_sink_2(new FakeAudioSink());