If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 46

Status

()

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

People

(Reporter: Loic, Assigned: JanH)

Tracking

({regression})

44 Branch
mozilla48
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46 fixed, firefox47+ fixed, firefox48+ fixed, firefox-esr45- wontfix)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

2 years ago
Blocks: 1202286
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
Flags: needinfo?(jh+bugzilla)
Keywords: regression
(Reporter)

Comment 1

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

Comment 2

2 years ago
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 :-)
Assignee: nobody → jh+bugzilla
Flags: needinfo?(jh+bugzilla)
See Also: → bug 1236639
(Assignee)

Comment 3

2 years ago
Created attachment 8739699 [details]
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/
Attachment #8739699 - Flags: review?(esawin)
(Reporter)

Comment 4

2 years ago
Let's track for ESR 45 too, because playback of these MP3 files worked before 45.
status-firefox-esr45: --- → affected
tracking-firefox-esr45: --- → ?
(Assignee)

Comment 5

2 years ago
If we want this uplifted all the way to 45, then the underlying crash fix needs uplifting as well.
Depends on: 1236639
See Also: bug 1236639
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.
Attachment #8739699 - Flags: review?(esawin) → review+
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d60ffe36cf22
Keywords: checkin-needed
(Assignee)

Comment 8

2 years ago
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
Attachment #8739699 - Flags: approval-mozilla-beta?
Attachment #8739699 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4c7118ebb9
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd4c7118ebb9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Attachment #8739699 - Flags: approval-mozilla-beta?
Attachment #8739699 - Flags: approval-mozilla-beta+
Attachment #8739699 - Flags: approval-mozilla-aurora?
Attachment #8739699 - Flags: approval-mozilla-aurora+

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/19f7428b8d98
status-firefox47: affected → fixed

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/01b90cb024b6
status-firefox46: affected → fixed
Not sure it is a big deal, not taking the fix in esr45
status-firefox45: affected → wontfix
status-firefox-esr45: affected → wontfix
tracking-firefox47: ? → +
tracking-firefox48: ? → +
tracking-firefox-esr45: ? → -
(Reporter)

Updated

a year ago
Depends on: 1291543
You need to log in before you can comment on or make changes to this bug.