Closed Bug 1141779 Opened 6 years ago Closed 6 years ago

Fix H264 negotiation when level-asymmetry-allowed is not specified

Categories

(Core :: WebRTC: Signaling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now, if level-asymmetry-allowed is not specified by the remote endpoint, we will fail to use H264 if the profile-level-id doesn't match up. We should be adjusting our profile-level-id to match.
Blocks: 1113283
Assignee: nobody → docfaraday
Attached file MozReview Request: bz://1141779/bwc (obsolete) —
/r/5265 - Bug 1141779: Fix H264 negotiation when level-asymmetry-allowed is not specified, and some cleanup.

Pull down this commit:

hg pull review -r 01589007024663073340579921e137ed439ab739
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6e8f9843cbd
Attachment #8576876 - Flags: review?(rjesup)
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

https://reviewboard.mozilla.org/r/5263/#review4283

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
(Diff revision 1)
> +    mProfileLevelId = (remoteProfileLevelId & 0xFFFF00) + localLevel;

Small preference when dealing with bitfielded constructs to use | instead of + (even if + wioll happen to result in the right value)

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -        codec->MakeNegotiatedCodec(remoteMsection, *fmt, sending);
> +    // the remote SDP (eg; max-fps).

I'm confused what this comment (and perahps the code) mean - for sending, we want to know what the remote SDP says for almost all cases; the one case where we want to know the local SDP for sending is profile-level-id if level-asymmetry is not agreed to.

So the comment is a bit confusing.  Perhaps it should instead say that almost all parameters in the CodecDescription come from the remote (for send) or local (for receive) SDP, with one or two specific exceptions for specific codecs and options.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +    UniquePtr<JsepCodecDescription> sendOrReceiveCodec;

Perhaps codecDescription?  Or description.  sendOrReceive really means nothing here

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
(Diff revision 1)
> -            GetSubprofile(mProfileLevelId)) {

Losing the subprofile check?  Why?  They must match per the RFC (section 8.1, profile-level-id)

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
(Diff revision 1)
> +    }

Full RFC 6184 is a bit more complex than this; you must also support max-recv-level (which if specified means they can support receiving at a level higher than profile-level-id indicates).

For now, just add a comment that we don't support max-recv-level.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
(Diff revision 1)
> +      const SdpFmtpAttributeList::H264Parameters& h264Params =

If we can't find the fmtp, we must assume packetization mode 0, profile-level-id 420010.

I would be ok with (as the old code did) ignoring this or throwing an error.  No real device actually wants to use the no-fmtp defaults.

This is mostly nits; the only thing that isn't really a nit is the loss of the subprofile-compatibility check.  This is admittedly rather arcane and unlikely, but in theory with level-asymmetry (and non-BP codecs in the mix) might be hit.  If there's a good reason we don't need it, then we can land this with nit fixes.
Attachment #8576876 - Flags: review?(rjesup)
https://reviewboard.mozilla.org/r/5263/#review4313

> If we can't find the fmtp, we must assume packetization mode 0, profile-level-id 420010.
> 
> I would be ok with (as the old code did) ignoring this or throwing an error.  No real device actually wants to use the no-fmtp defaults.

Right, I forgot about the default for profile-level-id. Will fix and add a test-case.
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

/r/5265 - Bug 1141779: Fix H264 negotiation when level-asymmetry-allowed is not specified, and some cleanup.

Pull down this commit:

hg pull review -r 7c6bbc6a0d983da0c1d154e397caaefb5bfd13ad
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

/r/5265 - Bug 1141779: Fix H264 negotiation when level-asymmetry-allowed is not specified, and some cleanup.

Pull down this commit:

hg pull review -r ffd678e4645644da17ec8c006b81597dda56279d
https://reviewboard.mozilla.org/r/5263/#review4365

> Perhaps codecDescription?  Or description.  sendOrReceive really means nothing here

sendOrReceive at least hints at the stuff explained in the comment, so I feel it is better than nothing. I wish I could come up with something more explanatory though.
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f073809a85
Attachment #8576876 - Flags: review?(rjesup)
Rank: 19
Flags: firefox-backlog+
Priority: -- → P1
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

https://reviewboard.mozilla.org/r/5263/#review4561

The comment is fine; I'm confused what the two files with WS-only changes are for (and if there are any other changes needed)
Attachment #8576876 - Flags: review?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #9)
> Comment on attachment 8576876 [details]
> MozReview Request: bz://1141779/bwc
> 
> https://reviewboard.mozilla.org/r/5263/#review4561
> 
> The comment is fine; I'm confused what the two files with WS-only changes
> are for (and if there are any other changes needed)

   Reviewboard does this occasionally with interdiffs when you rebase.
https://reviewboard.mozilla.org/r/5263/#review4639

Resolve the level 1b issue and we're good

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
(Diff revisions 1 - 3)
> +                              GetH264Level(mProfileLevelId)));

Profile 1b < profile 1.1; but in your test it will be returned as level 0x10a, which std::min will consider higher than level 5.0 (0x32)
You need to special-case 1b, or do some fancy shifting/masking work.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
(Diff revisions 1 - 3)
> +                              GetH264Level(mProfileLevelId)));

Ditto level

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
(Diff revisions 1 - 3)
> +                              GetH264Level(mProfileLevelId)));

Ditto level 1b
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

/r/5265 - Bug 1141779: Fix H264 negotiation when level-asymmetry-allowed is not specified, and some cleanup.

Pull down this commit:

hg pull review -r fb52a74bd704955981bf7cdf97cca1cec574c105
Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

Ok, I think I have this level stuff right now. It was complex enough that I added a unit-test for the level comparison/setting logic.
Attachment #8576876 - Flags: review?(rjesup)
Attachment #8576876 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/d9ba4c1d570a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8576876 - Attachment is obsolete: true
Attachment #8619724 - Flags: review+
You need to log in before you can comment on or make changes to this bug.