919572 - MP3 playback fails - Media resource could not be decoded.

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[[:philipp]]

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

Comment 1

[[:philipp]]

Attachment: test file

attachment of the mp3-file unable to play-back

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: mp3-frame-parser.cpp

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.

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 919572.patch

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 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 919572.patch

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 919572.patch

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

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 919572.patch

Forgot to set mMP3Offset.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 919572.patch v100000000

Blargh, bleh, and grargh.

Fixed that.

Trillionth time lucky.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 810951 [details] [diff] [review]
919572.patch v100000000

Review of attachment 810951 [details] [diff] [review]:
-----------------------------------------------------------------

Good! Works even.

Comment 14

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89512636e032

Comment 15

[(no longer active)]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[[:philipp]]

thanks for the quick fix!

Comment 18

[Sotaro Ikeda [:sotaro]]

This bug causes regression of Bug 912829 and Bug 922334.

Comment 19

[Sotaro Ikeda [:sotaro]]

(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.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like we need an uplift nomination here.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 <[e-mail]>.

I'd like gerv to sign off on this.

Comment 22

[Gervase Markham [:gerv]]

(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 <[e-mail]>.
> 
> 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

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 <[e-mail]> 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

Comment 24

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to [e-mail] [: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?

Comment 25

[Gervase Markham [:gerv]]

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

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 28

[Sotaro Ikeda [:sotaro]]

(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.

Comment 29

[Jason Smith [:jsmith]]

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

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 31

[Sotaro Ikeda [:sotaro]]

(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.

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Blocking koi+ 923451.

Comment 33

[Ed Morley [:emorley]]

(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.

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Email confirmation of licensing

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.

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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 37

[Gervase Markham [:gerv]]

Comment on attachment 810951 [details] [diff] [review]
919572.patch v100000000

r=gerv.

Gerv

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df153717f1ff

Comment 39

[Bogdan Maris, Desktop Test Engineering]

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?

Comment 40

[u279076]

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.

Comment 41

[[:philipp]]

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

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 43

[Bogdan Maris, Desktop Test Engineering]

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

Comment 44

[weirdkeen]

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

Comment 45

[Loic]

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

Comment 46

[martin]

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?

Comment 47

[u279076]

Thanks Martin,

We'll investigate on bug 977089.

Comment 48

[Sylvain Petreolle]

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.

Comment 49

[u279076]

Bogdan, can you please confirm comment 48?

Comment 50

[Bogdan Maris, Desktop Test Engineering]

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.

Comment 51

[u279076]

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).

Comment 52

[Yuxuan Shui]

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

Comment 53

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 54

[Yuxuan Shui]

(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.

Comment 55

[Yuxuan Shui]

(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?

Comment 56

[martin]

This might be related:

https://bugzilla.mozilla.org/show_bug.cgi?id=977089

Comment 57

[hoangchau]

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.

Comment 58

[Loic]

(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.