788204 - Gecko doesn't detect MIME type of MP3 files

Status: RESOLVED FIXED

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

Description

[[e-mail], [e-mail]]

STR
====

var player = new Audio();
player.src='style/ringtones/ALARM_progressive_dapple.mp3';

Actual result:
 - Can not load the .mp3 media.
 - Attached log: 
E/GeckoConsole( 1444): [JavaScript Warning: "HTTP "Content-Type" of "application/x-unknown-content-type" is not supported. Load of media resource app://clock.gaiamobile.org/style/ringtones/ALARM_progressive_dapple.mp3 failed." {file: "app://clock.gaiamobile.org/onring.html" line: 0}]

Expected result:
 - It should be work.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is odd ... if I go to http://people.mozilla.com/~cjones/media.html in the browser app and tap "MP3", I get playback.  Will see if I can make a testcase that doesn't work.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I added two new options to http://people.mozilla.com/~cjones/media.html to directly play mp3/ogg using an Audio element directly, and I still don't reproduce the bug.  Playback works just fine.

Ian, can you make a test case using the "Template" app that reproduces your bug?  Were you testing with a desktop build?

Comment 3

[Matthias Versen [:Matti]]

The issue is the wrong mime-type as the error message already indicates "application/x-unknown-content-type". a wrong content-type is intended to not work.

The testcase as http://people.mozilla.com/~cjones/media.html uses this URL:
http://pianoadventures.com/audio/mp3s/CD1002-21.mp3 and the server sends this with Content-Type:	audio/mpeg according to http://www.web-sniffer.net

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ah, this is gecko not detecting the mp3 MIME type properly.  I saw that we weren't doing this when working on a previous bug, and started to patch that, but tossed out the WIP since the "real bug" lay elsewhere.

Comment 5

[Doug Turner (:dougt)]

i am pretty sure there are other mimetypes that we should support but don't.

Comment 7

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

The patches in bug 567077 will hopefully sniff MP3 and thus fix this bug.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The way I had started cargo-culting this in is by
 - following the conditional recognition of mp4 when gstream support is enabled
 - add a check for media plugins
 - add mp3 to the list of recognized extensions

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Pearce (:cpearce) from comment #7)
> The patches in bug 567077 will hopefully sniff MP3 and thus fix this bug.

Note, this bug appears to be triggered when loading a .mp3 file off of disk: we don't detect audio/mpeg.  Doing the same thing with .ogg results in OGG being detected.  Will bug 567077 fix that?

Comment 10

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

Ah. We need to add audio/mpeg to nsMimeTypes.h and nsExternalHelperAppService.cpp, like we do for audio/ogg and friends.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK --- that's the code I was cargo-culting in comment 8.

dhylands, wan to grab this?

Comment 12

