MP4Demuxer::Init() creates a minimal MP4Metadata structure, and report success/failure from that alone. But other later-called functions (e.g.: GetNumberTracks, GetTrackDemuxer, etc.) could still fail with no useful error reporting, when MP4Metadata tries to gather more of the needed information. Also, MP4Demuxer needs to keep this MP4Metadata object forever, even though it could contain an arbitrary amount of extra data that will never be needed. Instead, Init() should just extract all the required data in advance, possibly catching and reporting potential errors, and discard the MP4Metadata that is not needed anymore.

Comment on attachment 8839714 [details] Bug 1341454 - MP4Demuxer::Init() pre-caches everything from MP4Metadata - https://reviewboard.mozilla.org/r/114290/#review115932 ::: dom/media/fmp4/MP4Demuxer.cpp:159 (Diff revision 1) > - __func__); > + __func__); > } > > + if (audioTrackCount != 0) { > + UniquePtr<TrackInfo> info = metadata.GetTrackInfo(TrackInfo::kAudioTrack, 0); > + if (!info) { this would be a regression on the current behaviour which is to ignore invalid tracks: audio: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1302 video: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1274 we only cause an error for a valid video track but with an unsupported codec. ::: dom/media/fmp4/MP4Demuxer.cpp:177 (Diff revision 1) > + mAudioDemuxer = new MP4TrackDemuxer(this, Move(info), indices); > + } > + > + if (videoTrackCount != 0) { > + UniquePtr<TrackInfo> info = metadata.GetTrackInfo(TrackInfo::kVideoTrack, 0); > + if (!info) { same problem as with audio, now invalid tracks make the stream unplayable ::: dom/media/fmp4/MP4Demuxer.cpp:215 (Diff revision 1) > } > > uint32_t > MP4Demuxer::GetNumberTracks(TrackInfo::TrackType aType) const > { > - return mMetadata->GetNumberTracks(aType); > + switch (aType) { this seems to be a regression to me for future expansion where we will want one day to support multiple tracks. We know force the use of the first track only.

Comment on attachment 8839714 [details] Bug 1341454 - MP4Demuxer::Init() pre-caches everything from MP4Metadata - https://reviewboard.mozilla.org/r/114290/#review115932 > this would be a regression on the current behaviour which is to ignore invalid tracks: > > audio: > https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1302 > > video: > https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1274 > > we only cause an error for a valid video track but with an unsupported codec. ... > same problem as with audio, now invalid tracks make the stream unplayable I've removed these checks. But it means this whole work may be less useful than I thought!? After I've gone through all tracks, I could add a check that there is at least one valid track overall... What do you think? > this seems to be a regression to me for future expansion where we will want one day to support multiple tracks. > > We know force the use of the first track only. Ok I'm now storing all provided tracks, which should just be 1 or 2 in real life; and since I'm discarding the MP4Metadata, the space taken should be equivalent in the end.

Comment on attachment 8839714 [details] Bug 1341454 - MP4Demuxer::Init() pre-caches everything from MP4Metadata - https://reviewboard.mozilla.org/r/114290/#review116278 ::: dom/media/fmp4/MP4Demuxer.cpp:159 (Diff revision 2) > - __func__); > + __func__); > } > > + if (audioTrackCount != 0) { > + mAudioDemuxers.SetLength(audioTrackCount); > + for (size_t i = 0; i < audioTrackCount; ++i) { I dont like ++i :) IF there's any loop in this code it will be i++ ::: dom/media/fmp4/MP4Demuxer.cpp:208 (Diff revision 2) > > uint32_t > MP4Demuxer::GetNumberTracks(TrackInfo::TrackType aType) const > { > - return mMetadata->GetNumberTracks(aType); > + switch (aType) { > + case TrackInfo::kAudioTrack: return uint32_t(mAudioDemuxers.Length()); Do you need the cast here?

Comment on attachment 8839714 [details] Bug 1341454 - MP4Demuxer::Init() pre-caches everything from MP4Metadata - https://reviewboard.mozilla.org/r/114290/#review116278 > I dont like ++i :) > > IF there's any loop in this code it will be i++ '++i' is a good habit, because at worst it ends up being exactly the same as 'i++' (like in this case: incrementing a number but the discarding the result), but in some cases it could be more efficient because it doesn't need to make a copy before incrementing (as would be necessary for more complex types). Some old article showed that 'i++' could significantly slow down non-optimized builds -- Don't you think non-optimized Firefox is slow enough? :-P Anyway, it's not important here, because it will typically increment once per track, so I'm fine with using the less-efficient option, for consistency. > Do you need the cast here? Length() is a size_t, which can be 64 bits, so we do need* a cast to a 32-bit value. * Actually, I'm not sure we currently warn/error about silent number truncations; but I think we should becasue it has caused issues in the past, so better be safe, or at least explicit when we're doing it!

Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aa4f4a00727d MP4Demuxer::Init() pre-caches everything from MP4Metadata - r=jya