diff --git a/webrtc/api/audio_codecs/audio_format.cc b/webrtc/api/audio_codecs/audio_format.cc index 3a1d0a912d..ea5f183923 100644 --- a/webrtc/api/audio_codecs/audio_format.cc +++ b/webrtc/api/audio_codecs/audio_format.cc @@ -45,6 +45,11 @@ SdpAudioFormat::SdpAudioFormat(const std::string& name, num_channels(num_channels), parameters(param) {} +bool SdpAudioFormat::Matches(const SdpAudioFormat& o) const { + return STR_CASE_CMP(name.c_str(), o.name.c_str()) == 0 && + clockrate_hz == o.clockrate_hz && num_channels == o.num_channels; +} + SdpAudioFormat::~SdpAudioFormat() = default; SdpAudioFormat& SdpAudioFormat::operator=(const SdpAudioFormat&) = default; SdpAudioFormat& SdpAudioFormat::operator=(SdpAudioFormat&&) = default; diff --git a/webrtc/api/audio_codecs/audio_format.h b/webrtc/api/audio_codecs/audio_format.h index 8814a3dcdc..d8e33426b6 100644 --- a/webrtc/api/audio_codecs/audio_format.h +++ b/webrtc/api/audio_codecs/audio_format.h @@ -41,6 +41,11 @@ struct SdpAudioFormat { const Parameters& param); ~SdpAudioFormat(); + // Returns true if this format is compatible with |o|. In SDP terminology: + // would it represent the same codec between an offer and an answer? As + // opposed to operator==, this method disregards codec parameters. + bool Matches(const SdpAudioFormat& o) const; + SdpAudioFormat& operator=(const SdpAudioFormat&); SdpAudioFormat& operator=(SdpAudioFormat&&); diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc index 2384ac28f1..99ada087d4 100644 --- a/webrtc/media/engine/webrtcvoiceengine.cc +++ b/webrtc/media/engine/webrtcvoiceengine.cc @@ -1515,20 +1515,10 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { RecreateAudioReceiveStream(); } - // Set a new payload type -> decoder map. The new map must be a superset of - // the old one. + // Set a new payload type -> decoder map. void RecreateAudioReceiveStream( const std::map& decoder_map) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - RTC_DCHECK([&] { - for (const auto& item : config_.decoder_map) { - auto it = decoder_map.find(item.first); - if (it == decoder_map.end() || *it != item) { - return false; // The old map isn't a subset of the new map. - } - } - return true; - }()); config_.decoder_map = decoder_map; RecreateAudioReceiveStream(); } @@ -1851,44 +1841,51 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs( return false; } - std::vector new_codecs; - // Find all new codecs. We allow adding new codecs but don't allow changing - // the payload type of codecs that is already configured since we might - // already be receiving packets with that payload type. - for (const AudioCodec& codec : codecs) { - AudioCodec old_codec; - // TODO(solenberg): This isn't strictly correct. It should be possible to - // add an additional payload type for a codec. That would result in a new - // decoder object being allocated. What shouldn't work is to remove a PT - // mapping that was previously configured. - if (FindCodec(recv_codecs_, codec, &old_codec)) { - if (old_codec.id != codec.id) { - LOG(LS_ERROR) << codec.name << " payload type changed."; - return false; - } - } else { - new_codecs.push_back(codec); - } - } - if (new_codecs.empty()) { - // There are no new codecs to configure. Already configured codecs are - // never removed. - return true; - } - // Create a payload type -> SdpAudioFormat map with all the decoders. Fail // unless the factory claims to support all decoders. std::map decoder_map; for (const AudioCodec& codec : codecs) { + // Log a warning if a codec's payload type is changing. This used to be + // treated as an error. It's abnormal, but not really illegal. + AudioCodec old_codec; + if (FindCodec(recv_codecs_, codec, &old_codec) && + old_codec.id != codec.id) { + LOG(LS_WARNING) << codec.name << " mapped to a second payload type (" + << codec.id << ", was already mapped to " << old_codec.id + << ")"; + } auto format = AudioCodecToSdpAudioFormat(codec); if (!IsCodec(codec, "cn") && !IsCodec(codec, "telephone-event") && !engine()->decoder_factory_->IsSupportedDecoder(format)) { LOG(LS_ERROR) << "Unsupported codec: " << format; return false; } + // We allow adding new codecs but don't allow changing the payload type of + // codecs that are already configured since we might already be receiving + // packets with that payload type. See RFC3264, Section 8.3.2. + // TODO(deadbeef): Also need to check for clashes with previously mapped + // payload types, and not just currently mapped ones. For example, this + // should be illegal: + // 1. {100: opus/48000/2, 101: ISAC/16000} + // 2. {100: opus/48000/2} + // 3. {100: opus/48000/2, 101: ISAC/32000} + // Though this check really should happen at a higher level, since this + // conflict could happen between audio and video codecs. + auto existing = decoder_map_.find(codec.id); + if (existing != decoder_map_.end() && !existing->second.Matches(format)) { + LOG(LS_ERROR) << "Attempting to use payload type " << codec.id << " for " + << codec.name << ", but it is already used for " + << existing->second.name; + return false; + } decoder_map.insert({codec.id, std::move(format)}); } + if (decoder_map == decoder_map_) { + // There's nothing new to configure. + return true; + } + if (playout_) { // Receive codecs can not be changed while playing. So we temporarily // pause playout. diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc index 75b6320166..dfb49cf87b 100644 --- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc +++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc @@ -916,8 +916,9 @@ TEST_F(WebRtcVoiceEngineTestFake, SetRecvCodecsWhilePlaying) { channel_->SetPlayout(true); EXPECT_TRUE(channel_->SetRecvParameters(parameters)); - // Changing the payload type of a codec should fail. - parameters.codecs[0].id = 127; + // Remapping a payload type to a different codec should fail. + parameters.codecs[0] = kOpusCodec; + parameters.codecs[0].id = kIsacCodec.id; EXPECT_FALSE(channel_->SetRecvParameters(parameters)); EXPECT_TRUE(GetRecvStream(kSsrcX).started()); } @@ -939,6 +940,18 @@ TEST_F(WebRtcVoiceEngineTestFake, AddRecvCodecsWhilePlaying) { EXPECT_EQ(kOpusCodec.id, gcodec.pltype); } +// Test that we accept adding the same codec with a different payload type. +// See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5847 +TEST_F(WebRtcVoiceEngineTestFake, ChangeRecvCodecPayloadType) { + EXPECT_TRUE(SetupRecvStream()); + cricket::AudioRecvParameters parameters; + parameters.codecs.push_back(kIsacCodec); + EXPECT_TRUE(channel_->SetRecvParameters(parameters)); + + ++parameters.codecs[0].id; + EXPECT_TRUE(channel_->SetRecvParameters(parameters)); +} + TEST_F(WebRtcVoiceEngineTestFake, SetSendBandwidthAuto) { EXPECT_TRUE(SetupSendStream());