|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
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 :-)
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/
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.
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 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 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.
Not sure it is a big deal, not taking the fix in esr45