Mp4s with multiple sample description entries containing crypto are rejected by parser
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox91 | --- | fixed |
People
(Reporter: huseyin, Assigned: bryce)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:88.0) Gecko/20100101 Firefox/88.0
Steps to reproduce:
We have realized some of the DRM content not playing on Firefox while it plays on Chrome. Same content without protection plays as expected.
https://test.unified-streaming.com/tkt32329/clear.ism/.mpd - Plays with all browsers.
https://test.unified-streaming.com/tkt32329/drm.ism/.mpd - Plays in Chrome but not on Firefox (MEDIA_ERR_SRC_NOT_SUPPORTED (NS_ERROR_FAILURE (0x80004005)) )
Test Player
https://reference.dashif.org/dash.js/v3.2.2/samples/dash-if-reference-player/index.html
Widevine license server
https://widevine-proxy.appspot.com/proxy
Actual results:
We get different results from different browsers. The error message below appears on the DASH-IF Player once try to get playback
(MEDIA_ERR_SRC_NOT_SUPPORTED (NS_ERROR_FAILURE (0x80004005)) )
Expected results:
It should just play as it plays on Chrome.
|
||
Comment 1•3 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'Core::Audio/Video: Playback' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.
|
||
Comment 2•3 years ago
|
||
Bryce, could you help me triage this DRM issue?
Thank you.
|
Assignee | |
Comment 3•3 years ago
|
||
I believe this is because the file contains multiple sample descriptions in the stsd box, each with crypto information. This isn't strictly forbidden by the spec, and is something we anticipated could happen, but that we don't handle as gracefully as we could. We reject these files during parsing, but we could be more tolerant.
We can handle some of these files, and I think the one in this bug works okay, because the different sample description use the same crypto info. If they had different crypto info and relied on that, then I don't think gecko would handle it properly.
I'll work on a patch.
|
|
Assignee | |
Comment 4•3 years ago
|
||
We don't need to explicitly reject such files, we can try to play them, and
provided the sample description entires specify the same crypto data, we should
be fine. However, if different entries use different crypto, we will likely run
into problems.
This patch fixes the blanket rejecting and lets us handle cases where the crypto
in each entry matches. Bug 1714626 tracks handling different crypto data. FWIW I
have never seen different crypto per sample description entry, and handling that
is a much bigger task, so this patch doesn't seek to do so.
|
||
Updated•3 years ago
|
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 6•3 years ago
|
||
This is an init segment with multiple sample description entries, each with crypto information. This is what triggers the issue. Reproducing here for future reference. I plan to construct a gtest from this.
|
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 7•3 years ago
|
||
Add an init segment with 2 sample description entries, each with crypto data, to
the mp4 parser gtests.
2 test cases are updated:
- We ensure the init data parses with expected results as part of our general
'does this header parse, and with what result' style test. - We ensure expected telemetry data is gather as part of our specialized
telemetry tests. Specifically, this adds coverage to the previously uncovered
multiple sample description entries with crypto case.
Note the test case has the same crypto info in both entires, so these tests do
not cover the case where we need to handle different crypto info per sample
description.
|
|
Assignee | |
Comment 8•3 years ago
|
||
Also add a TODO to rework telemetry gathering as part of broader mp4 parsing
reworks.
|
|
||
Updated•3 years ago
|
Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0ed3355e9f7d Add multiple sample descriptions with crypto testcase to mp4 parser gtests. r=jbauman https://hg.mozilla.org/integration/autoland/rev/07c910af0750 Don't reject mp4s that have more than one sample description entry with crypto. r=jbauman https://hg.mozilla.org/integration/autoland/rev/33fe5ea6200d Use an int count instead of bools to detect multiple crypto entries. r=jbauman
|
||
Comment 10•3 years ago
|
||
Backed out 3 changesets (bug 1714125) for Gtest failures in MoofParser.test_case_mp4. CLOSED TREE
Log:
https://treeherder.mozilla.org/logviewer?job_id=342170480&repo=autoland&lineNumber=30741
Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=33fe5ea6200dcb4f1a0af3c099923b4b332f6fba
Backout:
https://hg.mozilla.org/integration/autoland/rev/c03681b5a622cb6c94310eddb50241b2f39632b0
|
|
Assignee | |
Comment 11•3 years ago
|
||
Thanks for NI. I'd forgotten to relax some test expectations around yet another of our mp4 parsers. Working to resolve.
|
||
Comment 12•3 years ago
|
||
Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a13f4080610d Add multiple sample descriptions with crypto testcase to mp4 parser gtests. r=jbauman https://hg.mozilla.org/integration/autoland/rev/894eac5f7f4d Don't reject mp4s that have more than one sample description entry with crypto. r=jbauman https://hg.mozilla.org/integration/autoland/rev/10b2d7a756ab Use an int count instead of bools to detect multiple crypto entries. r=jbauman
|
||
Comment 13•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a13f4080610d
https://hg.mozilla.org/mozilla-central/rev/894eac5f7f4d
https://hg.mozilla.org/mozilla-central/rev/10b2d7a756ab
Description
•