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

@@ -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;

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&&);
 

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<int, webrtc::SdpAudioFormat>& 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<AudioCodec> 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<int, webrtc::SdpAudioFormat> 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.

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());