Closed Bug 1244477 Opened 4 years ago Closed 4 years ago

seeking in mp4 with aac is not frame accurate

Categories

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

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: cus, Assigned: jya)

Details

Attachments

(2 files)

Attached file 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).
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
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.
We adjust the timestamps as frames are returned by the decoder, but weren't adjusting the seek time.
Attachment #8714066 - Flags: review?(jwwang)
Assignee: nobody → jyavenard
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.
Attachment #8714066 - Flags: review?(jwwang) → review+
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.
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/16cb550a680d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.