diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc index 2db5526112..b07e9bb5db 100644 --- a/webrtc/media/engine/webrtcvoiceengine.cc +++ b/webrtc/media/engine/webrtcvoiceengine.cc @@ -1166,26 +1166,30 @@ AudioCodecs WebRtcVoiceEngine::CollectRecvCodecs() const { { 32000, false }, { 48000, false }}; - auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) { + auto map_format = [&mapper](const webrtc::SdpAudioFormat& format, + AudioCodecs* out) { rtc::Optional opt_codec = mapper.ToAudioCodec(format); - if (!opt_codec) { + if (opt_codec) { + if (out) { + out->push_back(*opt_codec); + } + } else { LOG(LS_ERROR) << "Unable to assign payload type to format: " << format; - return false; } - auto& codec = *opt_codec; - if (IsCodec(codec, kOpusCodecName)) { - // TODO(ossu): Set this specifically for Opus for now, until we have a - // better way of dealing with rtcp-fb parameters. - codec.AddFeedbackParam( - FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty)); - } - out.push_back(codec); - return true; + return opt_codec; }; for (const auto& spec : specs) { - if (map_format(spec.format)) { + // We need to do some extra stuff before adding the main codecs to out. + rtc::Optional opt_codec = map_format(spec.format, nullptr); + if (opt_codec) { + AudioCodec& codec = *opt_codec; + if (spec.supports_network_adaption) { + codec.AddFeedbackParam( + FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty)); + } + if (spec.allow_comfort_noise) { // Generate a CN entry if the decoder allows it and we support the // clockrate. @@ -1200,20 +1204,22 @@ AudioCodecs WebRtcVoiceEngine::CollectRecvCodecs() const { if (dtmf != generate_dtmf.end()) { dtmf->second = true; } + + out.push_back(codec); } } // Add CN codecs after "proper" audio codecs. for (const auto& cn : generate_cn) { if (cn.second) { - map_format({kCnCodecName, cn.first, 1}); + map_format({kCnCodecName, cn.first, 1}, &out); } } // Add telephone-event codecs last. for (const auto& dtmf : generate_dtmf) { if (dtmf.second) { - map_format({kDtmfCodecName, dtmf.first, 1}); + map_format({kDtmfCodecName, dtmf.first, 1}, &out); } } diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc index a7ea8c2c92..b655a49922 100644 --- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc +++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc @@ -3680,3 +3680,73 @@ TEST(WebRtcVoiceEngineTest, SetRecvCodecs) { parameters.codecs = engine.recv_codecs(); EXPECT_TRUE(channel.SetRecvParameters(parameters)); } + +TEST(WebRtcVoiceEngineTest, CollectRecvCodecs) { + std::vector specs; + webrtc::AudioCodecSpec spec1({"codec1", 48000, 2, {{"param1", "value1"}}}); + spec1.allow_comfort_noise = false; + spec1.supports_network_adaption = true; + specs.push_back(spec1); + webrtc::AudioCodecSpec spec2({"codec2", 32000, 1}); + spec2.allow_comfort_noise = false; + specs.push_back(spec2); + specs.push_back(webrtc::AudioCodecSpec({"codec3", 16000, 1, + {{"param1", "value1b"}, + {"param2", "value2"}}})); + specs.push_back(webrtc::AudioCodecSpec({"codec4", 8000, 1})); + specs.push_back(webrtc::AudioCodecSpec({"codec5", 8000, 2})); + + rtc::scoped_refptr mock_factory = + new rtc::RefCountedObject; + EXPECT_CALL(*mock_factory.get(), GetSupportedDecoders()) + .WillOnce(Return(specs)); + + cricket::WebRtcVoiceEngine engine(nullptr, mock_factory, nullptr); + auto codecs = engine.recv_codecs(); + EXPECT_EQ(11, codecs.size()); + + // Rather than just ASSERTing that there are enough codecs, ensure that we can + // check the actual values safely, to provide better test results. + auto get_codec = + [&codecs](size_t index) -> const cricket::AudioCodec& { + static const cricket::AudioCodec missing_codec(0, "", 0, 0, 0); + if (codecs.size() > index) + return codecs[index]; + return missing_codec; + }; + + // Ensure the general codecs are generated first and in order. + for (size_t i = 0; i != specs.size(); ++i) { + EXPECT_EQ(specs[i].format.name, get_codec(i).name); + EXPECT_EQ(specs[i].format.clockrate_hz, get_codec(i).clockrate); + EXPECT_EQ(specs[i].format.num_channels, get_codec(i).channels); + EXPECT_EQ(specs[i].format.parameters, get_codec(i).params); + } + + // Find the index of a codec, or -1 if not found, so that we can easily check + // supplementary codecs are orderd after the general codecs. + auto find_codec = + [&codecs](const webrtc::SdpAudioFormat& format) -> int { + for (size_t i = 0; i != codecs.size(); ++i) { + const cricket::AudioCodec& codec = codecs[i]; + if (STR_CASE_CMP(codec.name.c_str(), format.name.c_str()) == 0 && + codec.clockrate == format.clockrate_hz && + codec.channels == format.num_channels) { + return static_cast(i); + } + } + return -1; + }; + + // Ensure all supplementary codecs are generated last. Their internal ordering + // is not important. + // Without this cast, the comparison turned unsigned and, thus, failed for -1. + const int num_specs = static_cast(specs.size()); + EXPECT_GE(find_codec({"cn", 8000, 1}), num_specs); + EXPECT_GE(find_codec({"cn", 16000, 1}), num_specs); + EXPECT_EQ(find_codec({"cn", 32000, 1}), -1); + EXPECT_GE(find_codec({"telephone-event", 8000, 1}), num_specs); + EXPECT_GE(find_codec({"telephone-event", 16000, 1}), num_specs); + EXPECT_GE(find_codec({"telephone-event", 32000, 1}), num_specs); + EXPECT_GE(find_codec({"telephone-event", 48000, 1}), num_specs); +} diff --git a/webrtc/modules/audio_coding/codecs/audio_format.cc b/webrtc/modules/audio_coding/codecs/audio_format.cc index 88e42c5d3f..f2a87d3d86 100644 --- a/webrtc/modules/audio_coding/codecs/audio_format.cc +++ b/webrtc/modules/audio_coding/codecs/audio_format.cc @@ -77,4 +77,10 @@ std::ostream& operator<<(std::ostream& os, const SdpAudioFormat& saf) { return os; } +AudioCodecSpec::AudioCodecSpec(const SdpAudioFormat& format) + : format(format) {} + +AudioCodecSpec::AudioCodecSpec(SdpAudioFormat&& format) + : format(std::move(format)) {} + } // namespace webrtc diff --git a/webrtc/modules/audio_coding/codecs/audio_format.h b/webrtc/modules/audio_coding/codecs/audio_format.h index b3f803cd96..6f2c8cfa54 100644 --- a/webrtc/modules/audio_coding/codecs/audio_format.h +++ b/webrtc/modules/audio_coding/codecs/audio_format.h @@ -54,10 +54,26 @@ struct SdpAudioFormat { void swap(SdpAudioFormat& a, SdpAudioFormat& b); std::ostream& operator<<(std::ostream& os, const SdpAudioFormat& saf); +// To avoid API breakage, and make the code clearer, AudioCodecSpec should not +// be directly initializable with any flags indicating optional support. If it +// were, these initializers would break any time a new flag was added. It's also +// more difficult to understand: +// AudioCodecSpec spec{{"format", 8000, 1}, true, false, false, true, true}; +// than +// AudioCodecSpec spec({"format", 8000, 1}); +// spec.allow_comfort_noise = true; +// spec.future_flag_b = true; +// spec.future_flag_c = true; struct AudioCodecSpec { + explicit AudioCodecSpec(const SdpAudioFormat& format); + explicit AudioCodecSpec(SdpAudioFormat&& format); + ~AudioCodecSpec() = default; + SdpAudioFormat format; - bool allow_comfort_noise; // This encoder can be used with an external - // comfort noise generator. + bool allow_comfort_noise = true; // This codec can be used with an external + // comfort noise generator. + bool supports_network_adaption = false; // This codec can adapt to varying + // network conditions. }; } // namespace webrtc diff --git a/webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc b/webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc index e40a0e6598..5f14f97d92 100644 --- a/webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc +++ b/webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc @@ -174,31 +174,36 @@ NamedDecoderConstructor decoder_constructors[] = { class BuiltinAudioDecoderFactory : public AudioDecoderFactory { public: std::vector GetSupportedDecoders() override { - static std::vector specs = { + // Although this looks a bit strange, it means specs need only be initalized + // once, and that that initialization is thread-safe. + static std::vector specs = + []{ + std::vector specs; #ifdef WEBRTC_CODEC_OPUS - { { "opus", 48000, 2, { - {"minptime", "10" }, - {"useinbandfec", "1" } - } - }, false - }, + AudioCodecSpec opus({"opus", 48000, 2, { + {"minptime", "10"}, + {"useinbandfec", "1"} + }}); + opus.allow_comfort_noise = false; + opus.supports_network_adaption = true; + specs.push_back(opus); #endif #if (defined(WEBRTC_CODEC_ISAC) || defined(WEBRTC_CODEC_ISACFX)) - {{"isac", 16000, 1}, true}, + specs.push_back(AudioCodecSpec({"isac", 16000, 1})); #endif #if (defined(WEBRTC_CODEC_ISAC)) - {{"isac", 32000, 1}, true}, + specs.push_back(AudioCodecSpec({"isac", 32000, 1})); #endif #ifdef WEBRTC_CODEC_G722 - {{"G722", 8000, 1}, true}, + specs.push_back(AudioCodecSpec({"G722", 8000, 1})); #endif #ifdef WEBRTC_CODEC_ILBC - {{"iLBC", 8000, 1}, true}, + specs.push_back(AudioCodecSpec({"iLBC", 8000, 1})); #endif - {{"PCMU", 8000, 1}, true}, - {{"PCMA", 8000, 1}, true} - }; - + specs.push_back(AudioCodecSpec({"PCMU", 8000, 1})); + specs.push_back(AudioCodecSpec({"PCMA", 8000, 1})); + return specs; + }(); return specs; }