1042791 - H264 SDP in WebRTC has non-standard fmtp line

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Alan Ford]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.76.4 (KHTML, like Gecko) Version/7.0.4 Safari/537.76.4

Steps to reproduce:

getUserMedia with the new h264 plugin installed.


Actual results:

SDP m-line for h264 reads:

m=video 60046 RTP/SAVPF 120 126
a=rtpmap:120 VP8/90000
a=rtpmap:126 H264/90000
a=fmtp:126 profile-level-id=0x42e00c;packetization-mode=1
a=sendrecv


Expected results:

The profile-level-id is invalid, it should read 42e00c (i.e. no 0x prefix), like this:

a=rtpmap:126 H264/90000
a=fmtp:126 profile-level-id=42e00c;packetization-mode=1

Comment 1

[Alan Ford]

I should clarify that this means that the SIP endpoint I have tested against is rejecting this H264 offer.

Comment 2

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

QUite right, thanks!

Comment 3

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

Attachment: H.264 profile-level-id's in SDP have no leading '0x'

Comment 4

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

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

Comment 5

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

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

Comment 6

[Nils Ohlmeier [:drno]]

Attachment: bug_1042791_verify_h264_profile_level_id.patch

Test case verifying this fix.

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 8462333 [details] [diff] [review]
bug_1042791_verify_h264_profile_level_id.patch

lgtm

Comment 8

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

Comment on attachment 8461019 [details] [diff] [review]
H.264 profile-level-id's in SDP have no leading '0x'

For both patches here

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: No interop with other H.264 devices

[Describe test coverage new/current, TBPL]: See second patch

[Risks and why]: none

[String/UUID change made/needed]: none

Comment 9

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

Comment on attachment 8462333 [details] [diff] [review]
bug_1042791_verify_h264_profile_level_id.patch

Review of attachment 8462333 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/test_peerConnection_bug1042791.html
@@ +29,5 @@
> +      "PC_LOCAL_VERIFY_H264_OFFER",
> +      function (test) {
> +        ok(!test.pcLocal._last_offer.sdp.contains("profile-level-id=0x42e00c"),
> +          "H264 offer does not contain profile-level-id=0x42e00c");
> +        ok(test.pcLocal._last_offer.sdp.contains("profile-level-id=42e00c"),

Note: bug 1043515 will change this to be either 42e00c or 42e00d - and note the test should be case-insensitive.  I'd ignore the last 2 hex digits (both can change), and make it insensitive, before landing.  rs=me for that change

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Randell, 32 is marked as affected. Why don't you request uplift? Thanks

Comment 11

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

Attachment: check that H264 offer contains correct profile-level-id

Updated test to no be broken by already-landed changes to H.264 level or future changes; ready to land

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a131de723ce

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a131de723ce

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a131de723ce

Comment 15

[Nils Ohlmeier [:drno]]

Attachment: Shortened level id, case insensitive and activate test

Try run: https://tbpl.mozilla.org/?tree=Try&rev=1fc3b99cb651

Comment 16

[Nils Ohlmeier [:drno]]

Try build is green. Requesting checkin of attachment 8464085 [details] [diff] [review]

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f2ec90f7570

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f2ec90f7570

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/9730317edd1a

We should take this for beta too; will ask

Comment 20

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

Comment on attachment 8461019 [details] [diff] [review]
H.264 profile-level-id's in SDP have no leading '0x'

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Problems in interop between H.264 on 8x10 (Flame/etc) 2.0 devices and spec-compliant target endpoints, such as Cisco equipment, and need to include workarounds in our OpenH264 implementations in 33+.

[Describe test coverage new/current, TBPL]: Associated test should land as well

[Risks and why]: none

[String/UUID change made/needed]: none

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8461019 [details] [diff] [review]
H.264 profile-level-id's in SDP have no leading '0x'

Changing the approval request to be b2g32 as Randell confirmed that this fix is not required for Firefox 32.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

[Blocking Requested - why for this release]:

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8461019 [details] [diff] [review]
H.264 profile-level-id's in SDP have no leading '0x'

Erm, should have set the b2g blocking nom flag instead of approval. Justification is in comment 20.

Comment 24

[Preeti Raghunath(:Preeti)]

Given this is a new feature for 2.0.(H.264) Triage is blocking  for broken new feature.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d3b6c8598542
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3bc917772c0d

I've got the test queued up to land on Aurora as well the next time I'm pushing there.

Comment 26

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

This newly added test is failing on b2g32: https://tbpl.mozilla.org/php/getParsedLog.php?id=45282717&tree=Mozilla-B2g32-v2.0

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out from b2g32 for mochitest failures.
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6fbc60a80c6d

https://tbpl.mozilla.org/php/getParsedLog.php?id=45282717&tree=Mozilla-B2g32-v2.0

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Landed the test on Aurora (after a green Try run):
https://hg.mozilla.org/releases/mozilla-aurora/rev/7097c49c14e1

Comment 29

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

Revised test to work properly on B2G 2.0 (don't rely on GMP; don't try it on non-B2G)
https://tbpl.mozilla.org/?tree=Try&rev=39dd483ecb68

Comment 30

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

The tests do not run properly on b2g emulator on b2g 2.0; H264 support there is predicated on OMX H.264 support, which the emulator does not have, and so I can't get it to generate a valid H.264 offer.
This is a trivial change, and is well-tested on other branches/platforms.

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4db2f97f697e