Open Bug 1516481 Opened 5 years ago Updated 2 years ago

Reduce duplication between different mp4parse-rust and MoofParser

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: bryce, Unassigned)

References

Details

The Firefox codebase has mp4 parsing code in several different locations. In particular I want to highlight the following:
- mp4parse-rust[0] - this is currently used to parse metadata and surface it via MP4Metadata.cpp[1].
- MoofParser[2] - this handles parsing of movie fragments, but has also grown to parse various metadata. As such, it is responsible for more than just parsing the movie fragment box (moof), as the name may imply.
- Index[3] - this handles matching (indexing) various metadata to samples being read. It makes use of the MoofParser. It does not appear to currently have much knowledge of metadata from mp4parse-rust.

Between the code from these components, there is a fair amount of duplication for metadata handling. While the above sets of code are utilized by code living in `dom/media/mp4`, our abstractions and data handling can mean that even though mp4parse-rust surfaces some data, we do not forward it to Index and moof parsing code. This then means the MoofParser has to do the same parsing again to be aware of pertinent metadata.

For example, we update track information from mp4parse-rust[4]. In doing so we expose crypto information, but it is not plumbed all the way through where it's needed for handling of samples from fragments. So the MoofParser has code to parse the same boxes as mp4parse-rust does[5][6].

I would be good to reduce duplication such as this. It would help with code clarity, and it won't hurt if we're not spending cycles parsing the same data multiple times.

[0]: https://github.com/mozilla/mp4parse-rust
[1]: https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/dom/media/mp4/MP4Metadata.cpp
[2]: https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/dom/media/mp4/MoofParser.h
[3]: https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/dom/media/mp4/Index.cpp
[4]: https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/dom/media/mp4/MP4Metadata.cpp#250
[5]: https://searchfox.org/mozilla-central/rev/da45a6b1c560ff4629534c966b15500b9fe7af2b/dom/media/mp4/MoofParser.cpp#314
[6]: https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/dom/media/mp4/MoofParser.cpp#301

While I'm digging around in the mp4 parser: MP4Metadata only considering the presence of the pssh box for crypto data is kind of confusing, and leads to weird cases where there may not be a pssh box in the header, but there may be a sinf box, so MP4Metadata will say there's not crypto, but things like the Moof parser will report there is crypto data.

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.