seeking in mp4 with aac is not frame accurate

RESOLVED FIXED in Firefox 47

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cus, Assigned: jya)

Tracking

44 Branch
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8714025 [details]
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).

Updated

2 years ago
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8714066 [details] [diff] [review]
Offset seek time by start time.

We adjust the timestamps as frames are returned by the decoder, but weren't adjusting the seek time.
Attachment #8714066 - Flags: review?(jwwang)
(Assignee)

Updated

2 years ago
Assignee: nobody → jyavenard
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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+
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
(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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/16cb550a680d
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16cb550a680d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.