a user in the support forum reported* a regression in firefox 26 that words are no longer pronounced when you click on the speaker icon at http://tw.dictionary.search.yahoo.com/search;_ylt=A3eg.nXHREBSxRUAzBh7rolQ?p=test&fr=sfp i can replicate this on windows 7, a embedded MP3 file should be played once you click on the speaker icon, but there's no sound and a warning in the web console: Media resource http://l.yimg.com/tn/dict/kh/v1/97922.mp3 could not be decoded. this is the regression range: Last good nightly: 2013-09-09 First bad nightly: 2013-09-10 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7cc85e13f7a&tochange=e5ca10a2b3d0 so probably regressed by bug 910996 *https://support.mozilla.org/en-US/questions/972156

attachment of the mp3-file unable to play-back

The problem here is that in the first MP3 frame in this file is not being detected as MP3 by our MP3FrameParser. Not sure why. The first MP3 frame is detected as an MP3 frame by the frame parser code I found on hydrogenaudio, so I suggest that we just use that code instead of our existing MP3FrameParser, since it's much simpler code and much smaller. Edwin volunteered to do this.

Code to extract frames from MP3 file using the parser I found on hydrogenaudio, and log the frame sizes to disk. I emailed the original author of the code, Nick Wallette, and he consented to let us use this code in Firefox. We should also add the test to our test suite, it's small.

Switched the MP3 frame parser to the one from hydrogenaudio; seems to fix this particular bug. Also rewrote the ID3 header parser to one that didn't need any buffered data: this eliminated the need for most of the crazy hairy buffering stuff in MP3FrameParser. It's probable that without the buffering code we'll miss an MP3 frame header every now and again, but it would take a rather unlucky coincidence for that to make a difference -- maybe a slight performance hit from having to search for the next header.

Comment on attachment 809654 [details] [diff] [review] 919572.patch Review of attachment 809654 [details] [diff] [review]: ----------------------------------------------------------------- Patch doesn't apply cleanly to trunk, can you rebase/fix it? ::: content/media/MP3FrameParser.h @@ +75,5 @@ > // no MP3 frame has been detected yet. > int64_t GetMP3Offset(); > > // The location in the stream of the last byte we looked at. > uint64_t GetLastStreamOffset() { This function doesn't exist in mozilla-central. Did you have another patch in your queue before this one?

Comment on attachment 810250 [details] [diff] [review] 919572.patch Review of attachment 810250 [details] [diff] [review]: ----------------------------------------------------------------- r- because I want to go to lunch. Put the mBuffer stuff back. I know its not nice, but we need it to ensure deterministic behaviour.

MP3 header parser now buffers its dumb self because it's better that way. All is now right with the world.

Forgot to set mMP3Offset.

Comment on attachment 810869 [details] [diff] [review] 919572.patch Review of attachment 810869 [details] [diff] [review]: ----------------------------------------------------------------- This file fails to play with the DirectShow backend with your patch applied, but does play without: http://79.142.89.165/nagrani/20130910_-_55_Krutushka-2013.mp3 There may be more MP3 files here that don't play here: http://folkradio.ru/nagrani/ You're getting an MP3 data offset of 21, which may be wrong. You also need to handle multiple ID3 headers at the start if the file, some MP3 files have multiple ID3 headers, and you're returning the MP3 offset as being after the first ID3 header is found, which is wrong in the case of multiple ID3 headers. We need to strip off ID3 headers for the directshow backend, as it can't handle some ID3 tags.

