From f823ededce7a06dde31553a70111e2237233b170 Mon Sep 17 00:00:00 2001 From: magjed Date: Sat, 12 Nov 2016 09:53:04 -0800 Subject: [PATCH] Negotiate H264 profiles in SDP This CL will start to distinguish H264 profiles during SDP negotiation. We currently don't look at the H264 profile at all and assume they are all Constrained Baseline Level 3.1. This CL will start to check profiles for equality when matching, and will generate the correct answer H264 level. Each local supported H264 profile needs to be listed explicitly in the list of local supported codecs, even if they are redundant. For example, Baseline profile should be listed explicitly even though another profile that is a superset of Baseline is also listed. The reason for this is to simplify the code and avoid profile intersection during matching. So VideoCodec::Matches will check for profile equality, and not check if one codec is a subset of the other. This also leads to the nice property that VideoCodec::Matches is symmetric, i.e. iif a.Matches(b) then b.Matches(a). BUG=webrtc:6337 TBR=tkchin@webrtc.org Review-Url: https://codereview.webrtc.org/2483173002 Cr-Commit-Position: refs/heads/master@{#15051} --- .../android/jni/androidmediaencoder_jni.cc | 2 +- webrtc/common_video/h264/profile_level_id.cc | 8 ++++ .../h264/profile_level_id_unittest.cc | 2 +- webrtc/media/base/codec.cc | 39 +++++++++++++++---- webrtc/media/base/codec.h | 14 +++++-- webrtc/media/engine/fakewebrtcvideoengine.h | 2 +- webrtc/media/engine/webrtcvideoengine2.cc | 11 ++++-- .../engine/webrtcvideoengine2_unittest.cc | 3 +- webrtc/pc/DEPS | 1 + webrtc/pc/mediasession.cc | 5 +++ .../Classes/videotoolboxvideocodecfactory.cc | 4 +- 11 files changed, 71 insertions(+), 20 deletions(-) diff --git a/webrtc/api/android/jni/androidmediaencoder_jni.cc b/webrtc/api/android/jni/androidmediaencoder_jni.cc index 92b5aae65c..a4daa7a539 100644 --- a/webrtc/api/android/jni/androidmediaencoder_jni.cc +++ b/webrtc/api/android/jni/androidmediaencoder_jni.cc @@ -1355,7 +1355,7 @@ webrtc::VideoEncoder* MediaCodecVideoEncoderFactory::CreateVideoEncoder( ALOGW << "No HW video encoder for codec " << codec.name; return nullptr; } - if (IsCodecSupported(supported_codecs_, codec)) { + if (FindMatchingCodec(supported_codecs_, codec)) { ALOGD << "Create HW video encoder for " << codec.name; const VideoCodecType type = cricket::CodecTypeFromName(codec.name); return new MediaCodecVideoEncoder(AttachCurrentThreadIfNeeded(), type, diff --git a/webrtc/common_video/h264/profile_level_id.cc b/webrtc/common_video/h264/profile_level_id.cc index c7e933ac32..f682862ca1 100644 --- a/webrtc/common_video/h264/profile_level_id.cc +++ b/webrtc/common_video/h264/profile_level_id.cc @@ -263,6 +263,14 @@ void GenerateProfileLevelIdForAnswer( const CodecParameterMap& local_supported_params, const CodecParameterMap& remote_offered_params, CodecParameterMap* answer_params) { + // If both local and remote haven't set profile-level-id, they are both using + // the default profile. In this case, don't set profile-level-id in answer + // either. + if (!local_supported_params.count(kProfileLevelId) && + !remote_offered_params.count(kProfileLevelId)) { + return; + } + // Parse profile-level-ids. const rtc::Optional local_profile_level_id = ParseSdpProfileLevelId(local_supported_params); diff --git a/webrtc/common_video/h264/profile_level_id_unittest.cc b/webrtc/common_video/h264/profile_level_id_unittest.cc index ff4b5ba7f7..38a84d576e 100644 --- a/webrtc/common_video/h264/profile_level_id_unittest.cc +++ b/webrtc/common_video/h264/profile_level_id_unittest.cc @@ -152,7 +152,7 @@ TEST(H264ProfileLevelId, TestGenerateProfileLevelIdForAnswerEmpty) { CodecParameterMap answer_params; GenerateProfileLevelIdForAnswer(CodecParameterMap(), CodecParameterMap(), &answer_params); - EXPECT_EQ("42e01f", answer_params["profile-level-id"]); + EXPECT_TRUE(answer_params.empty()); } TEST(H264ProfileLevelId, diff --git a/webrtc/media/base/codec.cc b/webrtc/media/base/codec.cc index 66bc9ff7d9..0320e58c72 100644 --- a/webrtc/media/base/codec.cc +++ b/webrtc/media/base/codec.cc @@ -17,10 +17,20 @@ #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 kMaxPayloadId = 127; +static bool IsSameH264Profile(const CodecParameterMap& params1, + const CodecParameterMap& params2) { + const rtc::Optional profile_level_id = + webrtc::H264::ParseSdpProfileLevelId(params1); + const rtc::Optional other_profile_level_id = + webrtc::H264::ParseSdpProfileLevelId(params2); + // Compare H264 profiles, but not levels. + return profile_level_id && other_profile_level_id && + profile_level_id->profile == other_profile_level_id->profile; +} bool FeedbackParam::operator==(const FeedbackParam& other) const { return _stricmp(other.id().c_str(), id().c_str()) == 0 && @@ -218,6 +228,14 @@ 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 IsSameH264Profile(params, other.params); + return true; +} + VideoCodec VideoCodec::CreateRtxCodec(int rtx_payload_type, int associated_payload_type) { VideoCodec rtx_codec(rtx_payload_type, kRtxCodecName); @@ -317,12 +335,19 @@ webrtc::VideoCodecType CodecTypeFromName(const std::string& name) { return webrtc::kVideoCodecUnknown; } -bool IsCodecSupported(const std::vector& 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& supported_codecs, + const VideoCodec& codec) { + for (const VideoCodec& supported_codec : supported_codecs) { + if (!CodecNamesEq(codec.name, supported_codec.name)) + continue; + if (CodecNamesEq(codec.name.c_str(), kH264CodecName) && + !IsSameH264Profile(codec.params, supported_codec.params)) { + continue; + } + return &supported_codec; + } + return nullptr; } } // namespace cricket diff --git a/webrtc/media/base/codec.h b/webrtc/media/base/codec.h index 3bd93a9247..ca439a6e2f 100644 --- a/webrtc/media/base/codec.h +++ b/webrtc/media/base/codec.h @@ -24,8 +24,6 @@ namespace cricket { typedef std::map CodecParameterMap; -extern const int kMaxPayloadId; - class FeedbackParam { public: FeedbackParam(const std::string& id, const std::string& param) @@ -154,6 +152,11 @@ struct VideoCodec : public Codec { VideoCodec(VideoCodec&& c); virtual ~VideoCodec() = default; + // Indicates if this video codec is the same as the other video codec, e.g. if + // they are both VP8 or VP9, or if they are both H264 with the same H264 + // profile. H264 levels however are not compared. + bool Matches(const VideoCodec& codec) const; + std::string ToString() const; VideoCodec& operator=(const VideoCodec& c); @@ -213,8 +216,11 @@ webrtc::VideoCodecType CodecTypeFromName(const std::string& name); bool HasNack(const Codec& codec); bool HasRemb(const Codec& codec); bool HasTransportCc(const Codec& codec); -bool IsCodecSupported(const std::vector& supported_codecs, - const VideoCodec& codec); +// Returns the first codec in |supported_codecs| that matches |codec|, or +// nullptr if no codec matches. +const VideoCodec* FindMatchingCodec( + const std::vector& supported_codecs, + const VideoCodec& codec); } // namespace cricket diff --git a/webrtc/media/engine/fakewebrtcvideoengine.h b/webrtc/media/engine/fakewebrtcvideoengine.h index a6cfeb3edf..008f7a12d2 100644 --- a/webrtc/media/engine/fakewebrtcvideoengine.h +++ b/webrtc/media/engine/fakewebrtcvideoengine.h @@ -195,7 +195,7 @@ class FakeWebRtcVideoEncoderFactory : public WebRtcVideoEncoderFactory { webrtc::VideoEncoder* CreateVideoEncoder( const cricket::VideoCodec& codec) override { rtc::CritScope lock(&crit_); - if (!IsCodecSupported(codecs_, codec)) + if (!FindMatchingCodec(codecs_, codec)) return nullptr; FakeWebRtcVideoEncoder* encoder = new FakeWebRtcVideoEncoder(); encoders_.push_back(encoder); diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index efec0b5210..2ebbd78e98 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -638,7 +638,7 @@ static std::vector GetSupportedCodecs( out << ", "; } // Don't add internally-supported codecs twice. - if (IsCodecSupported(supported_codecs, codec)) + if (FindMatchingCodec(supported_codecs, codec)) continue; // External video encoders are given payloads 120-127. This also means that @@ -699,7 +699,12 @@ WebRtcVideoChannel2::SelectSendVideoCodec( GetSupportedCodecs(external_encoder_factory_); // Select the first remote codec that is supported locally. for (const VideoCodecSettings& remote_mapped_codec : remote_mapped_codecs) { - if (IsCodecSupported(local_supported_codecs, remote_mapped_codec.codec)) + // For H264, we will limit the encode level to the remote offered level + // regardless if level asymmetry is allowed or not. This is strictly not + // following the spec in https://tools.ietf.org/html/rfc6184#section-8.2.2 + // since we should limit the encode level to the lower of local and remote + // level when level asymmetry is not allowed. + if (FindMatchingCodec(local_supported_codecs, remote_mapped_codec.codec)) return rtc::Optional(remote_mapped_codec); } // No remote codec was supported. @@ -955,7 +960,7 @@ bool WebRtcVideoChannel2::GetChangedRecvParameters( const std::vector local_supported_codecs = GetSupportedCodecs(external_encoder_factory_); for (const VideoCodecSettings& mapped_codec : mapped_codecs) { - if (!IsCodecSupported(local_supported_codecs, mapped_codec.codec)) { + if (!FindMatchingCodec(local_supported_codecs, mapped_codec.codec)) { LOG(LS_ERROR) << "SetRecvParameters called with unsupported video codec: " << mapped_codec.codec.ToString(); return false; diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index d42afcefb3..7fab75df29 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2519,7 +2519,8 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectBadPayloadTypes) { TEST_F(WebRtcVideoChannel2Test, SetSendCodecsAcceptAllValidPayloadTypes) { cricket::VideoSendParameters parameters; parameters.codecs.push_back(kVp8Codec); - for (int payload_type = 0; payload_type <= 127; ++payload_type) { + // Only the dynamic payload types are valid for video codecs. + for (int payload_type = 96; payload_type <= 127; ++payload_type) { parameters.codecs[0].id = payload_type; EXPECT_TRUE(channel_->SetSendParameters(parameters)) << "Payload type '" << payload_type << "' rejected."; diff --git a/webrtc/pc/DEPS b/webrtc/pc/DEPS index e1c9e39ab2..2bb6253519 100644 --- a/webrtc/pc/DEPS +++ b/webrtc/pc/DEPS @@ -1,6 +1,7 @@ include_rules = [ "+webrtc/api", "+webrtc/base", + "+webrtc/common_video/h264", "+webrtc/logging/rtc_event_log", "+webrtc/media", "+webrtc/p2p", diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc index cc93a8b170..f91a729720 100644 --- a/webrtc/pc/mediasession.cc +++ b/webrtc/pc/mediasession.cc @@ -22,6 +22,7 @@ #include "webrtc/base/helpers.h" #include "webrtc/base/logging.h" #include "webrtc/base/stringutils.h" +#include "webrtc/common_video/h264/profile_level_id.h" #include "webrtc/media/base/cryptoparams.h" #include "webrtc/media/base/mediaconstants.h" #include "webrtc/p2p/base/p2pconstants.h" @@ -778,6 +779,10 @@ static void NegotiateCodecs(const std::vector& local_codecs, RTC_DCHECK(apt_it != theirs.params.end()); negotiated.SetParam(kCodecParamAssociatedPayloadType, apt_it->second); } + if (CodecNamesEq(ours.name.c_str(), kH264CodecName)) { + webrtc::H264::GenerateProfileLevelIdForAnswer( + ours.params, theirs.params, &negotiated.params); + } negotiated.id = theirs.id; negotiated.name = theirs.name; negotiated_codecs->push_back(std::move(negotiated)); diff --git a/webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc b/webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc index b7a5141dfd..07725309bf 100644 --- a/webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc +++ b/webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc @@ -47,7 +47,7 @@ VideoToolboxVideoEncoderFactory::~VideoToolboxVideoEncoderFactory() {} VideoEncoder* VideoToolboxVideoEncoderFactory::CreateVideoEncoder( const cricket::VideoCodec& codec) { #if defined(WEBRTC_IOS) - if (IsCodecSupported(supported_codecs_, codec)) { + if (FindMatchingCodec(supported_codecs_, codec)) { LOG(LS_INFO) << "Creating HW encoder for " << codec.name; return new H264VideoToolboxEncoder(); } @@ -83,7 +83,7 @@ VideoDecoder* VideoToolboxVideoDecoderFactory::CreateVideoDecoder( VideoCodecType type) { const auto codec = cricket::VideoCodec(NameFromCodecType(type)); #if defined(WEBRTC_IOS) - if (IsCodecSupported(supported_codecs_, codec)) { + if (FindMatchingCodec(supported_codecs_, codec)) { LOG(LS_INFO) << "Creating HW decoder for " << codec.name; return new H264VideoToolboxDecoder(); }