Pass SdpAudioFormat through Channel, without converting to CodecInst

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 89 matching lines @@
   void SetSink(Sink* sink) override {}
 };
 
 class WebRtcVoiceEngineTestFake : public testing::Test {
  public:
   WebRtcVoiceEngineTestFake() : WebRtcVoiceEngineTestFake("") {}
 
   explicit WebRtcVoiceEngineTestFake(const char* field_trials)
       : call_(webrtc::Call::Config(&event_log_)), voe_(&apm_),
         override_field_trials_(field_trials) {
-    auto factory = webrtc::MockAudioDecoderFactory::CreateUnusedFactory();
+    auto factory =
+        webrtc::MockAudioDecoderFactory::CreateUnusedPotemkinFactory();

[the sun, 2016/12/14 08:58:24]

I guess that will be explained later, but I'm not sure everyone would
immediately understand what Potemkin means

[ossu, 2016/12/14 10:21:36]

I had to google it. That said, it's pretty witty. :)

[kwiberg-webrtc, 2016/12/14 13:09:36]

"I had to google it" and "pretty witty"---what more could you possibly want from
a class name? :-)

If you have any good suggestions that will play well with our 80-column lines,
I'm all ears.

[ossu, 2016/12/14 13:53:35]

I think I'd just set up the mock manually in this function, possibly by basing
it on a MockAudioDecoderFactory gotten from CreateUnusedFactory(). I.e. adding
the ON_CALLs for GetSupportedDecoders and IsSupportedDecoder here. That said, if
we're basing it on the builtin factory, is there a reason to not just use the
builtin factory directly? Just making sure it never gets used to create a
decoder?

If the tests require the specific list of codecs to match what we (for the time
being) support internally, I think it's fine to just put the builtin factory
there and add a TODO about changing/moving that test once we're further along
with the modularization.

The CreateUnusedFactory and CreateEmptyFactory are there because sometimes you
just need to plug something in that's irrelevant to the test you're performing,
except that without it you might stumble on DCHECKs. Looking at it now,
CreateEmptyFactory isn't even used, so maybe it should go (not in this CL). I
don't think my intention was to set a precedent on how to set up mocks in
general.

[kwiberg-webrtc, 2016/12/15 14:30:50]

OK, using the normal factory and adding a TODO sounds good. (There are already
somewhat overlapping TODOs on you later in this file, but I'll add one anyway.)

     EXPECT_CALL(adm_, AddRef()).WillOnce(Return(0));
     EXPECT_CALL(adm_, Release()).WillOnce(Return(0));
     EXPECT_CALL(adm_, BuiltInAECIsAvailable()).WillOnce(Return(false));
     EXPECT_CALL(adm_, BuiltInAGCIsAvailable()).WillOnce(Return(false));
     EXPECT_CALL(adm_, BuiltInNSIsAvailable()).WillOnce(Return(false));
     EXPECT_CALL(apm_, ApplyConfig(testing::_));
     EXPECT_CALL(apm_, SetExtraOptions(testing::_));
     EXPECT_CALL(apm_, Initialize()).WillOnce(Return(0));
     engine_.reset(new cricket::WebRtcVoiceEngine(&adm_, factory,
                                                  new FakeVoEWrapper(&voe_)));
@@ skipping 742 matching lines @@
   EXPECT_STREQ("telephone-event", gcodec.plname);
 }
 
 TEST_F(WebRtcVoiceEngineTestFake, SetRecvCodecsAfterAddingStreams) {
   EXPECT_TRUE(SetupRecvStream());
   cricket::AudioRecvParameters parameters;
   parameters.codecs.push_back(kIsacCodec);
   parameters.codecs[0].id = 106;  // collide with existing CN 32k
   EXPECT_TRUE(channel_->SetRecvParameters(parameters));
 
-  int channel_num2 = voe_.GetLastChannel();
-  webrtc::CodecInst gcodec;
-  rtc::strcpyn(gcodec.plname, arraysize(gcodec.plname), "ISAC");
-  gcodec.plfreq = 16000;
-  gcodec.channels = 1;
-  EXPECT_EQ(0, voe_.GetRecPayloadType(channel_num2, gcodec));
-  EXPECT_EQ(106, gcodec.pltype);
-  EXPECT_STREQ("ISAC", gcodec.plname);
+  const auto& dm = GetRecvStreamConfig(kSsrc1).decoder_map;
+  ASSERT_EQ(1, dm.count(106));
+  EXPECT_EQ(webrtc::SdpAudioFormat("isac", 16000, 1), dm.at(106));

[the sun, 2016/12/14 08:58:25]

Any reason "ISAC" changed to "isac"?

[kwiberg-webrtc, 2016/12/14 13:09:36]

I re-wrote it by hand rather than copy+pasting, and I'm not found of SHOUTY
CASE.

(We always compare these strings case-insensitively, so the actual case doesn't
matter.)

 }
 
 // Test that we can apply the same set of codecs again while playing.
 TEST_F(WebRtcVoiceEngineTestFake, SetRecvCodecsWhilePlaying) {
   EXPECT_TRUE(SetupRecvStream());
   cricket::AudioRecvParameters parameters;
   parameters.codecs.push_back(kIsacCodec);
   parameters.codecs.push_back(kCn16000Codec);
   EXPECT_TRUE(channel_->SetRecvParameters(parameters));
   channel_->SetPlayout(true);
@@ skipping 2715 matching lines @@
       nullptr, webrtc::CreateBuiltinAudioDecoderFactory());
   webrtc::RtcEventLogNullImpl event_log;
   std::unique_ptr<webrtc::Call> call(
       webrtc::Call::Create(webrtc::Call::Config(&event_log)));
   cricket::WebRtcVoiceMediaChannel channel(&engine, cricket::MediaConfig(),
                                            cricket::AudioOptions(), call.get());
   cricket::AudioRecvParameters parameters;
   parameters.codecs = engine.recv_codecs();
   EXPECT_TRUE(channel.SetRecvParameters(parameters));
 }