Closed
Bug 862088
Opened 11 years ago
Closed 11 years ago
add raw mp3 to nsMediaSniffer
Categories
(Core :: Web Audio, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: rillian, Assigned: rillian)
References
(Blocks 2 open bugs, )
Details
(Whiteboard: [parity-chrome])
Attachments
(1 file, 5 obsolete files)
22.73 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
Forked from https://bugzilla.mozilla.org/show_bug.cgi?id=861187#c5 nsMediaSniffer only detects mp3 streams with id3 tags. That's the only method documented in http://mimesniff.spec.whatwg.org/ iirc because it's the mask/compare method which works on the first 500 bytes. Unfortunately the decodeAudioData() method from the webaudio api provides no way to pass a content type so it seems we're supposed to sniff the type. (https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#dfn-decodeAudioData) The page from the evangelism bug is mp3 without an id3 tag, so we pass it to nsMediaSniffer, get back content/octet-stream, and reject it. Chrome is presumedly sniffing it. I propose we add support for raw mp3 to nsMediaSniffer based on parsing the packet length and looking for a second capture pattern at the expected offset. Then we should push to standardize that, or a better method in the mimesniff spec. I have some code for this at http://svn.xiph.org/experimental/giles/mp3dump.c which can act as a starting point.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [parity-chrome]
Assignee | ||
Comment 1•11 years ago
|
||
My framing code is based on the file format description at https://en.wikipedia.org/wiki/Mp3
Assignee | ||
Comment 2•11 years ago
|
||
trangential note from irc: for webm we call nestegg_sniff which verifies the doctype. We should also propose this for the spec, which currently returns video/webm for any ebml file. This involves writing up a minimal parser to find the doctype.
Assignee | ||
Comment 3•11 years ago
|
||
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21702
Comment 5•11 years ago
|
||
Do I need ID3v1 to satisfy this or ID3v2?
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Kevin Gadd (:kael) from comment #5) > Do I need ID3v1 to satisfy this or ID3v2? Only ID3v2 is useful for sniffing, and that's what we have already. ID3v1 comes at the end of the file. This bug is about detecting raw mp3 streams from their framing as I understand it. That would work on ID3v1-tagged files too.
Flags: needinfo?(giles)
Assignee | ||
Comment 8•11 years ago
|
||
Sorry, I didn't read bug 865553 before responding. The work-around is to add an ID3v2 tag.
This would make a good first Web Audio bug for Ralph :-)
Assignee: nobody → giles
Flags: needinfo?(roc)
Assignee | ||
Comment 11•11 years ago
|
||
Ha, ok. I'll look at it next week.
Comment 12•11 years ago
|
||
Please consider Shoutcast streams when sniffing audio/mpeg.
Comment 13•11 years ago
|
||
Ralph, you might want to have a look at the patch set available in bug 831224, where some kind of mp3 bitstream parser had to be implemented to have an accurate duration reporting for mp3.
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to spender from comment #12) > Please consider Shoutcast streams when sniffing audio/mpeg. Can you be more specific about what you want to work? Are you asking us to handle the icy metadata interleave?
Comment 15•11 years ago
|
||
Ralph, At the moment, shoutcast (audio/mpeg) streams fail to play under the audio tag, even without the icy-metadata (FF Windows). My experiments with icecast were spectacularly bad, leading to reproducible app crash in Firefox. For instance: <audio src="http://94.25.53.133:80/nashe-128-3.mp3" controls="controls" type="audio/mpeg" style="width: 300px;height: 100px;"></audio> --boom-- Shoutcast (audio/mpeg) just doesn't play... I assume the sniffing is at fault. (In order to get Shoutcast to serve audio/mpeg instead of text/html it's necessary to append a semicolon to the end of the stream url)
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to spender from comment #15) > At the moment, shoutcast (audio/mpeg) streams fail to play under the audio > tag, even without the icy-metadata (FF Windows). My experiments with icecast > were spectacularly bad, leading to reproducible app crash in Firefox. Which Firefox and which version of Windows? If you're getting a crash please file a separate bug. This shouldn't happen. FWIW, the stream works fine for me in FF 22.0a2 on Windows 7. Tested with "var a = new Audio('http://94.25.53.133/nashe-128-3.mp3'); a.play();" But I note this stream claims to be Icecast 2.3 and not Shoutcast, and doesn't interleave metadata. Were you using "shoutcast" generically for http streaming mp3?
Comment 17•11 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #16) > (In reply to spender from comment #15) > > > At the moment, shoutcast (audio/mpeg) streams fail to play under the audio > > tag, even without the icy-metadata (FF Windows). My experiments with icecast > > were spectacularly bad, leading to reproducible app crash in Firefox. > > Which Firefox and which version of Windows? If you're getting a crash please > file a separate bug. This shouldn't happen. And which service pack do you have installed? We had to disable MP3 playback on Windows 7 without any service packs installed due to random crashes that we were unable to reproduce. If you've figured out a way to reliably make Firefox crash, we want to know about it!
Comment 18•11 years ago
|
||
Hi Ralph, Chris, Firstly, I should clear up some confusion. My interest in this is to promote Shoutcast support so that our site can ditch Flash Player altogether and move to "standards" based delivery (yes... calling mp3 "standard" is perhaps inflammatory... sorry!). In my experiments, I have not been able to get Shoutcast (audio/mpeg) streams to play at all (without interleaved meta-data; I'm not sure it's even possible to send the Icy-MetaData:1 header from the browser... anyway, it's a red herring). I didn't provide a stream for shoutcast in my previous message, so for completeness, here's an example of a failing SC stream (in my experience, they all fail in the same way): <!DOCTYPE html> <html> <body> <!--note the semicolon after the URL... it's important--> <audio src="http://206.217.201.136:8016/;" controls="controls" type="audio/mpeg" style="width: 300px;height: 100px;"></audio> </body> </html> So, in Firebug, I see the browser attempts to download 13K of the stream, then simply aborts the download. In all likelihood, I suspect that this is related to the broken ICY headers sent by Shoutcast that are missing a CRLF somewhere IIRC. With regards to the Icecast crash, the HTML I provided above reliably crashes my machine (WinServer2012 fully up to date). I'd very much like to report this at length... I'm completely new to the Mozilla bug process and would like to provide as much info as possible and post to the right place. Can you suggest what info I could gather to support the submission. I can competently gather whatever information is requested, so if it requires a deeper probe into WMF settings (or whatever), I could gather that info.
Comment 19•11 years ago
|
||
It occurs to me that I'm using release version. Will switch to nightly later today and report back.
Assignee | ||
Comment 20•11 years ago
|
||
I've filed bug 869632 about the shoutcast text/plain crash. Please follow up there.
Assignee | ||
Comment 21•11 years ago
|
||
Spender, as far as this bug goes, and if I understand properly, you're appending a semicolon to the shoutcast url to get it to set Content-Type: text/plain and hoping to trigger the sniffer? We only sniff if the header is missing or reports application/octet-stream. If I leave off the semicolon it sends text/html, which the audio tag rejects. If I pull the stream with wget it sends audio/mpeg. If I pull the stream with wget and 'Mozilla' in the user-agent string it returns text/html. And the mime-type is correct: it returns a status page instead of the stream itself based on this agent sniffing, as you can verify by pasting the same url in the address bar. Looks like this is a shoutcast bug. Please raise the issue with them. Once this bug is fixed you should be able to work around in javascript using XHR to feed the Web Audio api as well.
Comment 22•11 years ago
|
||
Ralph, yes... it's a "quirk" of Shoutcast... It sniffs the UA for substring "Mozilla". If it's not there, will serve `audio/mpeg`. If it's there but there's no semicolon, you'll get a status page of "text/html", but if there is a semicolon in the page, it will send the stream, regardless of UA. Here's my interpretation: SC server actually sends ICY 200 response line. This is only *approximately* HTTP conformant, but seems to not be parsed by FF (most likely because of ICY header, or missing CRLF). In these broken headers we have an "audio/mpeg" content-type. Because of issues above it appears that this is being ignored. There's certainly no "text/plain" in sight and suggests that there might be a problem elsewhere. I suspect what gets passed on as content includes the corrupt headers. I would expect this to be interpreted as a stream with no content-type and a bunch of junk followed by raw mp3 data (with no metadata interleave). The "text-plain" is definitely wrong. I'd venture that Shoutcast is a de-facto standard that has existed since 1999. Lots of broadcasters are using outdated server versions with little motivation to change. A considerable amount of Flash Player content on the web exists solely to support playing such streams which holds back adoption of newer standards based clients. Any chance of a reconsider of "working as intended"?
Comment 23•11 years ago
|
||
If I understand correctly, the junk header means that the response is considered to be HTTP "0.9". Does this force "text/plain"?
Assignee | ||
Comment 24•11 years ago
|
||
We're getting off topic again. I've filed bug 869725 to follow up with the shoutcast header/content-type issue.
Comment 25•11 years ago
|
||
Do we need to raise a spec issue on this?
Comment 26•11 years ago
|
||
We might need to, if we end up finding that we cannot determine that a file is a valid mp3 bitstream without ID3 header using no more than the 512 bytes the spec talks about. In any case, when we come up with a reliable technique to sniff mp3 bitstreams when there is no ID3 header (whether we need more than 512 bytes or not), it's probably going to go in the spec ([1]). [1]: "MP3 audio without ID3 tags.": http://mimesniff.spec.whatwg.org/#matching-an-audio-or-video-type-pattern
Assignee | ||
Comment 27•11 years ago
|
||
I volunteer to write up whatever we end up doing for the whatwg mimesniff spec.
Comment 28•11 years ago
|
||
Ralph, thank you! That sounds perfect.
Assignee | ||
Comment 29•11 years ago
|
||
Sniff based on header offset. Adds tests by copying files from content/media/tests, but I'd like to truncate them to the minimum length and add an negative case.
Assignee | ||
Comment 30•11 years ago
|
||
Green on try. https://tbpl.mozilla.org/?tree=Try&rev=6bce4f04e397
Assignee | ||
Comment 31•11 years ago
|
||
Fixed up the tests. Derf, if you have time I'd appreciate your review for overflow issues.
Attachment #751635 -
Attachment is obsolete: true
Attachment #751961 -
Flags: review?(paul)
Attachment #751961 -
Flags: feedback?(tterribe)
Comment 32•11 years ago
|
||
Comment on attachment 751961 [details] [diff] [review] sniff by packet offset v3 Review of attachment 751961 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/mediasniffer/mp3sniff.c @@ +9,5 @@ > +#include <stdint.h> > + > +#include "mp3sniff.h" > + > +#define MP3_MAX_SIZE 1441 Can you add a comment explaining the reasoning behind this apparently magic value? @@ +24,5 @@ > + int modex; > + int copyright; > + int original; > + int emphasis; > +} mp3_header; We don't need to store all that, right? I guess it does not matter that much, though. @@ +26,5 @@ > + int original; > + int emphasis; > +} mp3_header; > + > +static int mp3_parse(const uint8_t *p, mp3_header *header) I guess you can make that a void function, considering you always return 0 and never use the returned value. @@ +31,5 @@ > +{ > + const int bitrates[16] = > + {0, 32000, 40000, 48000, 56000, 64000, 80000, 96000, > + 112000, 128000, 160000, 192000, 224000, 256000, 320000, 0}; > + const int samplerates[4] = {44100, 48000, 32000}; You need a final 0 here as the fourth array member. @@ +136,5 @@ > + mp3_header header; > + const uint8_t *p, *q; > + long skip; > + long avail; > + FILE *out = stdout; Remove or #define that out (and all the related debug stuff).
Attachment #751961 -
Flags: review?(paul) → review+
Assignee | ||
Comment 34•11 years ago
|
||
Updated in response to review comments. - Add dummy entry for invalid samplerate table entry. - Document max mp3 header size calculation. - Remove return code from mp3_parse. - Remove debug output. - Improve comments. >> +} mp3_header; > > We don't need to store all that, right? I guess it does not matter that much, though. Correct. I'd like to keep it complete for now. Can trim it down after I've written the spec version.
Attachment #751961 -
Attachment is obsolete: true
Attachment #751961 -
Flags: feedback?(tterribe)
Attachment #753152 -
Flags: feedback?(tterribe)
Assignee | ||
Comment 35•11 years ago
|
||
Carrying forward previous r+ with irc assent. https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc40bbe844e
Assignee | ||
Comment 36•11 years ago
|
||
*shame* https://hg.mozilla.org/integration/mozilla-inbound/rev/bde36c2eacc0
Assignee | ||
Comment 37•11 years ago
|
||
Update to remove dump_header call missed in the previous version of the patch.
Attachment #753152 -
Attachment is obsolete: true
Attachment #753152 -
Flags: feedback?(tterribe)
Comment 38•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8dc40bbe844e https://hg.mozilla.org/mozilla-central/rev/bde36c2eacc0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 39•11 years ago
|
||
This causes all .exe files to now be saved as .exe.mp3. Can someone please fix this?
Comment 41•11 years ago
|
||
http://forums.mozillazine.org/viewtopic.php?p=12878657#p12878657 Alice confirms it happens also on the latest hourly builds, maybe it only happens on Windows? Not really sure.
Flags: needinfo?(ionnv)
Comment 42•11 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #40) > Can you provide a link? I can't reproduce, here. http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1368612627/ Try to download the .exe installer, says cannot play mp3.... win7 x64 here tested on cset: https://hg.mozilla.org/mozilla-central/rev/53bfd38cbc8c which contains this bug...
Comment 43•11 years ago
|
||
Confirmed on Windows 8 x64. Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20130524 Firefox/24.0 ID:20130524032751 CSet: efa4fb561e5b
Comment 44•11 years ago
|
||
(In reply to NVD from comment #39) > This causes all .exe files to now be saved as .exe.mp3. Can someone please > fix this? I will file bug.
Comment 45•11 years ago
|
||
Uh, yeah. Backed out. https://hg.mozilla.org/mozilla-central/rev/218785bab4bd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla24 → ---
Comment 46•11 years ago
|
||
Oh, and more tests wanted please :)
Comment 47•11 years ago
|
||
(In reply to NVD from comment #41) > http://forums.mozillazine.org/viewtopic.php?p=12878657#p12878657 > > Alice confirms it happens also on the latest hourly builds, maybe it only > happens on Windows? Not really sure. I see this on Linux.
Comment 48•11 years ago
|
||
As small sidenote, why was this implemented in C?
Comment 49•11 years ago
|
||
iirc, this is kind of imported from some old code Ralph wrote.
Assignee | ||
Comment 50•11 years ago
|
||
There was a bug giving false positives on a certain sequence which appeared in the installer header. I've fixed this. Will upload a new patch when I've done more tests.
Assignee | ||
Comment 51•11 years ago
|
||
Updated patch. - Fix zero-length packet bug which caused many false positives. - Remove code to skip garbage before the first header. Chrome doesn't do this so we don't need to either. - Add four more tests, including a regression test for bug 875769.
Attachment #753165 -
Attachment is obsolete: true
Attachment #756287 -
Flags: review?(paul)
Comment 52•11 years ago
|
||
Comment on attachment 756287 [details] [diff] [review] sniff by packet offset v7 Review of attachment 756287 [details] [diff] [review]: ----------------------------------------------------------------- We should file a followup to consider allowing sniffing more than 512 bytes. ::: toolkit/components/mediasniffer/mp3sniff.c @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* MPEG format parsing */ > + > +#include <stdio.h> You don't need that. @@ +5,5 @@ > +/* MPEG format parsing */ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <stdint.h> Remove that, it is in mp3sniff.h. ::: toolkit/components/mediasniffer/mp3sniff.h @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include <stdint.h> Prefer #include <mozilla/StandardInteger.h> for the reasons outlined in the file.
Attachment #756287 -
Flags: review?(paul) → review+
Assignee | ||
Comment 53•11 years ago
|
||
Updated patch implementing review comments. Carrying forward padenot's r+. - Remove unnecessary includes. These were used by test code in the upstream version and are no longer necessary. - Use our compatibility header instead of stdint.h.
Attachment #756287 -
Attachment is obsolete: true
Attachment #758092 -
Flags: review+
Assignee | ||
Comment 54•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e693549c022b
Comment 55•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e693549c022b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 56•11 years ago
|
||
Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.
Component: Video/Audio → Web Audio
Comment 57•11 years ago
|
||
Bug https://bugzilla.mozilla.org/show_bug.cgi?id=865553 was marked as a duplicate to this bug, but unfortunately this other bug was not fixed with this bug fix. mp3 files without ID3 tag cannot be decoded with the WebAudio API.
Comment 58•11 years ago
|
||
Then the right thing to do is to reopen the other bug.
You need to log in
before you can comment on or make changes to this bug.
Description
•