Negotiate H264 profiles in SDP

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 66bc9ff7d9d0a44f5129707c2cea06172cfe79f8..8913ddb1383b432e1568780762bd3dbf03c72f32 100644
--- a/webrtc/media/base/codec.cc
+++ b/webrtc/media/base/codec.cc
@@ -17,9 +17,11 @@
 #include "webrtc/base/logging.h"
 #include "webrtc/base/stringencode.h"
 #include "webrtc/base/stringutils.h"
+#include "webrtc/common_video/h264/profile_level_id.h"
 
 namespace cricket {
 
+const int kFirstDynamicPayloadId = 96;
 const int kMaxPayloadId = 127;
 
 bool FeedbackParam::operator==(const FeedbackParam& other) const {
@@ -203,7 +205,7 @@ VideoCodec::VideoCodec(int id, const std::string& name)
     : Codec(id, name, kVideoCodecClockrate) {}
 
 VideoCodec::VideoCodec(const std::string& name)
-    : VideoCodec(0 /* id */, name) {}
+    : VideoCodec(kFirstDynamicPayloadId, name) {}

[hta-webrtc, 2016/11/11 12:07:49]

We can't pile all the codecs on payload 96, so something else has to do the real
assigment here.

So why is the non-final number 96 an improvement over the more visible zero?

[magjed_webrtc, 2016/11/12 16:46:25]

Right, this is a non-final payload assignment. This ctor is used from external
codec factories and the external factories are not responsible for payload
assignment. The actual assignment for external factories takes place in
webrtc/media/engine/webrtcvideoengine2.cc, and it doesn't care about what the
original payload value is (e.g. 0 or 96).

However, the external factories use FindMatchingCodec below as a convenience to
make sure the input codec is supported, and I changed FindMatchingCodec to use
VideoCodec::Matches. VideoCodec::Matches use Codec::Matches that checks the
payload type, so e.g. VideoCodec(0, "VP8") won't match VideoCodec(100, "VP8")
because payload type=0 will be interpreted as PCMU audio.

I agree this looks strange and it's probably too brittle, so I changed
FindMatchingCodec to not look at the payload type (this is the current
behavior). I extracted a helper function IsSameH264Profile that I use from both
VideoCodec::Matches and FindMatchingCodec to avoid too much code duplication.

 
 VideoCodec::VideoCodec() : Codec() {
   clockrate = kVideoCodecClockrate;
@@ -218,6 +220,20 @@ bool VideoCodec::operator==(const VideoCodec& c) const {
   return Codec::operator==(c);
 }
 
+bool VideoCodec::Matches(const VideoCodec& other) const {
+  if (!Codec::Matches(other))
+    return false;
+  if (!CodecNamesEq(name.c_str(), kH264CodecName))
+    return true;
+  const rtc::Optional<webrtc::H264::ProfileLevelId> profile_level_id =
+      webrtc::H264::ParseSdpProfileLevelId(params);
+  const rtc::Optional<webrtc::H264::ProfileLevelId> other_profile_level_id =
+      webrtc::H264::ParseSdpProfileLevelId(other.params);
+  // Compare H264 profiles, but not levels.
+  return profile_level_id && other_profile_level_id &&
+         profile_level_id->profile == other_profile_level_id->profile;
+}
+
 VideoCodec VideoCodec::CreateRtxCodec(int rtx_payload_type,
                                       int associated_payload_type) {
   VideoCodec rtx_codec(rtx_payload_type, kRtxCodecName);
@@ -317,12 +333,14 @@ webrtc::VideoCodecType CodecTypeFromName(const std::string& name) {
   return webrtc::kVideoCodecUnknown;
 }
 
-bool IsCodecSupported(const std::vector<VideoCodec>& supported_codecs,
-                      const VideoCodec& codec) {
-  return std::any_of(supported_codecs.begin(), supported_codecs.end(),
-                     [&codec](const VideoCodec& supported_codec) -> bool {
-                       return CodecNamesEq(codec.name, supported_codec.name);
-                     });
+const VideoCodec* FindMatchingCodec(
+    const std::vector<VideoCodec>& supported_codecs,
+    const VideoCodec& codec) {
+  for (const VideoCodec& supported_codec : supported_codecs) {
+    if (supported_codec.Matches(codec))
+      return &supported_codec;
+  }
+  return nullptr;
 }
 
 }  // namespace cricket