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 733 matching lines @@
   EXPECT_EQ(107, gcodec.pltype);
   EXPECT_STREQ("telephone-event", gcodec.plname);
 }
 
 // Test that we fail to set an unknown inbound codec.
 TEST_F(WebRtcVoiceEngineTestFake, SetRecvCodecsUnsupportedCodec) {
   EXPECT_TRUE(SetupChannel());
   cricket::AudioRecvParameters parameters;
   parameters.codecs.push_back(kIsacCodec);
   parameters.codecs.push_back(cricket::AudioCodec(127, "XYZ", 32000, 0, 1));
-  EXPECT_FALSE(channel_->SetRecvParameters(parameters));
+
+  // TODO(kwiberg): Should this succeed or fail? With SdpAudioFormat, we
+  // currently don't check the codec spec early enough to be able to fail here.
+  EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+  //EXPECT_FALSE(channel_->SetRecvParameters(parameters));

[kwiberg-webrtc, 2016/12/08 09:44:06]

Should this test simply be changed, or do we want to change things around so
that we verify SdpAudioFormat earlier? For example with the two-stage factory we
discussed earlier?

[the sun, 2016/12/08 10:53:52]

The call to SetRecvParameters() should still fail if we're given an unsupported
codec, and this may be for a number of reason by looking at the old code. First,
I'd like for WVoMC::SetRecvCodecs() to validate as much as possible, so that the
"objective" attributes (payload type range, duplicate payload type assignments,
etc).

Then, is it possible to do something simple initially, e.g. just compare the
list of codecs provided by the decoder factory and reject anything not included
(compare on name, rate), or may there still be cases instantiation may later
fail so we need to do the two-stage?

[kwiberg-webrtc, 2016/12/09 02:39:01]

Sounds good.
The supported clockrate and number of channels etc. differ from codec to codec,
so I'd rather do the check properly. But that's going to be a chunk of work.
(Talking about the proposal where the factory emits a second-stage factory
instead of a decoder; the second-stage factory takes no arguments, and cannot
fail. And then we pass around pt -> 2nd-stage factory maps to all the places.)

Hmm. Maybe a compromise, where the factory can simply vet the SdpAudioFormat
values? Not as type safe as the full solution, but a much smaller change.

[the sun, 2016/12/09 12:29:49]

I don't think we should do the 2-stage right now. I'd go for the compromise, or
even simpler - can you implement the "vetting" function locally in WVoMC and
have it operate on data returned by the factory's GetSupportedDecoders()?

[ossu, 2016/12/09 13:23:39]

I don't see the immediate benefit of adding another level of indirection,
compared to, say, adding an IsSupported() method to the factory as we discussed
a number of months back. Both rely on the caller not messing up the return
value, i.e. checking that it gets a "true" from IsSupported() or that it gets a
non-null factory delegate from the other call.

However, it might be that even IsSupported() is unnecessary. If we initiate
negotiation, we'll only present the values the factory claims to support. Maybe
it could be useful as the answerer (accepting stuff that doesn't match 100% what
we want) but I don't think we're plumbed to support that at the moment - the
code essentially matches two lists.

[kwiberg-webrtc, 2016/12/11 11:24:55]

Checked against GetSupportedCodecs(), since IsSupported() is a pain to implement
(lots of code duplication from the create function, which is one reason I wanted
them to be the same).

 }
 
 // Test that we fail if we have duplicate types in the inbound list.
 TEST_F(WebRtcVoiceEngineTestFake, SetRecvCodecsDuplicatePayloadType) {
   EXPECT_TRUE(SetupChannel());
   cricket::AudioRecvParameters parameters;
   parameters.codecs.push_back(kIsacCodec);
   parameters.codecs.push_back(kCn16000Codec);
   parameters.codecs[1].id = kIsacCodec.id;
   EXPECT_FALSE(channel_->SetRecvParameters(parameters));
@@ skipping 103 matching lines @@
   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);
+
+  // TODO(kwiberg): channel_->SetRecvParameters recreates the
+  // AudioReceiveStream, which has no effect on the FakeWebRtcVoiceEngine in
+  // voe_. Not sure how to handle this---should FakeWebRtcVoiceEngine be fixed?

[the sun, 2016/12/08 10:53:52]

You should remove the call to FakeWebRtcVoiceEngine (and make a WEBRTC_STUB of
the SetRecPayloadType fake impl, removing anything it references that becomes
stale).

For this test it seems appropriate to use GetRecvStreamConfig(kSsrc1) (local
utility in this test) and EXPECT on contents of the stream config.

[kwiberg-webrtc, 2016/12/09 02:39:01]

Acknowledged.

+  EXPECT_EQ(-1, voe_.GetRecPayloadType(channel_num2, gcodec));
+  //EXPECT_EQ(0, voe_.GetRecPayloadType(channel_num2, gcodec));
+  //EXPECT_EQ(106, gcodec.pltype);
+  //EXPECT_STREQ("ISAC", gcodec.plname);

[kwiberg-webrtc, 2016/12/08 09:44:06]

As the comment says, I think this is fundamentally caused by
FakeWebRtcVoiceEngine not coping with AudioReceiveStream. Is fixing that the
right thing to do?

[the sun, 2016/12/08 10:53:52]

See above

 }
 
 // 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));
 }