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;
-    }());
     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 302 matching lines @@
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
 
   // Set the payload types to be used for incoming media.
   LOG(LS_INFO) << "Setting receive voice codecs.";
 
   if (!VerifyUniquePayloadTypes(codecs)) {
     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;
-      }
-    } 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 << " payload type changed.";

[the sun, 2017/04/26 07:44:10]

Logging the previous and current PTs would probably be useful in this case - or
do we log the new set of mappings in full elsewhere?

[Taylor Brandstetter, 2017/04/26 23:04:40]

That was suggested by Karl too; done.

+    }
     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

[the sun, 2017/04/26 07:44:10]

Would that mean that all validation at this level would be removed? Ideally I'd
like a situation where we validate input data as early as possible, so that the
rest of the system can just DCHECK that it is valid but not have to return
errors. Doing validation at multiple levels is both inefficient and error-prone.

[Taylor Brandstetter, 2017/04/26 23:04:40]

That's the approach I had in mind. We should only need to do input validation at
the API level.

+    // 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.
     ChangePlayout(false);
   }
 
   decoder_map_ = std::move(decoder_map);
   for (auto& kv : recv_streams_) {
     kv.second->RecreateAudioReceiveStream(decoder_map_);
   }
@@ skipping 762 matching lines @@
                       ssrc);
   if (it != unsignaled_recv_ssrcs_.end()) {
     unsignaled_recv_ssrcs_.erase(it);
     return true;
   }
   return false;
 }
 }  // namespace cricket
 
 #endif  // HAVE_WEBRTC_VOICE