Be less pessimistic about turning "default" receive streams into signaled 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..0bb350133933a0fc477b0644b620337535999990 100644
--- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc
+++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc
@@ -2814,6 +2814,50 @@ TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignalledAfterSignalled) {
   EXPECT_EQ(2, call_.GetAudioReceiveStreams().size());
 }
 
+// Two tests to verify that adding a receive stream with the same SSRC as a
+// previously added unsignaled stream will only recreate underlying stream
+// objects if the stream parameters have changed.
+TEST_F(WebRtcVoiceEngineTestFake, AddRecvStreamAfterUnsignaled_NoRecreate) {

[Taylor Brandstetter, 2017/02/17 16:24:51]

nit: I'd call this 

AddRecvStreamAfterUnsignaledNotRecreatedIfParamsNotChanged, and the other
AddRecvStreamAfterUnsignaledRecreatedIfParamsChanged. But that may just be my
preference, I like test names that are as descriptive as possible within 80
chars.

[ossu, 2017/02/17 17:05:10]

Not sure if we do this in other places, but I'd find it much easier to read
these long names if there were a _ separating the "group"
AddRecvStreamAfterUnsignaled and the rest (like in the current name). Other than
that, the extra clarity is good.

[the sun, 2017/02/17 17:19:09]

Uhm. I think super long names are not very readable. In this case the extra
information is offset by natural language parsing difficulties. :)

The important bit in these name(s) to me is "AddRecvStreamAfterUnsignaled". I
need to nevertheless look up the test case to see what exactly is going wrong (I
don't spend much time reading through test logs for tests that succeed). Then
the extra long name just makes it harder to remember when I switch from shell to
editor.

So, unless you can come up with a strong argument, I'll keep the current names.

[Taylor Brandstetter, 2017/02/17 17:53:44]

Yeah, that's fine.

+  EXPECT_TRUE(SetupChannel());
+
+  // Spawn unsignaled stream with SSRC=1.
+  DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));

[Taylor Brandstetter, 2017/02/17 16:24:51]

This test relies on the fact that kPcmuFrame uses SSRC 1. Similar to in your
other CL, I'd suggest either having a "MakePcmuFrame" method that takes an SSRC,
or rename it to "kPcmuFrameWithSsrcX", where "kSsrcX" is equal to the SSRC in
it.

[the sun, 2017/02/17 17:19:09]

I think I'll instead make a helper:

void DeliverPcmuFrame(uint32_t ssrc);

...but do that cleanup in a separate CL. Ok?

[Taylor Brandstetter, 2017/02/17 17:53:44]

Sounds good.

+  EXPECT_EQ(1, call_.GetAudioReceiveStreams().size());
+  EXPECT_TRUE(GetRecvStream(1).VerifyLastPacket(kPcmuFrame,
+                                                sizeof(kPcmuFrame)));
+
+  // Verify that the underlying stream object in Call is not recreated when a
+  // stream with SSRC=1 is added.
+  const auto& streams = call_.GetAudioReceiveStreams();
+  EXPECT_EQ(1, streams.size());
+  int audio_receive_stream_id = streams.front()->id();
+  EXPECT_TRUE(AddRecvStream(1));
+  EXPECT_EQ(1, streams.size());
+  EXPECT_EQ(audio_receive_stream_id, streams.front()->id());
+}
+
+TEST_F(WebRtcVoiceEngineTestFake, AddRecvStreamAfterUnsignaled_Recreate) {
+  EXPECT_TRUE(SetupChannel());
+
+  // Spawn unsignaled stream with SSRC=1.
+  DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame));
+  EXPECT_EQ(1, call_.GetAudioReceiveStreams().size());
+  EXPECT_TRUE(GetRecvStream(1).VerifyLastPacket(kPcmuFrame,
+                                                sizeof(kPcmuFrame)));
+
+  // Verify that the underlying stream object in Call *is* recreated when a
+  // stream with SSRC=1 is added, and which has changed stream parameters.
+  const auto& streams = call_.GetAudioReceiveStreams();
+  EXPECT_EQ(1, streams.size());
+  int audio_receive_stream_id = streams.front()->id();
+  cricket::StreamParams stream_params;
+  stream_params.ssrcs.push_back(1);
+  stream_params.sync_label = "sync_label";
+  EXPECT_TRUE(channel_->AddRecvStream(stream_params));
+  EXPECT_EQ(1, streams.size());
+  EXPECT_NE(audio_receive_stream_id, streams.front()->id());
+}
+
 // Test that we properly handle failures to add a receive stream.
 TEST_F(WebRtcVoiceEngineTestFake, AddRecvStreamFail) {
   EXPECT_TRUE(SetupChannel());