(In reply to Chris Pearce (:cpearce) from comment #10) > This file fails to play with the DirectShow backend with your patch applied, > but does play without: > > http://79.142.89.165/nagrani/20130910_-_55_Krutushka-2013.mp3 [...] > You also need to handle multiple ID3 headers at the start if the file, some > MP3 files have multiple ID3 headers, and you're returning the MP3 offset as > being after the first ID3 header is found, which is wrong in the case of > multiple ID3 headers. We need to strip off ID3 headers for the directshow > backend, as it can't handle some ID3 tags. WFM on OSX. Maybe it's not so sensitive about ID3 tags as DirectShow is. Thought the ID3 header was boxed into one big header; I guess not. Will put up a new patch with that fixed.

Blargh, bleh, and grargh. Fixed that. Trillionth time lucky.

Comment on attachment 810951 [details] [diff] [review] 919572.patch v100000000 Review of attachment 810951 [details] [diff] [review]: ----------------------------------------------------------------- Good! Works even.

This added two unused variables which broke my builds since they added two compiler warnings, so I took them out: https://hg.mozilla.org/integration/mozilla-inbound/rev/cea4196cd3b2

https://hg.mozilla.org/mozilla-central/rev/89512636e032 https://hg.mozilla.org/mozilla-central/rev/cea4196cd3b2

thanks for the quick fix!

This bug causes regression of Bug 912829 and Bug 922334.

(In reply to Sotaro Ikeda [:sotaro] from comment #18) > This bug causes regression of Bug 912829 and Bug 922334. This bug causes also youtube playback failure but Bug 912829 seems different one. sorry.

Looks like we need an uplift nomination here.

Comment on attachment 810951 [details] [diff] [review] 919572.patch v100000000 Review of attachment 810951 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MP3FrameParser.cpp @@ +13,5 @@ > namespace mozilla { > > +/* > + * Following code taken from http://www.hydrogenaudio.org/forums/index.php?showtopic=85125 > + * with permission from the author, Nick Wallette <sirnickity@gmail.com>. I'd like gerv to sign off on this.

(In reply to :Ms2ger from comment #21) > Comment on attachment 810951 [details] [diff] [review] > > + * Following code taken from http://www.hydrogenaudio.org/forums/index.php?showtopic=85125 > > + * with permission from the author, Nick Wallette <sirnickity@gmail.com>. > > I'd like gerv to sign off on this. Can whoever wrote this code paste in here the contents of the communication they got from Nick which gives permission? Gerv

Here's the mail thread I had with Nick: On 18-Sep-13 9:12 AM, Nick Wallette wrote: > Chris, > Absolutely, help yourself. :-) You can use my real name: Nick Wallette > FYI, I wrote a test player using libmpg123 and ALSA, if seeing the code in context might be of some benefit. I didn't publish that part because I ran into a snag with libmpg123's feed mechanism -- the output audio has periodic glitches that I suspect is caused by waiting for the NEED_MORE signal before feeding the next frame. From what I know about MP3 inter-frame dependency, if you underflow the playback buffer, the subsequent frame won't be decoded correctly. Nevertheless, it's my interpretation of the lib docs that polling the buffer is a valid approach. Not quite sure if that's a bug in the lib or an error on my part, but since it was a test and I was able to dump the file to disk with 100% integrity, I didn't pursue the decoding issue any further. Anyway, let me know if the rest of the code would be useful to you and I'll dig it up -- or if I can help any other way. > Thanks for the note! Take care, > -Nick. > > > On Tue, Sep 17, 2013 at 2:36 AM, Hydrogenaudio Forums <chris@pearce.org.nz> wrote: > > > SirNickity, > > cpearce has sent you this email from http://www.hydrogenaudio.org/forums/index.php. > > > Hi SirNickity, > > I work on Firefox's media player code that powers HTML <audio> and video playback. > > I found your code for determining the size in bytes of MP3 frames here: > http://www.hydrogenaudio.org/forums/index.php?showtopic=85125 > > I was wondering if you would consent to us using your MP3 frame parser code in that post in Firefox? > > We need an MP3 frame parser to work around various bugs in the MP3 decoders provided by the platforms that we run on, and our existing parser doesn't work correctly and is about four times the size of yours. Would it be OK for us to use your MP3 frame parser in Firefox? If so, what name would you like to be credited as? > > Regards, > Chris Pearce

(In reply to lsblakk@mozilla.com [:lsblakk] from comment #20) > Looks like we need an uplift nomination here. Not sure if we should, given b2g breakage, plus I'm away until November. Chris?

Hi cpearce: sorry to be a stickler, but can you specifically ask Nick if it's OK to license his code under the MPL 2.0 (give him a link) and check that he wrote it all from scratch? Thanks :-) Gerv

I think we should uplift this so that we fix the incorrect MP3 duration calculation in the DirectShow MP3 backend, otherwise we'll be shipping a regression in Firefox Desktop. This caused some additional regressions on B2G however, so we'll need to request uplift on addition patches. Sotaro: * Are all the additional regressions on B2G caused by the MP3FrameParser changes fixed on mozilla-central? * Is it safe to uplift the patch in this bug, along with the other fixes to MP3FrameParser to Aurora?

(In reply to Gervase Markham [:gerv] from comment #25) > Hi cpearce: sorry to be a stickler, but can you specifically ask Nick if > it's OK to license his code under the MPL 2.0 (give him a link) and check > that he wrote it all from scratch? I will follow up here.

(In reply to Chris Pearce (:cpearce) from comment #26) > Sotaro: > * Are all the additional regressions on B2G caused by the MP3FrameParser > changes fixed on mozilla-central? I confirmed that this fixes Bug 923451. other regressions were fixed by other fixes. - Bug 923451 - [music] duration of mp3 is not correct Bug 922953 is recognized as an unfixed bug. - Bug 922953 - [inari][music] Long mp3s "skip" like a record when first starting. > * Is it safe to uplift the patch in this bug, along with the other fixes to > MP3FrameParser to Aurora? I tried to apply the patch to b2gv1.2. Applying the patch out put a lot of 'Hunk". But it seems to work correctly. It might be better to create a patch for Aurora.

koi? - see the blocking bug in bug 923451 for reasoning.

Patch for b2g v1.2. It is created from 919572.patch v100000000 to prevent lot of hunk on b2g v1.2.

(In reply to Sotaro Ikeda [:sotaro] from comment #30) > Created attachment 820584 [details] [diff] [review] > 919572.patch v100000000 for b2g v1.2 > > Patch for b2g v1.2. It is created from 919572.patch v100000000 to prevent > lot of hunk on b2g v1.2. Confirmed the patch works on v1.2 hamachi.

Blocking koi+ 923451.

(In reply to Gervase Markham [:gerv] from comment #25) > Hi cpearce: sorry to be a stickler, but can you specifically ask Nick if > it's OK to license his code under the MPL 2.0 (give him a link) and check > that he wrote it all from scratch? > > Thanks :-) > > Gerv Not uplifting to aurora (/b2g1.2) until this is resolved.

Nick didn't respond to my email asking Gerv's questions. I have emailed again to try get this resolved. Failing that, Edwin can rewrite the code when he gets back on October 4.

OK, Nick replied, and confirmed: "The code is originally written by me, and yes it may be licensed under the MPL." See attached eml showing the thread I had with Nick where he confirmed this.

(In reply to Chris Pearce (:cpearce) from comment #34) > Nick didn't respond to my email asking Gerv's questions. I have emailed > again to try get this resolved. Failing that, Edwin can rewrite the code > when he gets back on October 4. November 4 I meant of course. Edwin can look at the remaining duration issues when we gets back then.

Comment on attachment 810951 [details] [diff] [review] 919572.patch v100000000 r=gerv. Gerv

Verified as fixed on latest Nightly 27.0a1 and Firefox 25 beta 11 (buildID: 20131025150754). In latest Aurora 26.0a2 the sound from TC under comment 0 does not work: http://tw.dictionary.search.yahoo.com/search;_ylt=A3eg.nXHREBSxRUAzBh7rolQ?p=test&fr=sfp - No sound can be heard. http://l.yimg.com/tn/dict/kh/v1/97922.mp3 - Video can't be played because the file is corrupt and no sound either. From comment 10 all sounds are played on Aurora. Any thoughts?

Bogdan, now that we have 26b1, can you please verify this is fixed? If you still see the same behaviour as in comment 39 I think we'll need to either reopen of file a new bug. Chris, can you please provide feedback.

it works for me in today's aurora build (26.0a2).

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #39) > Verified as fixed on latest Nightly 27.0a1 and Firefox 25 beta 11 (buildID: > 20131025150754). In latest Aurora 26.0a2 the sound from TC under comment 0 > does not work: > http://tw.dictionary.search.yahoo.com/search;_ylt=A3eg. > nXHREBSxRUAzBh7rolQ?p=test&fr=sfp - No sound can be heard. > http://l.yimg.com/tn/dict/kh/v1/97922.mp3 - Video can't be played because > the file is corrupt and no sound either. > From comment 10 all sounds are played on Aurora. Any thoughts? WFM in today's aurora, Mozilla/5.0 (Windows NT 6.3; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0.

Verified as fixed in Aurora 27.0a2 and Firefox 26 beta 1.

Has someone tested this on Aurora 28.a2? It isn't working anymore for me

WFM (link given in the 1st comment and MP3 file). Test with a new profile, please.

I think this might still be causing trouble (at least I think this could be the reason for https://bugzilla.mozilla.org/show_bug.cgi?id=977089 ) Can somebody confirm the behaviour mentioned there?

Thanks Martin, We'll investigate on bug 977089.

Hello, This still happens with Nightly 32.0a1 2014-05-15. I checked this in a new profile with the test file from the 1st comment. Please reopen.

Bogdan, can you please confirm comment 48?

I tested using two different machines with Windows 7 64bit on Nightly (2014-05-15) and latest Nightly (2014-05-18). The mp3 plays normally so I can`t confirm comment 48, works for me.

Thanks Bogdan. Sylvain, if you can still reproduce this bug in the latest Firefox Nightly then please file a new bug report so we can investigate it. Please refer back to this bug report so we can create a relation between the reports. Also, please include information specific to your system (ex. audio card, drivers, etc).

I can confirm this bug reappeared with firefox-nightly 37.0a1 2015-01-07

The STR from comment 1 works for me in 2015-01-07 and 2015-01-08 on Win8.1. Yuxuan: are you testing with the Steps To Reproduce/URL from comment 1? Are you on Windows 7?

(In reply to Chris Pearce (:cpearce) from comment #53) > The STR from comment 1 works for me in 2015-01-07 and 2015-01-08 on Win8.1. > > Yuxuan: are you testing with the Steps To Reproduce/URL from comment 1? Are > you on Windows 7? Yes, the mp3 file in comment 1 doesn't play on 37.0a1 2015-01-07, I'm using Arch Linux.

(In reply to Chris Pearce (:cpearce) from comment #53) > The STR from comment 1 works for me in 2015-01-07 and 2015-01-08 on Win8.1. > > Yuxuan: are you testing with the Steps To Reproduce/URL from comment 1? Are > you on Windows 7? Oh, I didn't realize this bug report is for Windows 7. Should I open a new one for Linux?

This might be related: https://bugzilla.mozilla.org/show_bug.cgi?id=977089

I have same error, pls fix it. I have latetest version of firefox and flash player to day 2015-08-11 Media resource http://aredir.nixcdn.com/adb47548a3d7e18eebc5999ca069f954/55c94ab2/NhacCuaTui840/BanTinhCuoi-LeQuyen-2761541.mp3 could not be decoded.

(In reply to hoangchau from comment #57) > I have same error, pls fix it. > I have latetest version of firefox and flash player to day 2015-08-11 > Media resource > http://aredir.nixcdn.com/adb47548a3d7e18eebc5999ca069f954/55c94ab2/ > NhacCuaTui840/BanTinhCuoi-LeQuyen-2761541.mp3 could not be decoded. Sorry but FF39 is able to play this MP3 natively without Flash. If you have an issue with it, file a new bug report because this one is closed.