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

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

Attachment: 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 01589007024663073340579921e137ed439ab739

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6e8f9843cbd

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 4

[Byron Campen [:bwc]]

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 5

[Byron Campen [:bwc]]

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 6

[Byron Campen [:bwc]]

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

Comment 7

[Byron Campen [:bwc]]

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 8

[Byron Campen [:bwc]]

Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f073809a85

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 10

[Byron Campen [:bwc]]

(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.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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 12

[Byron Campen [:bwc]]

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 13

[Byron Campen [:bwc]]

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.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8576876 [details]
MozReview Request: bz://1141779/bwc

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

Ship It!

Comment 15

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e45d641845

Comment 16

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ba4c1d570a

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9ba4c1d570a

Comment 19

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1141779: Fix H264 negotiation when level-asymmetry-allowed is not specified, and some cleanup.