Allow a received audio codec's payload type to change.

webrtc/media/engine/webrtcvoiceengine.cc

Index: webrtc/media/engine/webrtcvoiceengine.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc
index 2384ac28f173870959c97b6bc78224f931a03464..5fec94d131c32c215cf29d7045012189872ed6ed 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<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;
-    }());

[kwiberg-webrtc, 2017/04/21 12:49:00]

Why are you removing this check? I thought we weren't allowed to remove or
change existing payload type mappings?

[Taylor Brandstetter, 2017/04/21 13:02:06]

I'm removing this check because the assertion is not valid; I can pass SDP into
the JS API in Chrome that violates it.

I do add a check for payload types being remapped in this CL. But if I do
"setLocalDescription(lotsOfCodecs)" then "setLocalDescription(fewerCodecs)",
there's nothing that prevents this from being called with fewer codecs.

[kwiberg-webrtc, 2017/04/21 13:18:57]

But isn't the error that higher layers (or this one) ought to test for and
politely reject such a request, so that the DCHECK can never trigger?

[Taylor Brandstetter, 2017/04/21 14:20:23]

No; apps should be able to set a new local description later with a smaller set
of codecs.

One could argue that removing codecs from SDP shouldn't remove mappings at the
AudioReceiveStream level. But that sort of change would be better in a separate
CL.

     config_.decoder_map = decoder_map;
     RecreateAudioReceiveStream();
   }
@@ -1857,14 +1847,10 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs(
   // 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;
+        LOG(LS_WARNING) << codec.name << " payload type changed.";
+        new_codecs.push_back(codec);
       }
     } else {
       new_codecs.push_back(codec);
@@ -1886,6 +1872,15 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs(
       LOG(LS_ERROR) << "Unsupported codec: " << format;
       return false;
     }
+    // If a payload type is already mapped to a codec, it's illegal to map it
+    // to a new codec.
+    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;
+    }

[kwiberg-webrtc, 2017/04/21 12:49:00]

Hmm... here you disallow changing (but not removing) pt -> codec mappings. But
that will still allow a removal immediately followed by reusing the now vacant
pt for a new codec, which has the same effect as changing the map.

[Taylor Brandstetter, 2017/04/21 13:02:06]

True. We should probably store a permanent history of mappings. But for this CL,
I just wanted to make sure there was *some* equivalent level of error-checking
as there was before.

[kwiberg-webrtc, 2017/04/21 13:18:58]

Storing a permanent history of mappings sounds much more complex than simply
never changing existing mappings.

     decoder_map.insert({codec.id, std::move(format)});
   }