1244477 - seeking in mp4 with aac is not frame accurate

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[cus]

Attachment: Testcase for seeking

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

In the attached HTML example, press the first button, which seeks to 1.16, then the second button, which seeks +1 frame (0.04 sec).


Actual results:

After pressing the second button (seek +1 frame) the video seeked to +2 frames to timecode 00:00:01:05.


Expected results:

The video should have seeked to +1 frame. (timecode 00:00:01:04).

Comment 1

[Jean-Yves Avenard [:jya]]

There are two issues at hand.

The first one which gives confusing results is that this file is improperly muxed.
(confirmed with the command: ffprobe -show_entries packet=pts_time,duration_time,stream_index test.mp4)

The first frame of the audio track starts at -0.023220
And the first frame of the video track starts at 0.

In order to be spec conformant so that the timeline of each video starts at 0, we adjust the time of each sample by the start time (here -0.023220).
So when seeking at time 1.16, we are looking for the frames with a start time of 1.16+(-0.023220) = 1.136781

Now, there is an issue in our code, we don't adjust the seek time by the start time (we only adjust the time of the frames returned) So when seeking to 1.16 we are actually seeking to 1.16-(-0.023220)
Which makes us appear to seek to the next frame instead.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Offset seek time by start time.

We adjust the timestamps as frames are returned by the decoder, but weren't adjusting the seek time.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8714066 [details] [diff] [review]
Offset seek time by start time.

Review of attachment 8714066 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1642,5 @@
>  
>    // Do the seek.
>    RefPtr<MediaDecoderStateMachine> self = this;
>    mSeekRequest.Begin(InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__,
> +                                 &MediaDecoderReader::Seek, mCurrentSeek.mTarget.mTime + StartTime(),

I always though it is up to the reader to adjust the seek time by the start time since MDSM calls MediaDecoderReader::DispatchSetStartTime() to tell it about the start time.

Comment 4

[Jean-Yves Avenard [:jya]]

Seeing that it's the MDSM that adjust the time on On[Audio|Video]Decoded it made sense to me to continue doing so in the MDSM.

Do you think it should be done somewhere else?

Seeing that there are multiple MediaDecoderReader the change would be more extensive.

Alternatively, could mention in the documentation that the time received is to be in the reader's own reference.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #3)
> I always though it is up to the reader to adjust the seek time by the start
> time since MDSM calls MediaDecoderReader::DispatchSetStartTime() to tell it
> about the start time.

IRC, this is done for GetBuffered() as the buffered range returned by the reader is in the media element reference.

It's a bit inconsistent thinking about it.. Maybe the adjustment should be done in the MDSM too.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16cb550a680d

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

I think we can create a wrapper to put all the start time adjustment code in one place so MDSM always deals with adjusted times while readers with unadjusted ones.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16cb550a680d