This is a regression from bug 1163485. MediaFormatReader didn't copy the logic from MP4Reader handling mStartTime in GetBuffered, and so the buffered range will be wrong for any mp4 media with start time != 0. This went unnoticed because we don't have any tests for mp4 with non-zero start time. Our non-zero-startTime test coverage is very spotty in general - as far as I can see, it consists of only one webm file, which doesn't have an audio track. So with this bug, we should also add an mp4 test case with both audio and video channels, neither of whose first sample begins at 0. Plugging that in to test_playback.html will give us the broadest range of test coverage for this case.

I need the test coverage here to properly verify my work on bug 1163223.

jya - is this something you could take care of in the near term so that I can move forward with bug 1163223?

I'll fix this one ASAP. That's independent of 1163223 I think.

(In reply to Jean-Yves Avenard [:jya] from comment #3) > I'll fix this one ASAP. That's independent of 1163223 I think. Kinda, but I'm not comfortable landing bug 1163223 without at least one testcase with both an audio and video channel, so I need to wait on the test coverage for this bug.

Adjust timeranges to be in a referential that starts at 0.

Comment on attachment 8609935 [details] [diff] [review] Ensure buffered range referential starts at 0 Review of attachment 8609935 [details] [diff] [review]: ----------------------------------------------------------------- Can you add the test coverage discussed in comment 0? This codepath is entirely untested at the moment, unfortunately. Some (non-MSE) mini version of bipbop with offset timestamps would do the trick, I think. ::: dom/media/MediaFormatReader.cpp @@ -1283,3 @@ > } > if (NS_IsMainThread()) { > - if (!mCachedTimeRangesStale) { Hm, the context here doesn't match what I see on central or inbound. Is this based on another patch that has yet to be landed? I'd like to see the base revision in order to interpret the diff correctly.

I'll push to inbound bug 1163227 and bug 1166836 which is what this bug is based on. Do we have a way to retrieve the mStartTime value in JS ? or do I have to use gtest? I will add tests in a separate patch.

(In reply to Jean-Yves Avenard [:jya] from comment #7) > I'll push to inbound bug 1163227 and bug 1166836 which is what this bug is > based on. Ok, I'll have a look at those. > Do we have a way to retrieve the mStartTime value in JS ? or do I have to > use gtest? No, but it's observable by examining the end time and buffered range. test_playback.html already does this (which, along with split.webm, gives us basic smoketesting of start time handling for webm). So I don't think we actually need to add any more tests per se, just another mp4 file in gPlayTests.

Comment on attachment 8609935 [details] [diff] [review] Ensure buffered range referential starts at 0 Review of attachment 8609935 [details] [diff] [review]: ----------------------------------------------------------------- Nice. r=me assuming the landing includes tests. :-)

Sample with video starting at 1s and audio at 2s. From a quick testing, only QuickTime and Safari appears to play that file properly. All the other browsers and players (including ffmpeg, VLC and mplayer) shows the audio starting at the same time at the video and A/V sync is off by 1s. Interestingly, when I play that file converted to fragmented: we play it fine. But in plain MP4 (with moov at the end), then it's wrong. Looking at the timestamp of the first audio samples being returned from the MP4 demuxer has a time of 0, which is plain wrong.

Comment on attachment 8609983 [details] [diff] [review] B1;2cug : Part2. Add sample file Review of attachment 8609983 [details] [diff] [review]: ----------------------------------------------------------------- Can you add this to gPlayTests in manifest.js and make sure that the test passes? r=me if that's the case. Also, I'm assuming the duration is something short, and not the full 10 (or 9) seconds of bipbop?

We incorrectly read that file. I suspect a bug in the IndexIterator. Problem is that ffmpeg insert an invalid entry in the edts, it may be what's confusing everything. Duration is 2s If we pass the test, it will be meaningless really.

actually, the edts generated by ffmpeg is perfectly valid. for video we have: [edts] size=8+28 [elst] size=12+16 entry count = 1 entry/segment duration = 2402 entry/media time = 3000 entry/media rate = 1 so the first frame start with timestamp 3000 for audio we have: [edts] size=8+40 [elst] size=12+28 entry count = 2 entry/segment duration = 1148 entry/media time = -1 entry/media rate = 1 entry/segment duration = 1951 entry/media time = 0 entry/media rate = 1 timescale here is 0. so we have the first 1.148s of silence , and then the first audio frame with a timestamp of 0. This is what ISO 14496-12 states: " Starting offsets for tracks (streams) are represented by an initial empty edit. For example, to play a track from its start for 30 seconds, but at 10 seconds into the presentation, we have the following edit list: Entry-count = 2 Segment-duration = 10 seconds Media-Time = -1 Media-Rate = 1 Segment-duration = 30 seconds (could be the length of the whole track) Media-Time = 0 seconds Media-Rate = 1 " our demuxer/stagefright don't properly handle this and ignores the empty edit list entry..

Including in the list of tested file. This will always pass regardless of the first change or not as the first audio frame as a timestamp of 0, so mStartTime is always 0

https://hg.mozilla.org/integration/mozilla-inbound/rev/652f4b7aaaea https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9fbfd11bfb

https://hg.mozilla.org/mozilla-central/rev/652f4b7aaaea https://hg.mozilla.org/mozilla-central/rev/4e9fbfd11bfb