949036 - Firefox says mp3 is corrupt, but it downloads and plays just fine

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Josh Matthews [:jdm]]

I get the "Video can't be played because it is corrupt" box when I visit the URL, but I downloaded it via Firefox and played it in Rhythmbox and it worked just fine.

Comment 1

[cjerrells]

I have this problem too with some MP3 files on my site. I have a large number of MP3 files, all created in the same way using the same version of LAME. Some play back fine in Firefox (embedded with an <audio> tag or opening the URL directly in a tab), while others fail as described above. I can't see any difference in the filesize, ID3 tag, or encoding settings between the files which work and those which don't.

The problem files play back fine in other web browsers and media player software (including VLC, Quicktime Player and Quicktime Player 7) and they also work fine if I download the file and then drag it into a Firefox tab. But if I try to play directly from the remote URL in Firefox, I get the error "Video cannot be played because the file is corrupt" and the console log shows "Media resource <URL> could not be decoded".

What is particularly strange is that this problem is intermittent. If I force-refresh the URL several times, the error occurs about half the time, and otherwise the file plays back fine.

I have tested with Firefox 36 and 37.0.2 on Mac OS X. I have tried Safe Mode and resetting Firefox.

Here is an example of a file which works:

http://easyeartraining.com/example_works.mp3

and one which doesn't:

http://easyeartraining.com/example.mp3

