Fixing some issues with payload type mappings.

talk/session/media/mediasession.cc

 /*
  * libjingle
  * Copyright 2004 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
  *     this list of conditions and the following disclaimer in the documentation
  *     and/or other materials provided with the distribution.
  *  3. The name of the author may not be used to endorse or promote products
  *     derived from this software without specific prior written permission.
  *
  * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED
  * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
  * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO
  * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
  * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
  * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
  * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
  * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
  * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "talk/session/media/mediasession.h"
 
+#include <algorithm>  // For std::find_if.
 #include <functional>
 #include <map>
 #include <set>
 #include <utility>
 
 #include "talk/media/base/constants.h"
 #include "talk/media/base/cryptoparams.h"
 #include "talk/session/media/channelmanager.h"
 #include "talk/session/media/srtpfilter.h"
 #include "webrtc/base/helpers.h"
@@ skipping 771 matching lines @@
       !FindCodecById(codecs2, codec2_id, &codec2)) {
     return false;
   }
   return codec1.Matches(codec2);
 }
 
 template <class C>
 static void NegotiateCodecs(const std::vector<C>& local_codecs,
                             const std::vector<C>& offered_codecs,
                             std::vector<C>* negotiated_codecs) {
-  typename std::vector<C>::const_iterator ours;
-  for (ours = local_codecs.begin();
-       ours != local_codecs.end(); ++ours) {
-    typename std::vector<C>::const_iterator theirs;
-    for (theirs = offered_codecs.begin();
-         theirs != offered_codecs.end(); ++theirs) {
-      if (ours->Matches(*theirs)) {
-        C negotiated = *ours;
-        negotiated.IntersectFeedbackParams(*theirs);
-        if (IsRtxCodec(negotiated)) {
-          std::string offered_apt_value;
-          std::string local_apt_value;
-          if (!ours->GetParam(kCodecParamAssociatedPayloadType,
-                              &local_apt_value) ||
-              !theirs->GetParam(kCodecParamAssociatedPayloadType,
-                                &offered_apt_value)) {
-            LOG(LS_WARNING) << "RTX missing associated payload type.";
-            continue;
-          }
-          // Only negotiate RTX if kCodecParamAssociatedPayloadType has been
-          // set in local and remote codecs, and they match.
-          if (!ReferencedCodecsMatch(local_codecs, local_apt_value,
-                                     offered_codecs, offered_apt_value)) {
-            LOG(LS_WARNING) << "RTX associated codecs don't match.";
-            continue;
-          }
-          negotiated.SetParam(kCodecParamAssociatedPayloadType,
-                              offered_apt_value);
-        }
-
-        negotiated.id = theirs->id;
-        // RFC3264: Although the answerer MAY list the formats in their desired
-        // order of preference, it is RECOMMENDED that unless there is a
-        // specific reason, the answerer list formats in the same relative order
-        // they were present in the offer.
-        negotiated.preference = theirs->preference;
-        negotiated_codecs->push_back(negotiated);
+  for (const C& ours : local_codecs) {
+    C theirs;
+    if (FindMatchingCodec(local_codecs, offered_codecs, ours, &theirs)) {
+      C negotiated = ours;
+      negotiated.IntersectFeedbackParams(theirs);
+      if (IsRtxCodec(negotiated)) {
+        std::string offered_apt_value;
+        theirs.GetParam(kCodecParamAssociatedPayloadType, &offered_apt_value);
+        // FindMatchingCodec shouldn't return something with no apt value.
+        RTC_DCHECK(!offered_apt_value.empty());
+        negotiated.SetParam(kCodecParamAssociatedPayloadType,
+                            offered_apt_value);
       }
+      negotiated.id = theirs.id;
+      // RFC3264: Although the answerer MAY list the formats in their desired
+      // order of preference, it is RECOMMENDED that unless there is a
+      // specific reason, the answerer list formats in the same relative order
+      // they were present in the offer.
+      negotiated.preference = theirs.preference;
+      negotiated_codecs->push_back(negotiated);
     }
   }
 }
 
+// Finds a codec in |codecs2| that matches |codec_to_match|, which is
+// a member of |codecs1|. If |codec_to_match| is an RTX codec, both
+// the codecs themselves and their associated codecs must match.
 template <class C>
-static bool FindMatchingCodec(const std::vector<C>& codecs,
+static bool FindMatchingCodec(const std::vector<C>& codecs1,
+                              const std::vector<C>& codecs2,
                               const C& codec_to_match,
                               C* found_codec) {
-  for (typename std::vector<C>::const_iterator it = codecs.begin();
-       it  != codecs.end(); ++it) {
-    if (it->Matches(codec_to_match)) {
-      if (found_codec != NULL) {
-        *found_codec= *it;
+  for (const C& potential_match : codecs2) {
+    if (potential_match.Matches(codec_to_match)) {
+      if (IsRtxCodec(codec_to_match)) {
+        std::string apt_value_1;
+        std::string apt_value_2;
+        if (!codec_to_match.GetParam(kCodecParamAssociatedPayloadType,
+                                     &apt_value_1) ||
+            !potential_match.GetParam(kCodecParamAssociatedPayloadType,
+                                      &apt_value_2)) {
+          LOG(LS_WARNING) << "RTX missing associated payload type.";
+          continue;
+        }
+        if (!ReferencedCodecsMatch(codecs1, apt_value_1, codecs2,
+                                   apt_value_2)) {
+          LOG(LS_INFO) << "RTX associated codecs don't match.";

[pthatcher1, 2016/02/26 21:30:14]

Since this is FindMatchingCodec, LS_INFO here seems a bit strong.  Would
LS_VERBOSE make more sense?

[Taylor Brandstetter, 2016/03/04 16:12:23]

It was strong even before (in fact I changed it from WARNING to INFO). But I
agree it happens often enough that it's annoying. If you have N RTX codecs and
each has a match, you get N*(N-1) log messages. In fact, this is just a basic
step in the algorithm so I don't know why we log anything in the first place.

+          continue;
+        }
+      }
+      if (found_codec) {
+        *found_codec = potential_match;
       }
       return true;
     }
   }
   return false;
 }
 
 // Adds all codecs from |reference_codecs| to |offered_codecs| that dont'
 // already exist in |offered_codecs| and ensure the payload types don't
 // collide.
 template <class C>
 static void FindCodecsToOffer(
     const std::vector<C>& reference_codecs,
     std::vector<C>* offered_codecs,
     UsedPayloadTypes* used_pltypes) {
 
-  typedef std::map<int, C> RtxCodecReferences;
-  RtxCodecReferences new_rtx_codecs;
-
-  // Find all new RTX codecs.
-  for (typename std::vector<C>::const_iterator it = reference_codecs.begin();
-       it != reference_codecs.end(); ++it) {
-    if (!FindMatchingCodec<C>(*offered_codecs, *it, NULL) && IsRtxCodec(*it)) {
-      C rtx_codec = *it;
-      int referenced_pl_type =
-          rtc::FromString<int>(0,
-              rtx_codec.params[kCodecParamAssociatedPayloadType]);
-      new_rtx_codecs.insert(std::pair<int, C>(referenced_pl_type,
-                                              rtx_codec));
-    }
-  }
-
   // Add all new codecs that are not RTX codecs.
-  for (typename std::vector<C>::const_iterator it = reference_codecs.begin();
-       it != reference_codecs.end(); ++it) {
-    if (!FindMatchingCodec<C>(*offered_codecs, *it, NULL) && !IsRtxCodec(*it)) {
-      C codec = *it;
-      int original_payload_id = codec.id;
+  for (const C& reference_codec : reference_codecs) {
+    if (!IsRtxCodec(reference_codec) &&
+        !FindMatchingCodec<C>(reference_codecs, *offered_codecs,
+                              reference_codec, nullptr)) {
+      C codec = reference_codec;
       used_pltypes->FindAndSetIdUsed(&codec);
       offered_codecs->push_back(codec);
-
-      // If this codec is referenced by a new RTX codec, update the reference
-      // in the RTX codec with the new payload type.
-      typename RtxCodecReferences::iterator rtx_it =
-          new_rtx_codecs.find(original_payload_id);
-      if (rtx_it != new_rtx_codecs.end()) {
-        C& rtx_codec = rtx_it->second;
-        rtx_codec.params[kCodecParamAssociatedPayloadType] =
-            rtc::ToString(codec.id);
-      }
     }
   }
 
   // Add all new RTX codecs.
-  for (typename RtxCodecReferences::iterator it = new_rtx_codecs.begin();
-       it != new_rtx_codecs.end(); ++it) {
-    C& rtx_codec = it->second;
-    used_pltypes->FindAndSetIdUsed(&rtx_codec);
-    offered_codecs->push_back(rtx_codec);
+  for (const C& reference_codec : reference_codecs) {
+    if (IsRtxCodec(reference_codec) &&
+        !FindMatchingCodec<C>(reference_codecs, *offered_codecs,
+                              reference_codec, nullptr)) {
+      C rtx_codec = reference_codec;
+
+      std::string associated_pt_str;
+      if (!rtx_codec.GetParam(kCodecParamAssociatedPayloadType,
+                              &associated_pt_str)) {
+        LOG(LS_WARNING) << "RTX codec " << rtx_codec.name
+                        << " is missing an associated payload type.";
+        continue;
+      }
+
+      int associated_pt;
+      if (!rtc::FromString(associated_pt_str, &associated_pt)) {
+        LOG(LS_WARNING) << "Couldn't convert payload type " << associated_pt_str
+                        << " of RTX codec " << rtx_codec.name
+                        << " to an integer.";
+        continue;
+      }
+
+      // Find the associated reference codec for the reference RTX codec.
+      C associated_codec;
+      if (!FindCodecById(reference_codecs, associated_pt, &associated_codec)) {
+        LOG(LS_WARNING) << "Couldn't find associated codec with payload type "
+                        << associated_pt << " for RTX codec " << rtx_codec.name
+                        << ".";
+        continue;
+      }
+
+      // Find a codec in the offered list that matches the reference codec.
+      // Its payload type may be different than the reference codec.
+      C matching_codec;
+      if (!FindMatchingCodec<C>(reference_codecs, *offered_codecs,
+                               associated_codec, &matching_codec)) {
+        LOG(LS_WARNING) << "Couldn't find matching " << associated_codec.name
+                        << " codec.";
+        continue;
+      }
+
+      rtx_codec.params[kCodecParamAssociatedPayloadType] =
+          rtc::ToString(matching_codec.id);
+      used_pltypes->FindAndSetIdUsed(&rtx_codec);
+      offered_codecs->push_back(rtx_codec);
+    }
   }
 }
 
 
 static bool FindByUri(const RtpHeaderExtensions& extensions,
                       const RtpHeaderExtension& ext_to_match,
                       RtpHeaderExtension* found_extension) {
   for (RtpHeaderExtensions::const_iterator it = extensions.begin();
        it  != extensions.end(); ++it) {
     // We assume that all URIs are given in a canonical format.
@@ skipping 1046 matching lines @@
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_VIDEO));
 }
 
 const DataContentDescription* GetFirstDataContentDescription(
     const SessionDescription* sdesc) {
   return static_cast<const DataContentDescription*>(
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_DATA));
 }
 
 }  // namespace cricket