Bug 1715617 Comment 0 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

The [`mHeader` member](https://searchfox.org/mozilla-central/rev/79d73b4aff88dd4a0f06dd3789e1148c49b0de60/dom/media/gtest/mp4_demuxer/TestParser.cpp#170) is confusing. In my opinion, the naming suggests that it indicates if a test file includes a header or not. However, it's actually checking for a specific kind of header (and even then is doing so indirectly), and this results in people like myself footgunning themselves.

What the value is actually checked against is [`MoofParser::FirstCompleteMediaHeader`](https://searchfox.org/mozilla-central/rev/79d73b4aff88dd4a0f06dd3789e1148c49b0de60/dom/media/gtest/mp4_demuxer/TestParser.cpp#170), and is used to check if that function returns a zero or non-zero value. Essentially `mHeader` being true implies `FirstCompleteMediaHeader` should return a non-zero value.

`FirstCompleteMediaHeader` returns a non-zero value when the parser has parsed a `moof` box. The `moof` box is a movie fragment box, which must contain a `mfhd` movie fragment header. In that sense, a non-zero value implies a header is present, because having a `moof` implies a fragment header.

However, there are other kinds of headers in an mp4 -- the `moov` box must contain a `mvhd` (movie header box), which is also a header. It may be a bad heuristic on my behalf, but this is the header I first think of when thinking of mp4s, not the fragment header. This leads to ambiguity with terms like 'media header'. In the case of this test, `mHeader` is checking for a media fragment header. So I think it should be named something like `mHasMoof` or `mHadMediaFragmentHeader`.
The [`mHeader` member](https://searchfox.org/mozilla-central/rev/79d73b4aff88dd4a0f06dd3789e1148c49b0de60/dom/media/gtest/mp4_demuxer/TestParser.cpp#170) is confusing. In my opinion, the naming suggests that it indicates if a test file includes a header or not. However, it's actually checking for a specific kind of header (and even then is doing so indirectly), and this results in people like myself footgunning themselves.

What the value is actually checked against is [`MoofParser::FirstCompleteMediaHeader`](https://searchfox.org/mozilla-central/rev/79d73b4aff88dd4a0f06dd3789e1148c49b0de60/dom/media/gtest/mp4_demuxer/TestParser.cpp#170), and is used to check if that function returns an empty or non-empty range. Essentially `mHeader` being true implies `FirstCompleteMediaHeader` should return a non-empty range.

`FirstCompleteMediaHeader` returns a non-empty range when the parser has parsed a `moof` box. The `moof` box is a movie fragment box, which must contain a `mfhd` movie fragment header. In that sense, a non-empty value implies a header is present, because having a `moof` implies a fragment header.

However, there are other kinds of headers in an mp4 -- the `moov` box must contain a `mvhd` (movie header box), which is also a header. It may be a bad heuristic on my behalf, but this is the header I first think of when thinking of mp4s, not the fragment header. This leads to ambiguity with terms like 'media header'. In the case of this test, `mHeader` is checking for a media fragment header. So I think it should be named something like `mHasMoof` or `mHadMediaFragmentHeader`.

Back to Bug 1715617 Comment 0