Be less pessimistic about turning "default" receive streams into signaled streams.

webrtc/media/engine/webrtcvoiceengine_unittest.cc

 /*
  *  Copyright (c) 2008 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 2796 matching lines @@
 
   DeliverPacket(packet, sizeof(packet));
   EXPECT_EQ(2, GetRecvStream(unsignaled_ssrc).received_packets());
 
   rtc::SetBE32(&packet[8], signaled_ssrc);
   DeliverPacket(packet, sizeof(packet));
   EXPECT_EQ(2, GetRecvStream(signaled_ssrc).received_packets());
   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());
   voe_.set_fail_create_channel(true);
   EXPECT_FALSE(AddRecvStream(2));
 }
 
 // Test that we properly handle failures to add a send stream.
 TEST_F(WebRtcVoiceEngineTestFake, AddSendStreamFail) {
   EXPECT_TRUE(SetupChannel());
@@ skipping 928 matching lines @@
   // Without this cast, the comparison turned unsigned and, thus, failed for -1.
   const int num_specs = static_cast<int>(specs.size());
   EXPECT_GE(find_codec({"cn", 8000, 1}), num_specs);
   EXPECT_GE(find_codec({"cn", 16000, 1}), num_specs);
   EXPECT_EQ(find_codec({"cn", 32000, 1}), -1);
   EXPECT_GE(find_codec({"telephone-event", 8000, 1}), num_specs);
   EXPECT_GE(find_codec({"telephone-event", 16000, 1}), num_specs);
   EXPECT_GE(find_codec({"telephone-event", 32000, 1}), num_specs);
   EXPECT_GE(find_codec({"telephone-event", 48000, 1}), num_specs);
 }