H264 codec: Check profile-level-id when matching

webrtc/media/base/codec.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.
  */
 
 #include "webrtc/media/base/codec.h"
 
 #include <algorithm>
 #include <sstream>
 
 #include "webrtc/base/common.h"
 #include "webrtc/base/logging.h"
 #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;
 
 bool FeedbackParam::operator==(const FeedbackParam& other) const {
   return _stricmp(other.id().c_str(), id().c_str()) == 0 &&
       _stricmp(other.param().c_str(), param().c_str()) == 0;
 }
 
 bool FeedbackParams::operator==(const FeedbackParams& other) const {
@@ skipping 207 matching lines @@
   height = c.height;
   framerate = c.framerate;
   return *this;
 }
 
 bool VideoCodec::operator==(const VideoCodec& c) const {
   return width == c.width && height == c.height && framerate == c.framerate &&
          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);
   rtx_codec.SetParam(kCodecParamAssociatedPayloadType, associated_payload_type);
   return rtx_codec;
 }
 
 VideoCodec::CodecType VideoCodec::GetCodecType() const {
   const char* payload_name = name.c_str();
   if (_stricmp(payload_name, kRedCodecName) == 0) {
@@ skipping 66 matching lines @@
 bool HasTransportCc(const Codec& codec) {
   return codec.HasFeedbackParam(
       FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty));
 }
 
 bool CodecNamesEq(const std::string& name1, const std::string& name2) {
   return _stricmp(name1.c_str(), name2.c_str()) == 0;
 }
 
 }  // namespace cricket