Remove mp4 TestParser.cpp:TestFileData::mHeader because mValidMoof does the same thing
Categories
(Core :: Audio/Video: Playback, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox91 | --- | fixed |
People
(Reporter: bryce, Assigned: bryce)
Details
Attachments
(1 file)
The mHeader member 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, 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.
|
Assignee | |
Comment 1•4 years ago
|
||
I think we can do better yet and just remove mHeader. If I'm not mistaken, mHeader is just a 'do we expect to parse a moof?' flag. mValidMoof is already that flag. There's no test cases where these flags are inconsistent, so I don't know if it's worth making a distinction (if it is we can add a flag back in as needed).
|
|
Assignee | |
Comment 2•4 years ago
|
||
As far a I can tell, mValidMoof does the same job as mHeader and has a clearer
name. Let's just remove mHeader and make the tests a little simpler. Longer
discussion below about why we may want to separate the flags, but I'm fairly
confident that it does not apply to our current tests.
Hypothetically it may make sense to separate these flags. mValidMoof is checked
against RebuildFragmentedIndex, while mHeader is checking if the moof parser has
at least one moof. If we RebuildFragmentedIndex once and it returns true, that
implies we have a least one moof. That is the state of all our current test
IIUC. So there is no need to have separate flags. However, if we call
RebuildFragmentedIndex multiple times, and it succeeds, then fails, and we don't
evict moofs, then we could have times where the parser still has moofs stored
internally, even if we have a failed Rebuild. But we're not doing that as far as
I can tell in these tests.
|
||
Comment 4•4 years ago
|
||
| bugherder | ||
Description
•