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

[kwiberg-webrtc, 2017/04/24 08:39:34]

This warning will trigger if you try to map a fresh payload type to a previously
used codec, right? That doesn't seem right.

Can you just remove this check altogether? It looks like the new check you do
below will catch everything that needs catching. (Specifically, can you remove
the entire block of code (lines 1850-1863) that constructs |new_codecs|? The
early return on line 1862 could be handled by checking if decoder_map_ ==
decoder_map just before line 1887.)

[Taylor Brandstetter, 2017/04/24 22:44:53]

I wanted to keep the log statement as a warning, because even if it's no longer
treated as an error, it's still abnormal and is something I'd want to see in a
log when investigating a bug.
This seemed to work. No tests are failing, anyway.

[kwiberg-webrtc, 2017/04/25 09:06:04]

OK. But the message is no longer accurate, since the codec may still be mapped
from its old payload type. Change it to something like "%s mapped a second time
(was already at pt %d, now also at pt %d)"?
Well, yes, because all you're doing here is possibly printing a warning and
doing an early return if there are no changes in the pt->codec map. I just
figured the same job could be accomplished with less code.

However, if you really want to keep that log statement, you have to keep all the
code that decides when to use it.

... Oh, just saw the newest patch set where you took all my advice. Good! :-)

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

Fixed log message.

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

[kwiberg-webrtc, 2017/04/24 08:39:34]

Add a TODO about checking for clashes with previously mapped payload types too,
not just currently mapped ones?

[Taylor Brandstetter, 2017/04/24 22:44:54]

Done.

+    auto existing = decoder_map_.find(codec.id);

[kwiberg-webrtc, 2017/04/24 08:39:34]

const?

[Taylor Brandstetter, 2017/04/24 22:44:54]

What's our style for this sort of thing? Just "const auto" doesn't help, since
that gives me a "const std::map<int, SdpAudioFormat>::iterator", not an
"std::map<int, SdpAudioFormat>::const_iterator".

[kwiberg-webrtc, 2017/04/25 09:06:04]

The style guide basically just says "use const whenever it makes sense".
Personally, I find it a good idea to make local variables const whenever I can,
since it's very helpful when reading code to see at a glance that a variable
will never change.

In this particular case, "const auto" *would* be useful---sure, it wouldn't be a
const iterator, but the reader will know for sure that you never mutate *the
iterator itself*. Making it a const iterator in addition to that would also aid
readability, but unfortunately there aren't any low-overhead ways to do that;
the best I can come up with is

  const auto& cmap = decoder_map_;
  const auto existing = cmap.find(codec.id);

and that's too much overhead. But just changing "auto" to "const auto" in the
find call *is* worth it, in my opinion. However, it's just a suggestion---if you
don't want to, I'm fine with leaving out the "const".

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

Done.

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