From cb3836773c962c3544bb707c8376e14e64d52f70 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Wed, 26 Apr 2017 16:28:42 -0700 Subject: [PATCH] Allow a received audio codec's payload type to change. This will create another decoder instance, which isn't ideal, but that's better than the current behavior where things don't work at all. We still need to fix the root cause of the linked bug, which is that we don't remember previous payload type mappings when generating an offer. This CL also adds a check for what is a real error: when a payload type that was mapped to one codec is changed to map to a different codec. And lastly, this CL removes a DCHECK for an assumption that was not valid: that subsequently applied codec lists will always be supersets of previous lists. BUG=webrtc:5847 Review-Url: https://codereview.webrtc.org/2831333002 Cr-Commit-Position: refs/heads/master@{#17897} --- webrtc/api/audio_codecs/audio_format.cc | 5 ++ webrtc/api/audio_codecs/audio_format.h | 5 ++ webrtc/media/engine/webrtcvoiceengine.cc | 69 +++++++++---------- .../engine/webrtcvoiceengine_unittest.cc | 17 ++++- 4 files changed, 58 insertions(+), 38 deletions(-) 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());