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)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: bryce, Assigned: bryce)

References

Details

Attachments

(6 files)

7z containing files exemplifying problematic structure
7.08 KB, application/x-zip-compressed
Details
Bug 1515471 - Remove unused code from MoofParser.h r?jya
47 bytes, text/x-phabricator-request
Details | Review
Bug 1515471 - Have the moof parser store if sample description entries contain crypto info. r?jya
47 bytes, text/x-phabricator-request
Details | Review
Bug 1515471 - Persist the track fragment header box on the Moof class. r?jya
47 bytes, text/x-phabricator-request
Details | Review
Bug 1515471 - Tidy mp4parser test cases. r?jya
47 bytes, text/x-phabricator-request
Details | Review
Bug 1515471 - Add gtest for mp4 sample description entry lookup. r?jya
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
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
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.
The following code appears to be dead and is removed:
- The AuxInfo class.
- Moof::ParseSaiz.
- Moof::ParseSaio.
- MoofParser::ParseSinf.
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
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
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
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
Depends on: 1519683
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: