Pick a matching CN codec, rather than the first CN codec.

webrtc/media/engine/webrtcvoiceengine.cc

Index: webrtc/media/engine/webrtcvoiceengine.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc
index e0d9884e8f1b2939b777b9f61df6e4722636e9ce..66f60c268e412a6438d7e740ef597d7d89c2d2c4 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -1987,32 +1987,33 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs(
 
     // Loop through the codecs list again to find the CN codec.
     // TODO(solenberg): Break out into a separate function?
-    for (const AudioCodec& codec : codecs) {
+    for (const AudioCodec& cn_codec : codecs) {

[ossu, 2017/03/01 23:24:37]

ossu: Add a check here for if external CN is at all allowed with the chosen
codec.

[ossu, 2017/03/02 00:44:48]

No, wait, I'm mixing this up with changes I haven't landed yet. :)

       // Ignore codecs we don't know about. The negotiation step should prevent
       // this, but double-check to be sure.
       webrtc::CodecInst voe_codec = {0};
-      if (!WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) {
-        LOG(LS_WARNING) << "Unknown codec " << ToString(codec);
+      if (!WebRtcVoiceEngine::ToCodecInst(cn_codec, &voe_codec)) {
+        LOG(LS_WARNING) << "Unknown codec " << ToString(cn_codec);
         continue;
       }
 
-      if (IsCodec(codec, kCnCodecName)) {
+      if (IsCodec(cn_codec, kCnCodecName) &&
+          cn_codec.clockrate == codec->clockrate) {

[minyue-webrtc, 2017/02/27 08:19:54]

I am not familiar with the reason of the way it is implemented.

1. it was really unfortunate to use codec as var name, go ahead change it.

2. my questions are what if there is no CN codec with a matched clock rate. The
old code seems to say, well, cng_plfreq can be different from the send codec's
plfreq. The new code will then not use any CNG. which is desired?

[ossu, 2017/03/01 23:24:38]

From refactoring this further in my injectable audio encoders work, I've found
that the actual CN encoder lacks any parameters to set clockrate or samplerate,
but relies on the values from the codec it was paired with. There's also another
check in AudioSendStream that only enables CNG if the sample rate match, so I
figure we might not be enabling CNG in general, unless the codec we choose uses
a clockrate of 8000, which maybe it almost always does?

         // Turn voice activity detection/comfort noise on if supported.
         // Set the wideband CN payload type appropriately.
         // (narrowband always uses the static payload type 13).
         int cng_plfreq = -1;
-        switch (codec.clockrate) {
+        switch (cn_codec.clockrate) {
           case 8000:
           case 16000:
           case 32000:

[hlundin-webrtc, 2017/02/27 08:00:52]

You will have to check this, but I do think we can accept 48000 Hz as well
nowadays.

[ossu, 2017/03/01 23:24:38]

I think I recall the CN code dealing with that, now that you mention it. Right
now it isn't used, because of Opus having its own CN handling. This should be
filtered based on the appropriate AudioCodecSpec flag instead.

[ossu, 2017/03/02 00:44:48]

... which isn't used in this part of the code yet. I'll leave the 48kHz out of
this for now, since it would only apply for Opus and we don't use it with Opus,
but will check if 48kHz is supported and, if so, signaling of 48kHz CN in my
upcoming injectable encoders changes.

-            cng_plfreq = codec.clockrate;
+            cng_plfreq = cn_codec.clockrate;
             break;
           default:
-            LOG(LS_WARNING) << "CN frequency " << codec.clockrate
+            LOG(LS_WARNING) << "CN frequency " << cn_codec.clockrate
                             << " not supported.";
             continue;
         }
-        send_codec_spec.cng_payload_type = codec.id;
+        send_codec_spec.cng_payload_type = cn_codec.id;
         send_codec_spec.cng_plfreq = cng_plfreq;
         break;
       }