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.
  */
 
@@ skipping 100 matching lines @@
   return true;
 }
 
 bool Codec::GetParam(const std::string& name, int* out) const {
   CodecParameterMap::const_iterator iter = params.find(name);
   if (iter == params.end())
     return false;
   return rtc::FromString(iter->second, out);
 }
 
+std::string Codec::GetParam(const std::string& name,
+                            const std::string& default_value) const {
+  CodecParameterMap::const_iterator iter = params.find(name);
+  return (iter == params.end()) ? default_value : iter->second;
+}
+
 void Codec::SetParam(const std::string& name, const std::string& value) {
   params[name] = value;
 }
 
 void Codec::SetParam(const std::string& name, int value)  {
   params[name] = rtc::ToString(value);
 }
 
 bool Codec::RemoveParam(const std::string& name) {
   return params.erase(name) == 1;
@@ skipping 107 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;
+  if (name != kH264CodecName)

[hta-webrtc, 2016/09/19 06:38:36]

<standard rant>

This is another infection of the H.264 codec functionality into generic code.
Can you please consider if this can be made into "if
codec.matchesParameters(this.params) and subclassed for H.264?

That way, the negotiation can be put inside H.264-specific code.

Yes, I know cricket::VideoCodec is a struct, can't be reasonably subclassed, and
you want to get rid of it. At least add a TODO that says that this is what you
should be doing.

[magjed_webrtc, 2016/09/19 12:40:30]

Yes, I agree this is ugly and that it would be better with a H264 subclass of
VideoCodec where we can put the H264 specific logic. But like you say, this is a
struct, and the required refactoring work to make this an interface looks quite
daunting. I filed an issue for this refactoring and added a TODO with a link to
that issue.

I don't think we should block this fix on the refactoring work, because we have
already landed H264, and profile-level-id is not working properly.

At least this H264 specific code is inside the VideoCodec class.

+    return true;
+  // H264 codecs need to have matching profile-level-id.
+  const std::string our_profile_level_id =
+      GetParam(kH264FmtpProfileLevelId, kH264FmtpDefaultProfileLevelId);
+  const std::string their_profile_level_id =
+      codec.GetParam(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 =
+      GetParam(kH264FmtpLevelAsymmetryAllowed, "0") == "1" &&
+      codec.GetParam(kH264FmtpLevelAsymmetryAllowed, "0") == "1";
+  // 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));
+  return level_asymmetry_allowed && is_profile_match;
+}

[hta-webrtc, 2016/09/19 06:38:36]

Where's the test that we can successfully negotiate with the Firefox 42E0..
profile-level-ID?

[magjed_webrtc, 2016/09/19 12:40:30]

The tests are in codec_unittest.cc and the specific test for level asymmetry is
TestVideoCodecMatchesH264LevelAsymmetry. I used 42000a instead of 42e00a, but I
have now changed it to 42e00a so that it's exactly what Firefox uses in the
wild.

+
 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