[[e-mail], [e-mail]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> I added two new options to http://people.mozilla.com/~cjones/media.html to
> directly play mp3/ogg using an Audio element directly, and I still don't
> reproduce the bug.  Playback works just fine.
> 
> Ian, can you make a test case using the "Template" app that reproduces your
> bug?  Were you testing with a desktop build?

Yes, It doesn't work with a desktop build.

Comment 13

[Dave Hylands [:dhylands]]

Attachment: Patch which adds mp3 file extension

Comment 14

[Dave Hylands [:dhylands]]

Looking at frameworks/base/media/libstagefright/MediaExtractor.cpp, it can deal with the following mime types:

MEDIA_MIMETYPE_CONTAINER_MPEG4
MEDIA_MIMETYPE_AUDIO_MPEG
MEDIA_MIMETYPE_AUDIO_AMR_NB          audio/3gpp
MEDIA_MIMETYPE_AUDIO_AMR_WB          audio/amr-wb
MEDIA_MIMETYPE_AUDIO_FLAC            audio/flac
MEDIA_MIMETYPE_CONTAINER_WAV         audio/wav
MEDIA_MIMETYPE_CONTAINER_OGG         application/ogg
MEDIA_MIMETYPE_CONTAINER_MATROSKA    video/x-matroska
MEDIA_MIMETYPE_CONTAINER_MPEG2TS     video/mp2ts
MEDIA_MIMETYPE_CONTAINER_WVM         video/wvm
MEDIA_MIMETYPE_AUDIO_AAC_ADTS        audio/aac-adts
MEDIA_MIMETYPE_CONTAINER_MPEG2PS     video/mp2p

I'm not which, if any of these, we also want to add.

Comment 15

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

In my opinion, we should add none of those.

We don't want to encourage format inflation, we want authors to be able to target the smallest set of codecs/containers to get the widest possible coverage across all browsers.

Comment 16

[Dave Hylands [:dhylands]]

Being a good boy and submitted to try first:
https://tbpl.mozilla.org/?tree=Try&rev=5021785d573b

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b4ad0fcea64

Comment 18

[[e-mail], [e-mail]]

I got some error log with "unknow-content-type".

===== log =====
E/GeckoConsole( 5538): [JavaScript Warning: "HTTP "Content-Type" of "application/x-unknown-content-type" is not supported. Load of media resource app://clock.gaiamobile.org/style/ringtones/ALARM_progressive_dapple.mp3 failed." {file: "app://clock.gaiamobile.org/onring.html" line: 0}]

Gaia:  12a76c897aea24bdb3dee1cad0eb090bd3ffa2d2
Gecko: add440caf2f26a55f86e456502689709a9fed143

Comment 19

[Gene Lian (I already quit Mozilla)]

Hi dhylands :)

Could you please revisit this issue? According to Ian's investigation, our Alarm/Music Apps on B2G doesn't work after this change.

Comment 20

[Andreas Gal :gal]

Reading files from a package always results in application/x-unknown-content-type I think. I ran into the same problem with video. We could fix this in the reader code I guess, or try to guess the MP3 mime type from the file name.

Comment 21

[Andreas Gal :gal]

Jonas, what do you think?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Gal :gal from comment #20)
> Reading files from a package always results in
> application/x-unknown-content-type I think. I ran into the same problem with
> video. We could fix this in the reader code I guess, or try to guess the MP3
> mime type from the file name.

If it returned application/octet-stream, we would sniff the file to find out the type.

I think we should add application/x-unknown-content-type to the sniff list. Edwin can do this.

Comment 23

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

Attachment: Fix ifdef

Nothing wrong with the approach here; only issue is that we no longer use media plugins for MP3 on B2G.

Comment 24

[Dave Hylands [:dhylands]]

Comment on attachment 675416 [details] [diff] [review]
Fix ifdef

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

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +481,5 @@
>    { AUDIO_WEBM, "webm", "Web Media Audio" },
>  #ifdef MOZ_DASH
>    { APPLICATION_DASH, "mpd", "DASH Media Presentation Description" },
>  #endif
> +#if defined(MOZ_MEDIA_PLUGINS) or defined(MOZ_WIDGET_GONK)

I've never seen the word "or" used in this context. I've always seen ||.
Just to convince myself, I tried with g++/gcc 4.6 and it seems to work.

I'm concerned about compatibility though. Do you know when the word "or" was allowed?

I'm going to say that you should use || (unless you can convince me that this is supported in all of the compilers that we care about)

Comment 25

[Dave Hylands [:dhylands]]

r=me with or changed to || (so you don't have to re-review)

Comment 26

[Andreas Gal :gal]

dhylands, can you please take this bug and land it with your changes?

Thanks.

Comment 27

[Dave Hylands [:dhylands]]

My changes were landed a while ago, but I'm happy to land the new fix.

Comment 28

[Dave Hylands [:dhylands]]

Pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b45ff8ef0d

Comment 29

[Dave Hylands [:dhylands]]

Attachment: Fix #ifdef v2

Updating the patch that was pushed for completeness

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47b45ff8ef0d

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a1e0c0fcaa9

Comment 32

[Peter Dolanjski [:pdol]]

Ignore move to P1, didn't realize it was already fixed.