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..a0d9f486a5cbd58f0969e55dbf653b91f9f2bf41 100644
--- a/webrtc/media/base/codec.cc
+++ b/webrtc/media/base/codec.cc
@@ -118,6 +118,12 @@ bool Codec::GetParam(const std::string& name, int* out) const {
   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;
 }
@@ -245,6 +251,29 @@ bool VideoCodec::operator==(const VideoCodec& c) const {
          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);