Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481033003:
Add helper functions for negotiating H264 profile level id (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd helper functions for negotiating H264 profile level id
BUG=webrtc:6337, webrtc:6601
Committed: https://crrev.com/59be5f77c6b6555050579fac4b6f72d5728fdaa2
Cr-Commit-Position: refs/heads/master@{#15012}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments #Patch Set 3 : Add more comments. Remove DecideSendProfileLevelId. #
Dependent Patchsets: Messages
Total messages: 32 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add helper functions for negotiating H264 profile level id BUG=webrtc:6337 ========== to ========== Add helper functions for negotiating H264 profile level id BUG=webrtc:6337,webrtc:6601 ==========
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/...
magjed@webrtc.org changed reviewers: + hta@webrtc.org, kthelgason@webrtc.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/12833)
https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:161: : ParseProfileLevelId(profile_level_id_it->second.c_str()); nit: IMO this is a little too much code for a ternary statement. I think it would be more readable as a conditional. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:218: return rtc::Optional<ProfileLevelId>(); Can't you just choose the lower of the two profiles in this case?
Need to work this one over, and separate the cases into: - Have an offer, need to decide what to answer - Have an offer and an answer, need to decide how to configure your encoder The logic for the two cases is different. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:208: rtc::Optional<ProfileLevelId> NegotiateProfileLevelId( The name of this function is misleading - it doesn't negotiate. It returns the profile-level-id to use, given that the negotiation is over, and we know the local and remote parameter maps that resulted. I suggest you name it DecideSendingProfileLevelId. You need another function for negotiation, which takes the locally supported ProfileLevelId and the remote parameters from an offer, and returns the parameters to return in the answer. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:227: return rtc::Optional<ProfileLevelId>(); In the case where we end up here, the local profile-level-id is "smaller" than the remote profile-level-id, and asymmetry is not allowed. The only way we can get here (legally) is if the remote set is the offer and the local is the answer; by the rules of the RFC, this means that we have successfully negotiated the lower of the two profile-level-ids. Return rtc::optional(local_profile_level_id). (If the local had been the offer, the remote would have returned a "smaller or equal" profile-level-id by the negotiation rules.) https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id_unittest.cc (right): https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:155: EXPECT_FALSE(NegotiateProfileLevelId(local_params, remote_params)); Given that profiles form subsets too, I'm not sure whether this is correct - if the remote accepts high, we are OK with sending constrained baseline; they'll be able to handle it. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:164: EXPECT_FALSE(NegotiateProfileLevelId(local_params, remote_params)); This test is wrong. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:177: EXPECT_EQ(kLevel2_1, profile_level_id->level); It's not very readable to set the parameters in terms of strings and then make the check in terms of kLevel. Would it be more readable to say EXPECT_EQ("42e015", profile_level_id->ToString()); ? https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:193: EXPECT_EQ(kLevel3_1, profile_level_id->level); Actually, we should send with the remote capabilities. The remote capabilities are what they can accept, and we should never send more than that.
> Need to work this one over, and separate the cases into: > > - Have an offer, need to decide what to answer > - Have an offer and an answer, need to decide how to > configure your encoder > > The logic for the two cases is different. Sure. The first patch set was only supposed to cover the first case, i.e. 'Have an offer, need to decide what to answer'. I'm still not sure we need the other case, but I have implemented it in the latest patch set. You can see in the dependent patchset (https://codereview.webrtc.org/2483173002) how this code is planned to be used in the production code. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:161: : ParseProfileLevelId(profile_level_id_it->second.c_str()); On 2016/11/08 09:27:24, kthelgason wrote: > nit: IMO this is a little too much code for a ternary statement. I think it > would be more readable as a conditional. Acknowledged, but I think this is fine. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:208: rtc::Optional<ProfileLevelId> NegotiateProfileLevelId( On 2016/11/08 09:39:06, hta-webrtc wrote: > The name of this function is misleading - it doesn't negotiate. > > It returns the profile-level-id to use, given that the negotiation is over, and > we know the local and remote parameter maps that resulted. > > I suggest you name it DecideSendingProfileLevelId. > > You need another function for negotiation, which takes the locally supported > ProfileLevelId and the remote parameters from an offer, and returns the > parameters to return in the answer. What I'm trying to implement here is the negotiation you describe in the last section, i.e. what parameters to answer based on a local supported format and an offered remote format. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:218: return rtc::Optional<ProfileLevelId>(); On 2016/11/08 09:27:24, kthelgason wrote: > Can't you just choose the lower of the two profiles in this case? This is unfortunately not allowed according to the spec. We must answer the same profile. Only the level part is allowed to change. https://tools.ietf.org/html/rfc6184#section-8.2.2 "When H.264 is offered over RTP using SDP in an Offer/Answer model [8] for negotiation for unicast usage, the following limitations and rules apply: o The parameters identifying a media format configuration for H.264 are profile-level-id and packetization-mode. These media format configuration parameters (except for the level part of profile- level-id) MUST be used symmetrically; that is, the answerer MUST either maintain all configuration parameters or remove the media format (payload type) completely if one or more of the parameter values are not supported. Note that the level part of profile- level-id includes level_idc, and, for indication of Level 1b when profile_idc is equal to 66, 77, or 88, bit 4 (constraint_set3_flag) of profile-iop. The level part of profile- level-id is changeable." https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:227: return rtc::Optional<ProfileLevelId>(); On 2016/11/08 09:39:06, hta-webrtc wrote: > In the case where we end up here, the local profile-level-id is "smaller" than > the remote profile-level-id, and asymmetry is not allowed. > The only way we can get here (legally) is if the remote set is the offer and the > local is the answer; by the rules of the RFC, this means that we have > successfully negotiated the lower of the two profile-level-ids. > > Return rtc::optional(local_profile_level_id). > > (If the local had been the offer, the remote would have returned a "smaller or > equal" profile-level-id by the negotiation rules.) I see, I misunderstood what level-asymmetry-allowed meant. I have re-read https://tools.ietf.org/html/rfc6184#section-8.2.2 and updated the code accordingly. This is actually good, because then this function will always succeed in negotiating and we don't need to return an rtc::Optional. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id_unittest.cc (right): https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:155: EXPECT_FALSE(NegotiateProfileLevelId(local_params, remote_params)); On 2016/11/08 09:39:06, hta-webrtc wrote: > Given that profiles form subsets too, I'm not sure whether this is correct - if > the remote accepts high, we are OK with sending constrained baseline; they'll be > able to handle it. This test does not make sense anymore since the profiles must match before calling NegotiateProfileLevelId so I removed it. Regarding your question; we can't answer a different profile from the offered profile. We must either answer the same profile or reject the payload type completely. We will send different H264 profiles in different payload types. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:164: EXPECT_FALSE(NegotiateProfileLevelId(local_params, remote_params)); On 2016/11/08 09:39:06, hta-webrtc wrote: > This test is wrong. Yes, it's removed now. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:177: EXPECT_EQ(kLevel2_1, profile_level_id->level); On 2016/11/08 09:39:06, hta-webrtc wrote: > It's not very readable to set the parameters in terms of strings and then make > the check in terms of kLevel. > Would it be more readable to say > > EXPECT_EQ("42e015", profile_level_id->ToString()); > > ? Done, strings are used everywhere now. https://codereview.webrtc.org/2481033003/diff/20001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id_unittest.cc:193: EXPECT_EQ(kLevel3_1, profile_level_id->level); On 2016/11/08 09:39:06, hta-webrtc wrote: > Actually, we should send with the remote capabilities. The remote capabilities > are what they can accept, and we should never send more than that. Yeah, the 'send level' in the comment is actually the answer level. We can answer a higher level than what was offered and the remote side is allowed to use that level for encoding.
I defer to Harald in matters of correctness, and adherence to the spec. From a code quality perspective this lgtm.
Still not clear to me what local_profile is intended to represent, which makes it unclear whether this is correct or not. https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:117: remote_profile_level_id->profile); This is the one that looks iffy to me. if local->profile > remote->profile, then answer needs to have profile from remote, but it doesn't kill negotiation - if local->profile represents capabilities rather than representing what's already been committed to in SDP. If local->profile < remote->profile, we fail according to RFC 6184. https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.h:89: // level-asymmetry-allowed. I repeat.... please update this comment to say whether this is a local encoder and a remote decoder or a remote encoder and a local decoder. The important thing isn't which one it is, the important thing is that it's obvious for people reading the code which one it is. Froom your comments and the existence of the next function, I can assume that this is the function to use when the remote parameters are an offer for a stream that the remote encoder can send to the local decoder, and we want to generate the profile-level-id to send in the answer, but the comment above does not make it clear. GenerateProfileLevelIdForAnswer is long-winded, but clearer. I also believe that "The local and remote codec must have the same H264 profile" is false if the "local_supported_params" reflect local capabilities rather than what was signalled - if the local supported profile is High and the remote profile is Baseline, we're *fine*. The resulting answer must have Baseline according to the RFC (I was confused about that), but local capabilities can exceed those signalled in the answer, and still allow for success. Reviewing my confuision about this also makes me unsure if CodecParameterMap is the correct representation for local capabilities - it seems a bit strange to generate a CodecParameterMap for local capabilities and then parse those capabilities back out in order to set them again. Couldn't we generate the local caps straight from a cricket::VideoCodec record?
https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.cc (right): https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.cc:117: remote_profile_level_id->profile); On 2016/11/09 10:35:07, hta-webrtc wrote: > This is the one that looks iffy to me. if local->profile > remote->profile, then > answer needs to have profile from remote, but it doesn't kill negotiation - if > local->profile represents capabilities rather than representing what's already > been committed to in SDP. > > If local->profile < remote->profile, we fail according to RFC 6184. To clarify: Yes, |local_supported_params| and |local_profile_level_id->profile| represents supported capabilities rather than what has already been committed to in SDP. When |local->profile| > |remote->profile|, we are allowed to answer |remote->profile| like you say. However, we don't have to use this feature here. The way it's wired up in WebRTC is that we have only one list of cricket::VideoCodecs that represents our local supported sendrecv capabilites. Different H264 profiles will be represented as different cricket::VideoCodecs with different payload types in this list. So whenever we have a cricket::VideoCodec with High profile in the list, we should also have another cricket::VideoCodec with Baseline in the list. Therefore, we don't need to do clever H264 profile intersection here or in the VideoCodec::Matches function in the other CL. This simplifies a lot, and avoids having to refactor a whole bunch of generic codec matching functions in WebRTC, like this one in webrtc/pc/mediasession.cc: template <class C> static bool FindMatchingCodec(const std::vector<C>& codecs1, const std::vector<C>& codecs2, const C& codec_to_match, C* found_codec); that would be very annoying to specialize for H264 VideoCodecs since it's used for all kinds of codecs like Audio and Data codecs. So to summarize, my plan is this: * We need to explicitly list all H264 profiles we support in the GetSupportedCodecs() in webrtcvideoengine2.cc, e.g. both Baseline and High. * In the rest of the code, we treat different H264 profiles as different codecs. So every pair from "VP8", "VP9", ("H264", "42e0XX"), ("H264", "640cXX") will return false in VideoCodec::Matches. * The rest of the matching code is unchanged, and in functions like this, we are assured that profiles are valid and equal. https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2481033003/diff/60001/webrtc/common_video/h264/... webrtc/common_video/h264/profile_level_id.h:89: // level-asymmetry-allowed. On 2016/11/09 10:35:07, hta-webrtc wrote: > I repeat.... please update this comment to say whether this is a local encoder > and a remote decoder or a remote encoder and a local decoder. > > The important thing isn't which one it is, the important thing is that it's > obvious for people reading the code which one it is. > > Froom your comments and the existence of the next function, I can assume that > this is the function to use when the remote parameters are an offer for a stream > that the remote encoder can send to the local decoder, and we want to generate > the profile-level-id to send in the answer, but the comment above does not make > it clear. It's neither a local encoder and remote decoder nor remote encoder and local decoder. Both |remote_offered_params| and |answer_params| will be used as sendrecv media description, so they are a mix of both encode and decode capabilities. > I also believe that "The local and remote codec must have the same H264 profile" > is false if the "local_supported_params" reflect local capabilities rather than > what was signalled - if the local supported profile is High and the remote > profile is Baseline, we're *fine*. The resulting answer must have Baseline > according to the RFC (I was confused about that), but local capabilities can > exceed those signalled in the answer, and still allow for success. I replied to this in your other comment. > Reviewing my confuision about this also makes me unsure if CodecParameterMap is > the correct representation for local capabilities - it seems a bit strange to > generate a CodecParameterMap for local capabilities and then parse those > capabilities back out in order to set them again. Couldn't we generate the local > caps straight from a cricket::VideoCodec record? This is straight from a cricket::VideoCodec record. The CodecParameterMap here is the |params| field in cricket::Codec: typedef std::map<std::string, std::string> CodecParameterMap; struct Codec { int id; std::string name; int clockrate; CodecParameterMap params; FeedbackParams feedback_params; } The reason for redeclaring CodecParameterMap in this file is to avoid unnecessary circular dependencies.
lgtm OK, I will approve this. I DO recommend that you document how these are supposed to be used - that you assume that all possible profiles for a codec are represented as different codecs, and tested by these functions one at a time. Because otherwise, someone a year from now will read this code, read the RFC, see where they don't match up, and attempt to "fix" your logic.
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/...
Patchset #3 (id:60001) has been deleted
On 2016/11/09 15:39:09, hta-webrtc wrote: > lgtm > > OK, I will approve this. > > I DO recommend that you document how these are supposed to be used - that you > assume that all possible profiles for a codec are represented as different > codecs, and tested by these functions one at a time. Because otherwise, someone > a year from now will read this code, read the RFC, see where they don't match > up, and attempt to "fix" your logic. I have changed the name from NegotiateProfileLevelId to GenerateProfileLevelIdForAnswer and added more comments in the header file. I have also removed the DecideSendProfileLevelId function, because I'm convinced it's not needed after staring a bit more at the code and the spec. When deciding send level from local supported level, and remote offered sendrecv level, we can always use the remote level, regardless if level-asymmetry-allowed is true or not, so this function is not needed.
Patchset #3 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Now it is beautiful! Thank you!
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 Link to the patchset: https://codereview.webrtc.org/2481033003/#ps100001 (title: "Add more comments. Remove DecideSendProfileLevelId.")
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 ========== Add helper functions for negotiating H264 profile level id BUG=webrtc:6337,webrtc:6601 ========== to ========== Add helper functions for negotiating H264 profile level id BUG=webrtc:6337,webrtc:6601 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add helper functions for negotiating H264 profile level id BUG=webrtc:6337,webrtc:6601 ========== to ========== Add helper functions for negotiating H264 profile level id BUG=webrtc:6337,webrtc:6601 Committed: https://crrev.com/59be5f77c6b6555050579fac4b6f72d5728fdaa2 Cr-Commit-Position: refs/heads/master@{#15012} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/59be5f77c6b6555050579fac4b6f72d5728fdaa2 Cr-Commit-Position: refs/heads/master@{#15012} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