This seems to be a combination of encoding (since it affects some files and not others) and network connection (since it doesn't happen every time)

Any help would be very much appreciated!

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Interesting, I can confirm this also on Windows 7. I suppose whatever decoder Gecko uses is platform agnostic. I wonder if this is also the reason some MP3s refuse to play or even show up on my ZTE Open (which isn't really supported, so I never bothered filing).

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Ah, I see what you mean about how it does play if you drag it onto Firefox. So perhaps there's a race condition with accessing the file metadata, or something?

Comment 4

[cjerrells]

Thank you for confirming you can replicate this, Emanuel. 

I wasn't sure how to debug the network activity beyond just looking in Web Developer / Network panel, and that didn't reveal anything happening differently when this file works vs doesn't.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

I loaded this up in a local debug build (based on m-c revision 37d60e3b8be6). It logged the following to the terminal when I tried to load the 'broken' file a second time:

[8796] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file dom/media/directshow/DirectShowReader.cpp, line 121
[8796] WARNING: Decoder=8372a18 ReadMetadata failed, rv=80004005 HasValidMedia=0: file dom/media/MediaDecoderReader.cpp, line 232
[8796] WARNING: Decoder=8372a10 Decode metadata failed, shutting down decoder: file dom/media/MediaDecoderStateMachine.cpp, line 2214
[8796] WARNING: Decoder=8372a10 Decode error, changed state to ERROR: file dom/media/MediaDecoderStateMachine.cpp, line 2149
[8796] WARNING: NS_ENSURE_TRUE(aURI) failed: file netwerk/dns/nsEffectiveTLDService.cpp, line 158
[8796] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file dom/base/ThirdPartyUtil.cpp, line 355
[8796] WARNING: Appending an extra chunk for SourceBuffer: file image/src/SourceBuffer.cpp, line 69
[8796] WARNING: Appending an extra chunk for SourceBuffer: file image/src/SourceBuffer.cpp, line 69

Oddly it only logged the last two lines the first time I tried to load it. So there's definitely some sort of problem with reading the metadata.

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

I confirmed that the |aParser->IsMP3()| check [1] is failing. It also fails if I raise MAX_READ_SIZE to 16kiB, so that isn't the issue (not knowing anything about MP3, I figured it was worth a shot). I assume the same thing is happening on other platforms, since they all share MP3FrameParser ([2] and [3]). I don't know why it isn't recognizing the header though, especially since the file plays if loaded locally.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/directshow/DirectShowReader.cpp#95
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/MP3FrameParser.h
[3] https://dxr.mozilla.org/mozilla-central/source/dom/media/MP3FrameParser.cpp

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Hmm, it seems that by the time we get to ParseMP3Headers(), we've already decided whether or not the file is an mp3 - we never even enter the loop. This is because DirectShowReader::NotifyDataArrived() is called from MediaDecoderStateMachine::NotifyDataArrived() with the following stack:

#01: mozilla::MediaDecoderStateMachine::NotifyDataArrived (dom/media/mediadecoderstatemachine.cpp:1681)
#02: mozilla::MediaDecoder::NotifyDataArrived (dom/media/mediadecoder.cpp:1655)
#03: mozilla::ChannelMediaResource::CopySegmentToCache (dom/media/mediaresource.cpp:508)
#04: mozilla::net::CacheFileInputStream::ReadSegments (netwerk/cache2/cachefileinputstream.cpp:167)
#05: mozilla::ChannelMediaResource::OnDataAvailable (dom/media/mediaresource.cpp:540)
#06: mozilla::ChannelMediaResource::Listener::OnDataAvailable (dom/media/mediaresource.cpp:142)
#07: mozilla::dom::MediaDocumentStreamListener::OnDataAvailable (dom/html/mediadocument.cpp:95)
#08: nsDocumentOpenInfo::OnDataAvailable (uriloader/base/nsuriloader.cpp:308)
#09: mozilla::net::nsHttpChannel::OnDataAvailable (netwerk/protocol/http/nshttpchannel.cpp:5780)
#10: nsInputStreamPump::OnStateTransfer (netwerk/base/nsinputstreampump.cpp:610)
#11: nsInputStreamPump::OnInputStreamReady (netwerk/base/nsinputstreampump.cpp:437)
#12: nsInputStreamReadyEvent::Run (xpcom/io/nsstreamutils.cpp:93)
#13: nsThread::ProcessNextEvent (xpcom/threads/nsthread.cpp:866)
#14: NS_ProcessNextEvent (xpcom/glue/nsthreadutils.cpp:265)
#15: mozilla::ipc::MessagePump::Run (ipc/glue/messagepump.cpp:99)

Saying that, it just calls MP3FrameParser::Parse() on a buffer, which is what ParseMP3Headers() does too. So I'm not sure why it would change the state of MP3FrameParser::IsMP3() to something *invalid* - it might still be 'maybe' (which it starts off as), but 'no' should be conclusive. I haven't really looked into the Parse() logic though.

The one thing I can say is that DirectShowReader::NotifyDataArrived() should probably check MP3FrameParser::IsMP3() again *after* calling MP3FrameParser::Parse(), since the answer might have changed to 'no'. But that certainly wouldn't make ParseMP3Headers() fail any less.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Going through the NotifyDataArrived() path, which parses the whole 66.7kiB file at once, MP3FrameParser::ParseBuffer() finds and skips over an ID3v2 header with length *102110438*. Since this skips way past the end of the file, it then doesn't find any actual MP3 frames.

Loading locally goes through the ParseMP3Headers() path, which parses in 4kiB chunks. For some reason this *does not* see this nonsense header, and so it finds the frames just fine.

Comment 9

[cjerrells]

Thanks for investigating this, Emanuel. It sounds like you've pinned down the cause!

I'm surprised an ID3 tag is involved, as I did try stripping all ID3 tags (using eyeD3) on the example problem file and had the same issue:

http://easyeartraining.com/example_notags.mp3


$ cp example.mp3 example_notags.mp3
$ eyeD3 --remove-all example_notags.mp3 

example_notags.mp3	[ 66.76 KB ]
-------------------------------------------------------------------------------
Time: 00:02	MPEG1, Layer III	[ ~187 kb/s @ 44100 Hz - Joint stereo ]
-------------------------------------------------------------------------------
Removing ID3 v1.x and/or v2.x tag: SUCCESS

$ eyeD3 example_notags.mp3 

example_notags.mp3	[ 65.63 KB ]
-------------------------------------------------------------------------------
Time: 00:02	MPEG1, Layer III	[ ~187 kb/s @ 44100 Hz - Joint stereo ]
-------------------------------------------------------------------------------
No ID3 v1.x/v2.x tag found!

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

Well, I think the supposed ID3 tag it finds isn't an ID3 tag at all, just some random data that happens to look like an ID3 tag. I haven't looked at the actual bytes to confirm this, but ID3Parser::ParseChar() doesn't try very hard to detect invalid tags. I'm still not sure how ParseMP3Headers() avoids the problem though!

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Make ID3v2 tag detection more careful - not every bit of data containing "ID3" is a tag.

This appears to fix the bug for me. Edwin, you wrote this code, does this make sense to you?

In addition to making the ID3v2 parsing more careful, I also changed the length calculation: from my reading of the spec [1], the size stored in the header does *not* include the size of the header itself, or the optional footer. Since we use it as though it does, I changed mHeaderLength to include those bytes. Technically it probably hurts more to *overshoot* than to *undershoot* though, so I'd like confirmation that this is correct if possible.

[1] http://id3.org/id3v2.4.0-structure

Comment 12

[Emanuel Hoogeveen [:ehoogeveen]]

Hmm, it doesn't fix the mp3 file in the URL field though. But let's take things one issue at a time.

Comment 13

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

Comment on attachment 8597740 [details] [diff] [review]
Make ID3v2 tag detection more careful - not every bit of data containing "ID3" is a tag.

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

Looks good to me.

Comment 14

[Emanuel Hoogeveen [:ehoogeveen]]

Trying to decode the mp3 from the URL fails in two places:

[6256] WARNING: CoCreateInstance failed, hr=80040154: file dom/media/directshow/DirectShowUtils.cpp, line 198
[6256] WARNING: NS_ENSURE_TRUE(SUCCEEDED(hr)) failed: file dom/media/directshow/DirectShowReader.cpp, line 186

The first one is part of a call that is expected to fail on versions of Windows after Windows XP, but it's odd that CoCreateInstance is failing of all things (it doesn't fail with the other mp3s linked in this bug).

The second failure is the first ConnectFilters() call, which simply connects the demuxer to the source filter. I have no idea why this would fail when all the previous calls succeeded. It might be interesting to see if things fail the same way on other OSes.

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v2 - Make ID3v2 tag detection more careful - not every bit of data containing "ID3" is a tag.

Updated the other instances of NotifyDataArrived to make them all consistent with each other. I also made ID3Parser::ParseChar() retry on failure so that it doesn't skip a character (but only on a partial match, so it won't infinitely recurse).

