Closed Bug 1190558 Opened 5 years ago Closed 5 years ago

MP3 playback broken after seeking

Categories

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

41 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox41 --- verified
firefox42 --- verified

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(2 files)

This is an intermittent bug I have noticed on Android when testing with VBR MP3 files (e.g., http://me73.com/audio).

After seeking, playback would pause, seek back to the beginning, and remain paused disregarding further play attempts.
The reason for this seems to be that MP3TrackDemuxer::Reset is called before MP3TrackDemuxer::Seek, however Reset does not reset the frame parser (for efficiency reasons) which then gets stuck in a bad state.

The proposed patch resets the frame parser on Reset which introduces a minor overhead of re-parsing of/searching for the ID3 header.
Attachment #8642602 - Flags: review?(kinetik)
Status: NEW → ASSIGNED
Basic seeking tests, would most likely not catch the intermittent failure, but it's a start.
Attachment #8642603 - Flags: review?(kinetik)
Note on the patch from comment 1: I've also renamed FinishParsing to EndFrameSession, since the former was too ambiguous.
Attachment #8642602 - Flags: review?(kinetik) → review+
Attachment #8642603 - Flags: review?(kinetik) → review+
Try looks good (Linux builds are currently busted): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4a096668742
https://hg.mozilla.org/mozilla-central/rev/69f15a2039fd
https://hg.mozilla.org/mozilla-central/rev/d31e43ead2dc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1163667
Comment on attachment 8642602 [details] [diff] [review]
0001-Bug-1190558-Reset-MP3-frame-parser-on-track-demuxer-.patch

Approval Request Comment
[Feature/regressing bug #]: 1093815
[User impact if declined]: MP3 playback may stop and break (non-resumable) after seeking
[Describe test coverage new/current, TreeHerder]: has been on Nightly for over a week and this bug extends test coverage for this issue
[Risks and why]: low, minor correctness fix that currently only affects MP3 playback on Android
[String/UUID change made/needed]: none
Attachment #8642602 - Flags: approval-mozilla-beta?
Attachment #8642603 - Flags: approval-mozilla-beta?
Eugen, I was trying out your STR on my win8 laptop and noticed that the seek often pauses still. I do not see it seek back to the beginning of the audio clip but it does stay paused. I am also noticing a second or two delay when I seek to a new position and when the audio clip plays. Is that normal?
Flags: needinfo?(esawin)
(In reply to Ritu Kothari (:ritu) from comment #8)
> Eugen, I was trying out your STR on my win8 laptop and noticed that the seek
> often pauses still. I do not see it seek back to the beginning of the audio
> clip but it does stay paused. I am also noticing a second or two delay when
> I seek to a new position and when the audio clip plays. Is that normal?

This doesn't sound normal, however, if you don't have the pref media.mp3.enabled set to true, then the new MP3 demuxer isn't used on desktop, which means it's an unrelated issue.

By default, the new demuxer is only enabled on Android (Jelly Bean and later).

If it is reproducible, we should file a bug on it though.
Flags: needinfo?(esawin)
Comment on attachment 8642602 [details] [diff] [review]
0001-Bug-1190558-Reset-MP3-frame-parser-on-track-demuxer-.patch

Both the patches baked in Nightly for about 2 weeks. Same safe to uplift to Beta.
Attachment #8642602 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8642603 [details] [diff] [review]
0002-Bug-1190558-Add-MP3-track-demuxer-seek-tests.patch

Beta+
Attachment #8642603 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I was able to reproduce this issue on Firefox 41 Beta 2 on http://me73.com/audio
Verified as fixed on Firefox 41 Beta 4 and on latest Auroraon http://me73.com/audio
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.