Fragmented mp4 parser should store sample description entries and use the appropriate entry per fragment based on the track fragment header

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P1
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: bryce, Assigned: bryce)

Tracking

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(6 attachments)

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
See Also: → 1516481
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.