H264 codec: Check profile-level-id when matching

webrtc/media/base/codec.cc

Index: webrtc/media/base/codec.cc
diff --git a/webrtc/media/base/codec.cc b/webrtc/media/base/codec.cc
index 01350f7123695276b735166a6517769c69d14a42..3570ef65271f11a502fb27097d70bbc034dc500d 100644
--- a/webrtc/media/base/codec.cc
+++ b/webrtc/media/base/codec.cc
@@ -18,6 +18,19 @@
 #include "webrtc/base/stringencode.h"
 #include "webrtc/base/stringutils.h"
 
+namespace {
+
+// Return the contained value for |key| if available, and |default_value|
+// otherwise.
+std::string GetParamOrDefault(const cricket::Codec& codec,
+                              const std::string& key,
+                              const std::string& default_value) {
+  cricket::CodecParameterMap::const_iterator iter = codec.params.find(key);
+  return (iter == codec.params.end()) ? default_value : iter->second;

[tommi, 2016/10/25 09:19:58]

nit:

this function should just return |const char*|.  The values for key and
default_value are always const char*, so there's no need for code that copies
them or to create a string copy for return. |codec| is also passed in, so
returning iter->second.c_str() should be fine.

[magjed_webrtc, 2016/10/25 10:58:01]

The reason for not returning char* was that CodecParameterMap is holding
strings. I can do iter->second.c_str() like you say, but that's a raw pointer
and we need to be careful to sync the lifetime with |codec|, but yes it's
possible.

I can make the key arg char* as well. I don't think it will make a difference
performance-wise since std::map<std::string, std::string>::find will convert it
to a std::string.

+}
+
+}  // anonymous namespace
+
 namespace cricket {
 
 const int kMaxPayloadId = 127;
@@ -245,6 +258,31 @@ bool VideoCodec::operator==(const VideoCodec& c) const {
          Codec::operator==(c);
 }
 
+bool VideoCodec::Matches(const VideoCodec& codec) const {
+  if (!Codec::Matches(codec))
+    return false;
+  // TODO(magjed): It would be better to have this logic in a H264 subclass. See
+  // http://crbug/webrtc/6385 for more info.
+  if (!CodecNamesEq(name, kH264CodecName))
+    return true;
+  // H264 codecs need to have matching profile-level-id.
+  const std::string our_profile_level_id = GetParamOrDefault(
+      *this, kH264FmtpProfileLevelId, kH264FmtpDefaultProfileLevelId);
+  const std::string their_profile_level_id = GetParamOrDefault(
+      codec, kH264FmtpProfileLevelId, kH264FmtpDefaultProfileLevelId);
+  if (our_profile_level_id == their_profile_level_id)
+    return true;
+  // At this point, profile-level-id is not an exact match, but that is still ok
+  // if only level_idc differs and level asymmetry is allowed.
+  const bool level_asymmetry_allowed =
+      GetParamOrDefault(*this, kH264FmtpLevelAsymmetryAllowed, "0") == "1" &&
+      GetParamOrDefault(codec, kH264FmtpLevelAsymmetryAllowed, "0") == "1";

[tommi, 2016/10/25 09:19:58]

nit: "0" -> "" (or nullptr?)

[magjed_webrtc, 2016/10/25 10:58:01]

Yes, should be possible. I wanted to be clear here. "0" is the default value for
level asymmetry if it's not set explicitly.

+  // Ignore level_idc and compare only profile_idc and profile_iop.
+  const bool is_profile_match = (our_profile_level_id.substr(0, 4) ==
+                                 their_profile_level_id.substr(0, 4));

[tommi, 2016/10/25 09:19:58]

three nits:

* do you know if our_profile_level_id.length() >= 4? (same for
their_profile_level_id).
* it would be good to document why we use 4, or have a constant for it.
* There's no need to create two string copies just to compare sub strings. 
Instead you could use strncmp or string::compare().

[hta-webrtc, 2016/10/25 09:36:25]

1) profile-level-id is always six characters; anything other than six characters
is a protocol error.
2) there are 3 separate fields in profile-level-id, each using two characters of
the string. This function compares the first 2.

[magjed_webrtc, 2016/10/25 10:58:01]

I will add explicit functionality for parsing the profile-level-id string into
two enums (profile (BP, CBP, MP, HP) + level) in a separate CL, so these
comments will be automatically addressed there.

+  return level_asymmetry_allowed && is_profile_match;

[tommi, 2016/10/25 09:19:58]

nit: if level_asymmetry_allowed is required, you might as well check that and
return before checking for a profile match.

[magjed_webrtc, 2016/10/25 10:58:01]

Sure. As long as the code is as clear as possible. This is not a tight function.

+}
+
 VideoCodec VideoCodec::CreateRtxCodec(int rtx_payload_type,
                                       int associated_payload_type) {
   VideoCodec rtx_codec(rtx_payload_type, kRtxCodecName, 0, 0, 0);