Closed
Bug 1515471
Opened 5 years ago
Closed 5 years ago
Fragmented mp4 parser should store sample description entries and use the appropriate entry per fragment based on the track fragment header
Categories
(Core :: Audio/Video: Playback, enhancement, P1)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: bryce, Assigned: bryce)
References
Details
Attachments
(6 files)
7.08 KB,
application/x-zip-compressed
|
Details | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
An mp4 may contain multiple sample entries in the sample description box (stsd). These entries are used to describe samples in that mp4. Typically, an mp4 would contain one entry, though I have been noticing files include 2 while working with encrypted media -- one for plain data, one for encrypted. Our mp4 parser maps multiple sample entries to a single TrackInfo, this has historically been okay. However, a more robust approach would be to store all the sample entries and look them based on the `sample_description_index` field in the track fragment header box (tfhd). This is now a concern for handling cbcs encrypted media. We've previously used the presence of other data to determine the crypto status within fragments. These include the presence of pssh[0] and saio+saiz[1] data. However, for the cbcs scheme, both of these need not be present. In these cases we will fail to identify encrypted samples appropriately. See attachment for an example of cbcs files that this is problematic for. Things to note: - These files were created with shaka-packager, a Notes.txt details the specific command used. - The init segment contains multiple sample entries in the stsd box. - The first fragment is not encrypted and references the second sample entry, that lacks encryption info. - The second fragment is encrypted and references the first sample entry, which contains crypto info. - The second fragment lacks pssh, saio, and saiz boxes. To be clear, this bug is not about cbcs, but fixing this should unblock ongoing work on cbcs (as well as making us more robust in general). The attached file is used as an example of why we need to handle this case. [0]: https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/dom/media/mp4/Index.cpp#115 [1]: https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/dom/media/mp4/Index.cpp#128
Assignee | ||
Comment 1•5 years ago
|
||
Initially I had hope that the information parsed by mp4parse-rust could be used in our fragmented parser. After bug 1513042 landed, we are now able to read all the sample description entries from the rust parser. However, it may be more timely to update the moof parser to do this instead -- this would require less reworking of code. The moof parser already does some reading of the stsd box to pick up crypto info[0], and this logic could be expanded to pick up relevant information from sample description entries. [0]: https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/dom/media/mp4/MoofParser.cpp#302
Assignee | ||
Comment 2•5 years ago
|
||
Bug 1516481 has been created for a more involved follow up to this, time permitting. Per comment #1, I'm going to focus on a more timely unblocking of cbcs here.
Assignee | ||
Comment 3•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=520e1e0d655a3c15faeff65f818980e95ba300c2
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e656a9e919657f8f05db660cd2f83d7396347b7d
Assignee | ||
Comment 5•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98b38d28d44e20101bd83998d244a08649993b3e
Assignee | ||
Comment 6•5 years ago
|
||
The following code appears to be dead and is removed: - The AuxInfo class. - Moof::ParseSaiz. - Moof::ParseSaio. - MoofParser::ParseSinf.
Assignee | ||
Comment 7•5 years ago
|
||
This is required to disambiguate if samples should be considered encrypted or not when parsing certain cbcs encrypted files. Unlike with cenc encryption, cbcs encrypted media may have fragments that lack characteristics from which we can infer encryption. Because of this we need to store and trust this information from the sample description box. Depends on D15438
Assignee | ||
Comment 8•5 years ago
|
||
Persisting this box lets us use is later while indexing into metadata. Importantly, it lets us look up the appropriate sample description entry, which lets us determine if a fragment is associated with crypto information in the init segment. Being able to do this is required for cbcs encryption. This will be done in a follow up bug: Bug 1487416 (and possibly others). Depends on D15439
Assignee | ||
Comment 9•5 years ago
|
||
Driveby tidying: - Tidy and clarify comment impacted by global clang-formatting. - Update comments on two test cases disabled by bug 1224019. These tests no longer have log spam issues, and have some value as sanity tests, but remain disabled due to being resource hogs. Comments indicate this. Depends on D15440
Assignee | ||
Comment 10•5 years ago
|
||
Depends on D15441
Assignee | ||
Comment 11•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d97cb2bae013ba511984fe361ae83d500f6ce34
Comment 12•5 years ago
|
||
Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9386061fb62a Remove unused code from MoofParser.h r=jya https://hg.mozilla.org/integration/autoland/rev/6bcfd324b867 Have the moof parser store if sample description entries contain crypto info. r=jya https://hg.mozilla.org/integration/autoland/rev/e9b36f8f6c22 Persist the track fragment header box on the Moof class. r=jya https://hg.mozilla.org/integration/autoland/rev/416d0d25d932 Tidy mp4parser test cases. r=jya https://hg.mozilla.org/integration/autoland/rev/8bbbc4ae3211 Add gtest for mp4 sample description entry lookup. r=jya
Comment 13•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9386061fb62a https://hg.mozilla.org/mozilla-central/rev/6bcfd324b867 https://hg.mozilla.org/mozilla-central/rev/e9b36f8f6c22 https://hg.mozilla.org/mozilla-central/rev/416d0d25d932 https://hg.mozilla.org/mozilla-central/rev/8bbbc4ae3211
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•