1263334 - When playing MP3, seekbar goes to the end and total duration is not displayed

Status: RESOLVED FIXED

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

Description

[Loic]

Issue found when testing bug 1263241.

STR (use FF46+ to avoid crashing):
Play https://don1uexbbetbo.cloudfront.net/content/mp3s/uncertain_elections_2016.mp3

Result: when starting playback, seekbar goes to the end and the total duration of the MP3 is not displayed.

Regressed by bug Bug 1202286:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=89732fcdb0baca70e8b7a25a2725117113f0db80&tochange=2722b65059df232c43dfaec075dfe484e0db952c

Comment 1

[Loic]

5m47s should be displayed:

General
Complete name                            : uncertain_elections_2016.mp3
Format                                   : MPEG Audio
File size                                : 2.64 MiB
Duration                                 : 5mn 46s
Overall bit rate mode                    : Constant
Overall bit rate                         : 64.0 Kbps
Writing library                          : LAME3.98 

Audio
Format                                   : MPEG Audio
Format version                           : Version 2
Format profile                           : Layer 3
Duration                                 : 5mn 46s
Bit rate mode                            : Constant
Bit rate                                 : 64.0 Kbps
Channel(s)                               : 1 channel
Sampling rate                            : 24.0 KHz
Compression mode                         : Lossy
Stream size                              : 2.64 MiB (100%)
Writing library                          : LAME3.98 
Encoding settings                        : -m m -V 4 -q 5 -lowpass 12 -b 64

Comment 2

[Jan Henning [:JanH]]

It's another file which claims to have zero frames in the XING/INFO header. MP3TrackDemuxer::Duration() is missing a check that numAudioFrames is not only something, but actually > 0 (or alternatively that the VBR header is valid) before using that value for any duration calculations.

The relation to bug 1202286 seems to be that with part 2 we started parsing INFO headers as well, whereas previously we simply ignored them. This shows that no good deed goes unpunished :-)

Comment 3

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1263334 - Check VBR header is valid before using it for duration calculations. r=esawin

Review commit: https://reviewboard.mozilla.org/r/45345/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45345/

Comment 4

[Loic]

Let's track for ESR 45 too, because playback of these MP3 files worked before 45.

Comment 5

[Jan Henning [:JanH]]

If we want this uplifted all the way to 45, then the underlying crash fix needs uplifting as well.

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8739699 [details]
MozReview Request: Bug 1263334 - Check VBR header is valid before using it for duration calculations. r=esawin

https://reviewboard.mozilla.org/r/45345/#review41995

Looks good.

Comment 7

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d60ffe36cf22

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8739699 [details]
MozReview Request: Bug 1263334 - Check VBR header is valid before using it for duration calculations. r=esawin

Approval Request Comment
[Feature/regressing bug #]: MP3 playback with new MP3 parser
[User impact if declined]: Some MP3 files (those affected by bug 1236639) will display with an initial duration of 00:00, which makes seeking within the file more difficult.
[Describe test coverage new/current, TreeHerder]: existing media gtests
[Risks and why]: Minimal. For files with invalid VBR headers we simply fall back to the normal duration estimation for CBR files.
[String/UUID change made/needed]: none

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4c7118ebb9

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd4c7118ebb9

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8739699 [details]
MozReview Request: Bug 1263334 - Check VBR header is valid before using it for duration calculations. r=esawin

Fix for recent regression, we want seek to work reliably, let's fix this for beta 11.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/19f7428b8d98

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01b90cb024b6

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Not sure it is a big deal, not taking the fix in esr45