From 0c4b849b278de3666e8bb10781f2082e3feb8f74 Mon Sep 17 00:00:00 2001 From: ossu Date: Thu, 2 Mar 2017 11:03:25 -0800 Subject: [PATCH] Pick a matching CN codec, rather than the first CN codec. It seems to me that we're currently just picking the first CN codec, rather than the one that matches the clock rate of the voice codec. The only test I've gotten to fail by changing this behavior is the one that's also changed in this CL, which explicitly expects a CN codec to be chosen even though there's none matching. BUG=webrtc:7282 Review-Url: https://codereview.webrtc.org/2707133007 Cr-Commit-Position: refs/heads/master@{#16979} --- webrtc/media/engine/webrtcvoiceengine.cc | 17 +++++++++-------- .../media/engine/webrtcvoiceengine_unittest.cc | 7 ++----- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc index ae4867883a..d562f9209b 100644 --- a/webrtc/media/engine/webrtcvoiceengine.cc +++ b/webrtc/media/engine/webrtcvoiceengine.cc @@ -1995,32 +1995,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) { // 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) { // 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: - 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; } diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc index e72a9a0152..4c8599008a 100644 --- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc +++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc @@ -1244,8 +1244,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecs) { EXPECT_EQ(48000, send_codec_spec.codec_inst.rate); EXPECT_STREQ("ISAC", send_codec_spec.codec_inst.plname); EXPECT_NE(send_codec_spec.codec_inst.plfreq, send_codec_spec.cng_plfreq); - EXPECT_EQ(13, send_codec_spec.cng_payload_type); - EXPECT_EQ(webrtc::kFreq8000Hz, send_codec_spec.cng_plfreq); + EXPECT_EQ(-1, send_codec_spec.cng_payload_type); EXPECT_FALSE(channel_->CanInsertDtmf()); } @@ -2350,9 +2349,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecsWithMultipleSendStreams) { const auto& send_codec_spec = call_.GetAudioSendStream(ssrc)->GetConfig().send_codec_spec; EXPECT_STREQ("PCMU", send_codec_spec.codec_inst.plname); - EXPECT_NE(send_codec_spec.codec_inst.plfreq, send_codec_spec.cng_plfreq); - EXPECT_EQ(97, send_codec_spec.cng_payload_type); - EXPECT_EQ(webrtc::kFreq16000Hz, send_codec_spec.cng_plfreq); + EXPECT_EQ(-1, send_codec_spec.cng_payload_type); } }