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

webrtc/media/engine/webrtcvoiceengine.cc

 /*
  *  Copyright (c) 2004 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 1497 matching lines @@
     RecreateAudioReceiveStream();
   }
 
   void RecreateAudioReceiveStream(
       const std::vector<webrtc::RtpExtension>& extensions) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     config_.rtp.extensions = extensions;
     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();
   }
 
   void MaybeRecreateAudioReceiveStream(const std::string& sync_group) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     if (config_.sync_group != sync_group) {
       config_.sync_group = sync_group;
       RecreateAudioReceiveStream();
     }
@@ skipping 308 matching lines @@
     LOG(LS_ERROR) << "Codec payload types overlap.";
     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;
+        LOG(LS_WARNING) << codec.name << " payload type changed.";
+        new_codecs.push_back(codec);
       }
     } 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) {
     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;
     }
+    // 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)});
   }
 
   if (playout_) {
     // Receive codecs can not be changed while playing. So we temporarily
     // pause playout.
     ChangePlayout(false);
   }
 
   decoder_map_ = std::move(decoder_map);
@@ skipping 765 matching lines @@
                       ssrc);
   if (it != unsignaled_recv_ssrcs_.end()) {
     unsignaled_recv_ssrcs_.erase(it);
     return true;
   }
   return false;
 }
 }  // namespace cricket
 
 #endif  // HAVE_WEBRTC_VOICE