Closed Bug 862088 Opened 11 years ago Closed 11 years ago

add raw mp3 to nsMediaSniffer

Categories

(Core :: Web Audio, defect)

Product:
Core
Component:
Web Audio
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: rillian, Assigned: rillian)

References

(Blocks 2 open bugs,
URL
)

Details

(Whiteboard: [parity-chrome])

Attachments

(1 file, 5 obsolete files)

sniff by packet offset v8
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.
Whiteboard: [parity-chrome]
My framing code is based on the file format description at https://en.wikipedia.org/wiki/Mp3
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.
Blocks: webaudio
Do I need ID3v1 to satisfy this or ID3v2?
I'm not sure, Ralph?
Flags: needinfo?(giles)
(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)
Sorry, I didn't read bug 865553 before responding. The work-around is to add an ID3v2 tag.
Can we get somebody who knows this stuff work on it?
Flags: needinfo?(roc)
This would make a good first Web Audio bug for Ralph :-)
Assignee: nobody → giles
Flags: needinfo?(roc)
Ha, ok. I'll look at it next week.
Please consider Shoutcast streams when sniffing audio/mpeg.
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.
(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?
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)
(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?
(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!
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.
It occurs to me that I'm using release version. Will switch to nightly later today and report back.
I've filed bug 869632 about the shoutcast text/plain crash. Please follow up there.
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.
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"?
If I understand correctly, the junk header means that the response is considered to be HTTP "0.9". Does this force "text/plain"?
We're getting off topic again. I've filed bug 869725 to follow up with the shoutcast header/content-type issue.
Do we need to raise a spec issue on this?
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
I volunteer to write up whatever we end up doing for the whatwg mimesniff spec.
Ralph, thank you!  That sounds perfect.
Attached patch sniff by packet offset v2 (obsolete) — — Splinter Review 
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.
Attached patch sniff by packet offset v3 (obsolete) — — Splinter Review 
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 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+
Attached patch sniff by packet offset v4 (obsolete) — — Splinter Review 
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)
Carrying forward previous r+ with irc assent.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc40bbe844e
Attached patch sniff by packet offset v5 (obsolete) — — Splinter Review 
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)
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
This causes all .exe files to now be saved as .exe.mp3. Can someone please fix this?
Can you provide a link? I can't reproduce, here.
Flags: needinfo?(ionnv)
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)
(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...
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
(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.
Depends on: 875769
Uh, yeah. Backed out.
https://hg.mozilla.org/mozilla-central/rev/218785bab4bd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla24 → ---
Oh, and more tests wanted please :)
(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.
As small sidenote, why was this implemented in C?
iirc, this is kind of imported from some old code Ralph wrote.
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.
Attached patch sniff by packet offset v7 (obsolete) — — Splinter Review 
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 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+
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+
Blocks: 879413
Blocks: 879429
https://hg.mozilla.org/mozilla-central/rev/e693549c022b
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: