1168004 - MediaFormatReader doesn't handle mStartTime in GetBuffered

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Jean-Yves Avenard [:jya]]

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

Comment 4

[Bobby Holley (:bholley)]

(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.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Ensure buffered range referential starts at 0

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

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Jean-Yves Avenard [:jya]]

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.

Comment 8

[Bobby Holley (:bholley)]

(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 9

[Bobby Holley (:bholley)]

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. :-)

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: B1;2cug : Part2. Add sample file

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 11

[Bobby Holley (:bholley)]

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?

Comment 12

[Jean-Yves Avenard [:jya]]

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.

Comment 13

[Jean-Yves Avenard [:jya]]

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..

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Add sample file

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

Comment 15

[Pulsebot]

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

Comment 16

[Phil Ringnalda (:philor)]

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