899042 - Update the mimesniff spec to a better WebM sniffing algorithm

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

Attachment: webm-sniff.tar.gz

Since bug 835381, we are using an algorithm to sniff WebM [1] more strict than what is is the spec [2]. We should propose this new algorithm for inclusion in the spec.

Since we are using nestegg to do this new sniffing, we need to write an independent algorithm (without all the nestegg stuff), simple enough to be implemented without any problems. The attached archive contains a test program and a few matroska files (some of them being webm, some of them mkv), to test.

The proposed algorithm would be something along the lines of:

- Match the EBML ID
- Until we go past the maximum offset a doctype can be found at (reading the spec, I think it is 38, the reasoning is explained in the source file), try to match a doctype id, and then the 'webm' doctype.

[1]: http://mxr.mozilla.org/mozilla-central/source/media/libnestegg/src/nestegg.c#2218
[2]: http://mimesniff.spec.whatwg.org/#matching-an-audio-or-video-type-pattern

Comment 1

[Matthew Gregan [:kinetik]]

Looks good!

- sniff_webm: return error at end of function
- sniff_webm_file: unused variable header_size
- sniff_webm_file: handle error from fread
- make MAX_SNIFF_OFFSET and DOCTYPE_MAX_OFFSET size_t (or unsigned)

Comment 2

[Paul Adenot (:padenot)]

I've got a draft of what the spec could look like [1], but I think I should just send an email to the whatwg.

[1]: http://paul.cx/public/Overview.html#matches-the-signature-for-webm

Comment 3

[Matthew Gregan [:kinetik]]

Having read the spec draft, I think step 5.1.1 should decode (or match all of the possible encodings -- need to check if Matroska/WebM requires the minimal encoding) and verify the doctype string length rather than skipping it, otherwise it will match (hypothetical) stuff like "webmonkey".

Comment 4

[Paul Adenot (:padenot)]

So, I've seen files that have 'webm\0', with a doctype string length of 5, and files with 'webm', with a doctype string length of 4.

Concerning the encoding part, the spec says that the type of the doctype should be 'String', and this means ASCII, from 0x20 to 0x7e, so we don't have to do decoding. But that means files that have 'webm\0' as doctype are actually invalid (because \0 is outside the [0x20, 0x7e] range).

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Paul Adenot (:padenot) from comment #4)
> So, I've seen files that have 'webm\0', with a doctype string length of 5,
> and files with 'webm', with a doctype string length of 4.

Really? :-(

Well, accepting (length=4, "webm") and (length=5, "webm\0") might still be better than accepting any length string prefixed with "webm".
 
> Concerning the encoding part, the spec says that the type of the doctype
> should be 'String', and this means ASCII, from 0x20 to 0x7e, so we don't
> have to do decoding. But that means files that have 'webm\0' as doctype are
> actually invalid (because \0 is outside the [0x20, 0x7e] range).

Sorry, I meant the encoding of the size field itself.  The rest of the spec is able to do simple byte comparisons, but for the size field you'd either need to decode the vint before verifying or include the vint literal bytes for every possible length encoding (or verify that it's not permissible to encode the size vint more than one way for a given value, which may be true but violated in the wild).

Comment 6

[Paul Adenot (:padenot)]

(In reply to Matthew Gregan [:kinetik] from comment #5)
> (In reply to Paul Adenot (:padenot) from comment #4)
> > So, I've seen files that have 'webm\0', with a doctype string length of 5,
> > and files with 'webm', with a doctype string length of 4.
> 
> Really? :-(
> 
> Well, accepting (length=4, "webm") and (length=5, "webm\0") might still be
> better than accepting any length string prefixed with "webm".

Agreed. 

> > Concerning the encoding part, the spec says that the type of the doctype
> > should be 'String', and this means ASCII, from 0x20 to 0x7e, so we don't
> > have to do decoding. But that means files that have 'webm\0' as doctype are
> > actually invalid (because \0 is outside the [0x20, 0x7e] range).
> 
> Sorry, I meant the encoding of the size field itself.  The rest of the spec
> is able to do simple byte comparisons, but for the size field you'd either
> need to decode the vint before verifying or include the vint literal bytes
> for every possible length encoding (or verify that it's not permissible to
> encode the size vint more than one way for a given value, which may be true
> but violated in the wild).

If we end up doing like you propose above (matching 'webm' and 'webm\0'), and because (1) the domain of the values we are interested in is just the vint representation of 4 or 5  and (2) the vint encoding is bijective, we can just compare the size to 0x84 or 0x85. If someone screws up the vint encoding in their encoder, I'm not sure we'd be able to play the media anyways.

On a related note, I'm not sure what would be the doctype of an eventual WebM2 container, we might want to take that into account as well.

Comment 7

[Paul Adenot (:padenot)]

I found the encoder that produces a 5 character doctype.

> $ gst-launch-0.10 filesrc location=input.mp4  ! decodebin name=demux ! queue !
> ffmpegcolorspace ! vp8enc ! webmmux name=mux ! filesink location=out.webm demux. ! queue ! 
> progressreport ! audioconvert ! audioresample ! vorbisenc ! mux.

> xxd out.webm | head -n10
> >  0000000: 1a45 dfa3 0100 0000 0000 0010 4282 8577  .E..........B..w
> > 0000010: 6562 6d00 4287 8102 4285 8102 1853 8067  ebm.B...B....S.g

0x85 for the size, and [0x77, 0x65, 0x62, 0x6d, 0x00] for the actual doctype.

I'll post on their mailing list (and maybe send a patch, haven't looked if it's easy yet).

Anyway, given how mainstream gstreamer is, we can't really ignore this. I'll update the code and the proposal accordingly.

Comment 8

[Paul Adenot (:padenot)]

In fact, re-reading the spec, Matroska accept zero padded strings, so gstreamer's implementation is actually spec compliant.

Comment 9

[Paul Adenot (:padenot)]

I updated the code to take into account the fact that strings can be padded.

It now lives here: https://github.com/padenot/webm-sniff

Comment 10

[Paul Adenot (:padenot)]

I forgot that this has landed in the spec upstream: https://mimesniff.spec.whatwg.org/#signature-for-webm, we can close this, as we have had the code in gecko implementing this sniffing algorithm for quite a while now, living at https://dxr.mozilla.org/mozilla-central/source/media/libnestegg/src/nestegg.c#2566.