Carrying forward r=eflores since the changes are trivial. Running it through try now.

Comment 16

[Emanuel Hoogeveen [:ehoogeveen]]

Looks okay:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70c4d6549bdf

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5936402a349d

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5936402a349d

Comment 19

[Emanuel Hoogeveen [:ehoogeveen]]

Josh, I was going to open another bug on the mp3 that you filed this one for, but I downloaded and tried to play it in foobar2000 (on Windows 7), and it agrees with Firefox that the file is corrupt. I haven't looked at the file itself more closely (perhaps the extension is wrong?), but perhaps there's no bug here?

Comment 20

[Josh Matthews [:jdm]]

It plays fine in VLC and QuickTime on OS X.

Comment 21

[Emanuel Hoogeveen [:ehoogeveen]]

Aha, VLC identifies the codec as "PCM S16 LE (s16l)". This is just an uncompressed WAVE file - changing the extension to .wav allows foobar2000 to play it as well. Firefox doesn't support WAVE, so it just offers to download it or open it with an external program.

I guess we could handle this better or at least more clearly - I'm not sure why things fall over quite the way they do - but this is not an MP3 decoding problem.

Comment 22

[cjerrells]

Thanks for the swift work on this bug, I'm very impressed!

I see that the fix has been assigned to mozilla40. If I'm reading https://wiki.mozilla.org/RapidRelease/Calendar correctly, does that mean it won't be publicly available until 2015-08-11?

Comment 23

[Emanuel Hoogeveen [:ehoogeveen]]

Firefox 40 won't be officially released until then, but you can try out tomorrow's Nightly [1] to see the fix. It will then follow the usual route to Aurora/Dev Edition, Beta and finally Release.

We could also try for uplift to 39, but I think we're too late into beta for an uplift to 38.

[1] https://nightly.mozilla.org/

Comment 24

[cjerrells]

Thank you, that's helpful to understand. 

I'll see if my members who are encountering this issue can switch to the Nightly build. And if not, understanding that it's a spurious "ID3" in the file might be enough to let me identify and re-encode the problem tracks.

Thanks!

Comment 25

[Catalin Varga [QA][:VarCat]]

I verified this bug using the following environment:

FF 40
Build Id:20150429030202
OS: Win 7 x64

This fix however does not resolves the issue related with the original URL eg: http://www.theyoungnovelists.com/SilentNight.mp3 .
So the verification was done using the example from comment 1. Due to the fact that the issue described in the original STR is not fixed I cloned this bug. The clone of the bug is 1159745 .

Comment 26

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8598013 [details] [diff] [review]
v2 - Make ID3v2 tag detection more careful - not every bit of data containing "ID3" is a tag.

Let's try for a late approval uplift.

Approval Request Comment
[Feature/regressing bug #]: Possibly bug 919572 (Firefox 26), but might date back further. Not a recent regression.
[User impact if declined]: Some MP3s will not play, apparently common with MP3s produced using the popular LAME encoder.
[Describe test coverage new/current, TreeHerder]: Didn't break any existing media tests, has been on Nightly for several days.
[Risks and why]: Low risk: Only affects MP3 detection / duration estimation. We do not use this logic to actually *decode* the MP3, and it shouldn't impact other codecs. Bugs in the code could cause us to fail to detect ID3v2 tags, which could raise chances of finding bogus 'MP3 frame' data, impacting duration estimation.
[String/UUID change made/needed]: none

Comment 27

[Emanuel Hoogeveen [:ehoogeveen]]

Er, a late Aurora uplift even.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8598013 [details] [diff] [review]
v2 - Make ID3v2 tag detection more careful - not every bit of data containing "ID3" is a tag.

Approving for uplift to aurora since this has had some time on Nightly, has been verified, and appears to be low risk.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Emanuel Hoogeveen [:ehoogeveen]]

@cjerrells: this is now on Aurora/Dev Edition, and should make it to Firefox 39. The next Extended Service Release (ESR) will be based on 38 and won't have it I'm afraid (the policy for uplifts to ESR is pretty strict).

Comment 31

[cjerrells]

Emanuel: That's great news. Thank you again!