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

RESOLVED FIXED in Firefox 39

Status

()

defect
P1
normal
Rank:
19
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Blocks: 1113283
Assignee

Updated

4 years ago
Assignee: nobody → docfaraday
Assignee

Comment 1

4 years ago
Posted 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
Assignee

Comment 2

4 years ago
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)
Assignee

Comment 4

4 years ago
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.
Assignee

Comment 5

4 years ago
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
Assignee

Comment 6

4 years ago
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
Assignee

Comment 7

4 years ago
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.
Assignee

Comment 8

4 years ago
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)
Assignee

Comment 10

4 years ago
(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
Assignee

Comment 12

4 years ago
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
Assignee

Comment 13

4 years ago
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee

Comment 18

4 years ago
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.