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

webrtc/media/engine/webrtcvoiceengine.cc

 /*
  *  Copyright (c) 2004 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 1969 matching lines @@
       if (!WebRtcVoiceCodecs::SetPTimeAsPacketSize(
           &send_codec_spec.codec_inst, ptime_ms)) {
         LOG(LS_WARNING) << "Failed to set packet size for codec "
                         << send_codec_spec.codec_inst.plname;
         return false;
       }
     }
 
     // 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;
       }
     }
 
     // Find the telephone-event PT exactly matching the preferred send codec.
     for (const AudioCodec& dtmf_codec : dtmf_codecs) {
       if (dtmf_codec.clockrate == codec->clockrate) {
         dtmf_payload_type_ = rtc::Optional<int>(dtmf_codec.id);
         dtmf_payload_freq_ = dtmf_codec.clockrate;
@@ skipping 636 matching lines @@
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   const auto it = send_streams_.find(ssrc);
   if (it != send_streams_.end()) {
     return it->second->channel();
   }
   return -1;
 }
 }  // namespace cricket
 
 #endif  // HAVE_WEBRTC_VOICE