Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483173002:
Negotiate H264 profiles in SDP (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNegotiate 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
Committed: https://crrev.com/f823ededce7a06dde31553a70111e2237233b170
Cr-Commit-Position: refs/heads/master@{#15051}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase #
Total comments: 2
Patch Set 3 : Extract IsSameH264Profile and don't check payload type in FindMatchingCodec #Messages
Total messages: 59 (45 generated)
Description was changed from ========== Negotiate H264 profiles in SDP WIP because the hardcoded payload types does not work. BUG=webrtc:6337 ========== to ========== Negotiate H264 profiles in SDP WIP because the hardcoded payload types does not work. BUG=webrtc:6337 ==========
magjed@webrtc.org changed reviewers: + hta@webrtc.org, kthelgason@webrtc.org
I think we're still confusing ourselves by lumping the encoder together with the decoder. https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.cc File webrtc/media/base/codec.cc (right): https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.cc#ne... webrtc/media/base/codec.cc:233: profile_level_id->profile == other_profile_level_id->profile; See comment about matching. This says whether they are the same, not whether an encoder for one can be decoded by the other. https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.h File webrtc/media/base/codec.h (right): https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.h#new... webrtc/media/base/codec.h:158: // the profiles must be the same. H264 levels however are not compared. if Codec has a Matches function, shouldn't there be an "override" below? (overriding on a reference argument may be a problem.) https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.h#new... webrtc/media/base/codec.h:159: bool Matches(const VideoCodec& codec) const; This is actually an issue with the concept of "codec" as encompassing both an encoder and a decoder. If we have codecs A(encoder E, decoder D) and codec a(encoder e, decoder d), it's perfectly reasonable to have encoder E produce a stream that is decodable by d, but encoder e can produce a stream that is not decodable by D. Example: A=constrained baseline, a=high profile. Matching needs to be on an encoder vs a decoder. https://codereview.webrtc.org/2483173002/diff/1/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2483173002/diff/1/webrtc/pc/mediasession.cc#new... webrtc/pc/mediasession.cc:778: webrtc::H264::NegotiateProfileLevelId(local.params, remote.params); Illustrates my point about Negotiate being misnamed in the CL that introduced it - "Intersect" would be a better name.
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.cc File webrtc/media/base/codec.cc (right): https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.cc#ne... webrtc/media/base/codec.cc:233: profile_level_id->profile == other_profile_level_id->profile; On 2016/11/08 13:59:53, hta-webrtc wrote: > See comment about matching. This says whether they are the same, not whether an > encoder for one can be decoded by the other. Yes, this function compares whether the codecs are the same (and distinguishes different H264 profiles). It's not supposed to be used to determine whether an encoder for one can be decoded by the other. I have updated the function comment to better reflect this. Since we need to send different H264 profiles as different payload types, this is the natural extension of the current Matches function that only compares codec names. https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.h File webrtc/media/base/codec.h (right): https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.h#new... webrtc/media/base/codec.h:158: // the profiles must be the same. H264 levels however are not compared. On 2016/11/08 13:59:53, hta-webrtc wrote: > if Codec has a Matches function, shouldn't there be an "override" below? > > (overriding on a reference argument may be a problem.) No, this is overloading, not overriding. https://codereview.webrtc.org/2483173002/diff/1/webrtc/media/base/codec.h#new... webrtc/media/base/codec.h:159: bool Matches(const VideoCodec& codec) const; On 2016/11/08 13:59:53, hta-webrtc wrote: > This is actually an issue with the concept of "codec" as encompassing both an > encoder and a decoder. If we have codecs A(encoder E, decoder D) and codec > a(encoder e, decoder d), it's perfectly reasonable to have encoder E produce a > stream that is decodable by d, but encoder e can produce a stream that is not > decodable by D. > > Example: A=constrained baseline, a=high profile. > > Matching needs to be on an encoder vs a decoder. The codec here does not really encompass both an encoder and a decoder. The codec we offer/answer over SDP happens to represent both the encoder and the decoder, but I would say that's an SDP artifact and not built into this class. https://codereview.webrtc.org/2483173002/diff/1/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2483173002/diff/1/webrtc/pc/mediasession.cc#new... webrtc/pc/mediasession.cc:778: webrtc::H264::NegotiateProfileLevelId(local.params, remote.params); On 2016/11/08 13:59:53, hta-webrtc wrote: > Illustrates my point about Negotiate being misnamed in the CL that introduced it > - "Intersect" would be a better name. Yeah, I'm not that happy with either the name intersect or negotiate here.
Patchset #2 (id:40001) has been deleted
Now this looks good to me, negotiation-wise. I don't think the RTX problem is solved yet (WIP still in CL description), so not including the magic token yet.
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12385)
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12397) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16070)
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/19502)
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Negotiate H264 profiles in SDP WIP because the hardcoded payload types does not work. BUG=webrtc:6337 ========== to ========== 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 ==========
Patchset #2 (id:160001) has been deleted
On 2016/11/10 10:28:33, hta-webrtc wrote: > Now this looks good to me, negotiation-wise. > > I don't think the RTX problem is solved yet (WIP still in CL description), so > not including the magic token yet. Please take another look, I have rebased and changed the code a bit and updated the CL description. It's true that the RTX problem is not yet solved, but I will fix that in a separate CL. So even when this CL lands, it's not possible to list more than one H264 profile. However, it will be nice to verify that interop with Chromium, Firefox, Edge, etc. still works after this change since they are all using slightly different profile-level-ids.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm One question about something that looks like a drive-by change. https://codereview.webrtc.org/2483173002/diff/180001/webrtc/media/base/codec.cc File webrtc/media/base/codec.cc (right): https://codereview.webrtc.org/2483173002/diff/180001/webrtc/media/base/codec.... webrtc/media/base/codec.cc:208: : VideoCodec(kFirstDynamicPayloadId, name) {} 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?
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2483173002/diff/180001/webrtc/media/base/codec.cc File webrtc/media/base/codec.cc (right): https://codereview.webrtc.org/2483173002/diff/180001/webrtc/media/base/codec.... webrtc/media/base/codec.cc:208: : VideoCodec(kFirstDynamicPayloadId, name) {} On 2016/11/11 12:07:49, hta-webrtc wrote: > 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? 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.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/2483173002/#ps200001 (title: "Extract IsSameH264Profile and don't check payload type in FindMatchingCodec")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10117)
Description was changed from ========== 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 ========== to ========== 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 ==========
magjed@webrtc.org changed reviewers: + tkchin@webrtc.org
Zeke - please review webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc. I'm TBR:ing you on that file since it's a trivial name change and I want to roll this CL to chromium to test it.
The CQ bit was checked by magjed@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/f823ededce7a06dde31553a70111e2237233b170 Cr-Commit-Position: refs/heads/master@{#15051} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f823ededce7a06dde31553a70111e2237233b170 Cr-Commit-Position: refs/heads/master@{#15051}
Message was sent while issue was closed.
On 2016/11/12 18:06:30, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/f823ededce7a06dde31553a70111e2237233b170 > Cr-Commit-Position: refs/heads/master@{#15051} lgtm |
