Closed Bug 1042791 Opened 6 years ago Closed 6 years ago

H264 SDP in WebRTC has non-standard fmtp line

Categories

(Core :: WebRTC, defect)

34 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- unaffected
firefox33 --- fixed
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: alan.ford, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ft:loop])

Attachments

(3 files, 1 obsolete file)

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
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
QUite right, thanks!
Assignee: nobody → rjesup
Blocks: OpenH264
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [webrtc-uplift][ft:loop][openh264-uplift]
Attachment #8461019 - Flags: review?(ethanhugg)
Attachment #8461019 - Flags: review?(ethanhugg) → review+
https://hg.mozilla.org/mozilla-central/rev/e95cd62d1a96
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Test case verifying this fix.
Attachment #8462333 - Flags: review?(ethanhugg)
Comment on attachment 8462333 [details] [diff] [review]
bug_1042791_verify_h264_profile_level_id.patch

lgtm
Attachment #8462333 - Flags: review?(ethanhugg) → review+
See Also: → 1044335
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?
Attachment #8462333 - Flags: approval-mozilla-aurora?
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)
Flags: in-testsuite+
Updated test to no be broken by already-landed changes to H.264 level or future changes; ready to land
Attachment #8464085 - Flags: review?(rjesup) → review+
Try build is green. Requesting checkin of attachment 8464085 [details] [diff] [review]
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/9730317edd1a

We should take this for beta too; will ask
Flags: needinfo?(rjesup)
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?
Given this is a new feature for 2.0.(H.264) Triage is blocking  for broken new feature.
blocking-b2g: 2.0? → 2.0+
Landed the test on Aurora (after a green Try run):
https://hg.mozilla.org/releases/mozilla-aurora/rev/7097c49c14e1
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
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
Whiteboard: [webrtc-uplift][ft:loop][openh264-uplift] → [ft:loop][openh264-uplift]
Whiteboard: [ft:loop][openh264-uplift] → [ft:loop]
You need to log in before you can comment on or make changes to this bug.