Pass SdpAudioFormat through Channel, without converting to CodecInst

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 d90695de26a310fb94cf9ef8a189c08cfd60f6aa..cacb10ae57b16108d13a01c6e6a924e0faff4280 100644
--- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc
+++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc
@@ -107,7 +107,8 @@ class WebRtcVoiceEngineTestFake : public testing::Test {
   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));
@@ -870,14 +871,9 @@ TEST_F(WebRtcVoiceEngineTestFake, SetRecvCodecsAfterAddingStreams) {
   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.