Seeking in an MP3 file plays silence for a while

VERIFIED FIXED in Firefox 54



Audio/Video: Playback
10 months ago
7 months ago


(Reporter: roc, Assigned: esawin)



Firefox Tracking Flags

(firefox54 verified)



(1 attachment, 1 obsolete attachment)

1) Load
2) Click in the controls to seek near the middle of the file

Silence plays for a while and then audio eventually resumes. There are no prolonged periods of silence in this file so this is a bug. You don't get the same silence using, say, Chrome.

mozilla-central opt build 80eac484366a.
Got this warning after seeking: [Child 75554] WARNING: Decoder=12355e800 state=SEEKING Audio not synced after seek, maybe a poorly muxed file?

When I seek to 720s, the timestamp of the 1st audio sample returned is 715768922. The 5s gap is filled with silence when the media element plays from 720s after seeking.

Something wrong to the MP3 demuxer?
Flags: needinfo?(jyavenard)
:esawin is on this
Flags: needinfo?(jyavenard)
timestamps after seeking are always estimated from the average bitrate, so it will never be exactly what you expect it to be.

That the MDSM fill with silence if you were to seek to 715s and got 720s would make sense.

But seeking to 720s, getting 715s as first frame to then fill with silence for 5s certainly doesn't. The MDSM should simply continue to request audio data until it get what it wants.
Sorry, I got it wrong.

The seek target is 710s and the 1st audio sample is 715768922 which is 5s after the seek target.

Comment 5

10 months ago
Created attachment 8835632 [details] [diff] [review]

For big VBR files, the provided TOC yields highly inaccurate offsets.

In this case, the file is at ~50MB, which yields a TOC resolution of ~505kB or ~636 frames.
In such extreme cases, it would be more accurate to rely on average-frame-based seeking, like we would with CBR files, instead of using TOC-based seeking.

For now, I'm leaving the logic as it is, but improve the ScanUntil correction procedure by adding a heuristic rewind on overshooting.
Assignee: nobody → esawin
Attachment #8835632 - Flags: review?(jyavenard)
Comment on attachment 8835632 [details] [diff] [review]

Review of attachment 8835632 [details] [diff] [review]:

::: dom/media/MP3Demuxer.cpp
@@ +259,4 @@
>    }
>    if (Duration(mFrameIndex) > aTime) {
> +    // We've seeked past the target time, rewind back a little to correct it.

how going back unconditionally corrects it in any way?
seems a bit ugly to me

@@ +259,5 @@
>    }
>    if (Duration(mFrameIndex) > aTime) {
> +    // We've seeked past the target time, rewind back a little to correct it.
> +    const int64_t rewind = 0.01 * aTime.ToMicroseconds();

aTime.ToMicroseconds() / 100;

as otherwise you get a 24 bits (size of a 32 bits float mantissa) rounding which could be bad.
Attachment #8835632 - Flags: review?(jyavenard) → review+

Comment 7

10 months ago
Created attachment 8836038 [details] [diff] [review]

Addressed comments.

As discussed on IRC, we might want to look into relying more on the TOC, when provided, instead of only using it for offset approximation to allow for time-accurate seeking.
Attachment #8835632 - Attachment is obsolete: true
Attachment #8836038 - Flags: review+

Comment 8

10 months ago
Pushed by
[1.1] Add rewind heuristic when seeking past the target time. r=jya

Comment 9

10 months ago
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: qe-verify+
I have reproduced the issue on a Nightly 54.0a1 (id: 20170207110148) build and can confirm that the issue is fixed in Firefox 54.0b1 on an Ubuntu 14.04 x64 OS.
status-firefox54: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.