Closed Bug 1263334 Opened 4 years ago Closed 4 years ago

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

Categories

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

44 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed
firefox47 + fixed
firefox48 + fixed
firefox-esr45 - wontfix

People

(Reporter: epinal99-bugzilla2, Assigned: JanH)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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
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
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: → 1236639
Let's track for ESR 45 too, because playback of these MP3 files worked before 45.
If we want this uplifted all the way to 45, then the underlying crash fix needs uplifting as well.
Depends on: 1236639
See Also: 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+
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/mozilla-central/rev/bd4c7118ebb9
Status: NEW → RESOLVED
Closed: 4 years ago
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+
Depends on: 1291543
You need to log in before you can comment on or make changes to this bug.