H264 SDP in WebRTC has non-standard fmtp line

RESOLVED FIXED in Firefox 33, Firefox OS v2.0

Status

()

Core
WebRTC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Alan Ford, Assigned: jesup)

Tracking

(Blocks: 1 bug)

34 Branch
mozilla34
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox32 unaffected, firefox33 fixed, firefox34 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [ft:loop])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

4 years ago
I should clarify that this means that the SIP endpoint I have tested against is rejecting this H264 offer.
Component: Untriaged → WebRTC
Product: Firefox → Core
(Assignee)

Comment 2

4 years ago
QUite right, thanks!
Assignee: nobody → rjesup
Blocks: 948160
Status: UNCONFIRMED → ASSIGNED
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [webrtc-uplift][ft:loop][openh264-uplift]
(Assignee)

Comment 3

4 years ago
Created attachment 8461019 [details] [diff] [review]
H.264 profile-level-id's in SDP have no leading '0x'
(Assignee)

Updated

4 years ago
Attachment #8461019 - Flags: review?(ethanhugg)

Updated

4 years ago
Attachment #8461019 - Flags: review?(ethanhugg) → review+
QA Contact: drno
(Assignee)

Comment 4

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e95cd62d1a96
Target Milestone: --- → mozilla34
https://hg.mozilla.org/mozilla-central/rev/e95cd62d1a96
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Created attachment 8462333 [details] [diff] [review]
bug_1042791_verify_h264_profile_level_id.patch

Test case verifying this fix.
Attachment #8462333 - Flags: review?(ethanhugg)

Comment 7

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

lgtm
Attachment #8462333 - Flags: review?(ethanhugg) → review+

Updated

4 years ago
See Also: → bug 1044335
(Assignee)

Comment 8

4 years ago
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
Attachment #8461019 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

4 years ago
Attachment #8462333 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 9

4 years ago
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
Attachment #8462333 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8461019 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Randell, 32 is marked as affected. Why don't you request uplift? Thanks
Flags: needinfo?(rjesup)
status-firefox34: affected → fixed
Flags: in-testsuite+
(Assignee)

Comment 11

4 years ago
Created attachment 8463803 [details] [diff] [review]
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
Created attachment 8464085 [details] [diff] [review]
Shortened level id, case insensitive and activate test

Try run: https://tbpl.mozilla.org/?tree=Try&rev=1fc3b99cb651
Attachment #8463803 - Attachment is obsolete: true
Attachment #8464085 - Flags: review?(rjesup)
(Assignee)

Updated

4 years ago
Attachment #8464085 - Flags: review?(rjesup) → review+
Try build is green. Requesting checkin of attachment 8464085 [details] [diff] [review]
Keywords: checkin-needed
(Assignee)

Comment 19

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

We should take this for beta too; will ask
status-firefox33: affected → fixed
Flags: needinfo?(rjesup)
(Assignee)

Comment 20

4 years ago
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
Attachment #8461019 - Flags: approval-mozilla-beta?
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.
Attachment #8461019 - Flags: approval-mozilla-beta? → approval-mozilla-b2g32?
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
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.
Attachment #8461019 - Flags: approval-mozilla-b2g32?
status-b2g-v2.0: --- → affected
status-firefox32: affected → unaffected
Given this is a new feature for 2.0.(H.264) Triage is blocking  for broken new feature.
blocking-b2g: 2.0? → 2.0+
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.
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: --- → fixed
Landed the test on Aurora (after a green Try run):
https://hg.mozilla.org/releases/mozilla-aurora/rev/7097c49c14e1
(Assignee)

Comment 29

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

Comment 30

4 years ago
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
status-b2g-v2.0: affected → fixed
Whiteboard: [webrtc-uplift][ft:loop][openh264-uplift] → [ft:loop][openh264-uplift]
(Assignee)

Updated

4 years ago
Whiteboard: [ft:loop][openh264-uplift] → [ft:loop]
You need to log in before you can comment on or make changes to this bug.