1190558 - MP3 playback broken after seeking

Status: VERIFIED FIXED

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

Description

[Eugen Sawin [:esawin]]

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.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1190558-Reset-MP3-frame-parser-on-track-demuxer-.patch

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.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1190558-Add-MP3-track-demuxer-seek-tests.patch

Basic seeking tests, would most likely not catch the intermittent failure, but it's a start.

Comment 3

[Eugen Sawin [:esawin]]

Note on the patch from comment 1: I've also renamed FinishParsing to EndFrameSession, since the former was too ambiguous.

Comment 4

[Eugen Sawin [:esawin]]

Try looks good (Linux builds are currently busted): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4a096668742

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f15a2039fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31e43ead2dc

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69f15a2039fd
https://hg.mozilla.org/mozilla-central/rev/d31e43ead2dc

Comment 7

[Eugen Sawin [:esawin]]

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

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 9

[Eugen Sawin [: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.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8642603 [details] [diff] [review]
0002-Bug-1190558-Add-MP3-track-demuxer-seek-tests.patch

Beta+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e29b0ab7e6d
https://hg.mozilla.org/releases/mozilla-beta/rev/e969f02272fb

Comment 13

[u421692]

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