Last Comment Bug 567077 - Sniff types of media files that are served with no Content-Type
: Sniff types of media files that are served with no Content-Type
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Paul Adenot (:padenot)
:
Mentors:
: 546129 589564 763761 (view as bug list)
Depends on: 767087 781141 789077 1079747
Blocks: 787293 1151811
  Show dependency treegraph
 
Reported: 2010-05-20 03:05 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2015-04-11 05:17 PDT (History)
20 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch v0: sniff to get the media type when we have no content-type or if the content-type is application/octet-stream (10.42 KB, patch)
2012-06-22 18:02 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Review
Patch v0: don't try to resniff when recreating a channel. (5.11 KB, patch)
2012-06-22 18:05 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Review
Patch v0: add some tests for the sniffing. and files too. (49.07 KB, patch)
2012-06-22 18:09 PDT, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Review
Addressed cpearce's comments. (11.89 KB, patch)
2012-06-26 11:02 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Review
Patch v2: addressed last comments (11.92 KB, patch)
2012-06-27 09:47 PDT, Paul Adenot (:padenot)
cpearce: review+
benjamin: review+
Details | Diff | Review
Removed sSnifferEntriesCount. (11.88 KB, patch)
2012-07-03 17:06 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Review
Patch v1: rephrased comment. (5.11 KB, patch)
2012-07-09 15:40 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Avoid sniffing when recreating a channel. r= (5.52 KB, patch)
2012-07-10 09:33 PDT, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Review
Sniff types of media files that are served with no Content-Type (13.18 KB, patch)
2012-08-07 15:55 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Avoid sniffing when recreating a channel. (5.53 KB, patch)
2012-08-07 15:56 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Sniff types of media files that are served with no Content-Type -- Tests (48.68 KB, patch)
2012-08-07 15:56 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Fix an assertion failure when the decoder was cloned. r= (1.41 KB, patch)
2012-08-07 18:04 PDT, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Review
Fix an assertion failure when the decoder was cloned. (1.42 KB, patch)
2012-08-07 19:55 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Sniff types of media files that are served with no Content-Type (12.68 KB, patch)
2012-09-04 16:24 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Avoid sniffing when recreating a channel. (5.53 KB, patch)
2012-09-04 16:24 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Sniff types of media files that are served with no Content-Type -- Tests (48.70 KB, patch)
2012-09-04 16:24 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Fix an assertion failure when the decoder was cloned. (1.42 KB, patch)
2012-09-04 16:25 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Remove the classinfo stubs. r= (1.25 KB, patch)
2012-09-04 16:29 PDT, Paul Adenot (:padenot)
peterv: review+
Details | Diff | Review
Remove the classinfo stubs. (1.25 KB, patch)
2012-09-05 12:27 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Avoid sniffing when recreating a channel. (5.53 KB, patch)
2012-09-05 15:36 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-20 03:05:32 PDT
The spec more or less says we should. I don't see a reason not to.

(The spec also says to treat application/octet-stream like no Content-Type. I'm less sure about doing that.)
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-20 04:58:39 PDT
See http://article.gmane.org/gmane.ietf.http-wg/4340
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-05-20 06:52:34 PDT
Just to explain my reasoning on the post there... Treating application/octet-stream that way would allow servers who just can't tell to just flag the file as "binary data" while still having us handle it sanely.

For what it's worth, <object> falls back on certain kinds of sniffing (e.g. using its @type and the like) for application/octet-stream.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-05-20 06:53:01 PDT
But I don't feel too strongly about the octet-stream part, fwiw.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-20 15:11:03 PDT
I guess it makes sense. So we need to add sniffing logic to Necko, I guess. How do we avoid replicating the sniffing code between Necko and the video code (for the application/octet-stream case)?
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-05-21 10:13:19 PDT
The simplest approach is probably to create a service that implements nsIContentSniffer and register its contract in the "content-sniffing-services" category.  Then nsUnknownDecoder will call it, and you can call it yourself too (either via the nsIContentSniffer API or whatever internal API you prefer).
Comment 6 Matthew Gregan [:kinetik] 2010-10-13 19:32:35 PDT
*** Bug 602597 has been marked as a duplicate of this bug. ***
Comment 7 Chris Pearce (:cpearce) 2011-09-18 21:24:00 PDT
If we won't implement content sniffing, what about naively mapping the file extension to mime type when we encounter a channel with content-type application/octet-stream in nsHTMLMediaElement::InitializeDecoderForChannel? That would make us more likely to be able to play something in our most most common (and probably our most unintuitive to debug) failure case.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-19 05:10:47 PDT
I would much rather do magic byte sniffing than extension sniffing if we do sniffing at all, for what it's worth.
Comment 9 Chris Pearce (:cpearce) 2011-09-19 15:26:44 PDT
I have no problem implementing simple magic byte sniffing in preference to extension sniffing.

I think we should do *something* here soon, as we're being bitten by this and it looks really bad when HTML5 media Just Works in browsers that sniff but not Firefox.
Comment 10 Chris Pearce (:cpearce) 2011-09-20 15:21:46 PDT
Boris, would the approach you outline in comment 5 mean that we always open an nsVideoDocument whenever we sniff that an application/octet stream has media content? Meaning the only possible way to download media is in an nsVideoDocument? I think I'd prefer to sniff only when loading a channel inside nsHTMLMediaElement, so users/developers still have the option to download media outside of our built in player. I note that Chrome takes this approach.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-20 19:56:47 PDT
> Boris, would the approach you outline in comment 5 mean that we always open an
> nsVideoDocument whenever we sniff that an application/octet stream has media content? 

It would mean that if the server sends no MIME type at all, or if you're dealing with a file:// channel or some other protocol that has no MIME type metadata your sniffer would be invoked and you would get to set a useful type on the data.  Things actually sent as application/octet-stream would NOT be affected by this.

> I think I'd prefer to sniff only when loading a channel inside nsHTMLMediaElement

For cases when the type is actually application/octet-stream, yes.  Comment 5 was about how to use the same code to do both application/octet-stream sniffing inside nsHTMLMediaElement and general sniffing on the necko level when no content type is sent.
Comment 12 Justin Dolske [:Dolske] 2011-09-26 09:53:17 PDT
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #9)

> I think we should do *something* here soon, as we're being bitten by this
> and it looks really bad when HTML5 media Just Works in browsers that sniff
> but not Firefox.

If we implement bug 513405 and have text for this condition, then at least when webdevs run across this they have some idea what's going on (and thus how to fix it). Almost Just Works. :-)
Comment 13 Chris Pearce (:cpearce) 2012-06-11 17:23:20 PDT
*** Bug 763761 has been marked as a duplicate of this bug. ***
Comment 14 Diego Casorran [:diegocr] 2012-06-11 17:28:32 PDT
*** Bug 763761 has been marked as a duplicate of this bug. ***
Comment 15 Paul Adenot (:padenot) 2012-06-13 14:08:47 PDT
I'm in the process of starting working on that. Do we agree that we want to sniff for media when there is no Content-Type, AND when the Content-Type is "application/octet-stream"? The former would be done in the nsUnknowDecoder, the later in nsHTMLMediaElement (when creating the decoder).

I'm planning to implement that feature the way comment 5 describes it, it still seem to be the way to go, looking at the code, but I wanted to make sure I was not missing anything.
Comment 16 Paul Adenot (:padenot) 2012-06-13 14:41:58 PDT
I also plan to use the mimesniff spec [1] from the whatwg. It is incomplete (no mp3, for example), but provides what we need, I suppose.

[1] : http://mimesniff.spec.whatwg.org
Comment 17 Paul Adenot (:padenot) 2012-06-14 16:29:23 PDT
I've implemented what comment 5 mentions, that is a nsIContentSniffer, it get called by nsUnknownDecoder, succeeds to provide the Content-Type by sniffing the media, but then the load fails (Saying that the media is corrupted using a webm or a ogg video) or the media playback is bogus with tons of assertions firing (using an mp4 and the gstreamer backend), despite the right decoder being created in nsHTMLMediaElement.

Investigating the error, I found that images (that are supposed to be sniffed as well when not served with a Content-Type, I used their code for example) don't load as well, they "cannot be displayed because they contain errors". Is that supposed to work?

I suppose this has to do with the fact that nsUnknownDecoder read()s the data from the channel (that is, discards them after using them to determine the content type) instead of peeking the channel, thus providing an incomplete bitstream to the decoder. Reading nsBaseChannel and nsHttpChannel code, both classes can peek and sniff content type, but it is somehow not called.

Do I miss anything, or is the behavior I observe a bug?
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 18:26:47 PDT
nsUnknown decoder can operate in two modes.

It can be a stream converter, in which case it will read the data from the channel, buffer it up, detect the type, then send it on to the final consumer.

Or it can be a content sniffer itself (but not registered with the content sniffer category, so it doesn't try to call itself).

I'd be very interested in what testcase shows the image behavior you describe...
Comment 19 Paul Adenot (:padenot) 2012-06-14 18:58:50 PDT
bz, I've modified [1] cpearce's web server [2] to prevent the Content-Type header to be set if we pass "?nomime" as parameter. Using curl, I checked that this works. I then load a png in Firefox (the urls looks like httpL//localhost:8080/image.pnblah?nomime, with a bizarre extension to prevent url sniffing).

I see in gdb that sniffing occurs, using the first mode you mention, that is, 512 bytes are consumed from the stream by the nsUnknownDecoder and handed to the sniffing logic. The sniffer finds the right type by using these 512 bytes. Then, the channel is used by the image decoders, but the first 512 bytes are missing, so the image does not load.

With my patch, the exact same scenario occurs for a media that is loaded. The gstreamer backend somehow succeeds to make sense of the remaining data, because it can play some frames, but a lot of assertion are failing, and we miss a lot of frames.

[1] : https://github.com/padenot/HttpMediaServer
[2] : https://github.com/cpearce/HttpMediaServer
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 19:34:20 PDT
> Using curl, I checked that this works.

I just tried pulling from [1] above, building it on Linux, and testing it on a 174-byte png I have lying around, and it sent down 174 bytes alright... but the first two bytes were a CRLF and the last two bytes of the image data were missing.  So in my case the result actually got sniffed as application/octet-stream, not as a PNG.

> Then, the channel is used by the image decoders, but the first 512 bytes are missing

The nsUnknownDecoder sends those 512 bytes on to the downstream consumer at http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsUnknownDecoder.cpp#606 and this works just fine last I tested.  Are you hitting that code?

Would you mind attaching your patch?
Comment 21 Paul Adenot (:padenot) 2012-06-14 23:14:11 PDT
bz, the server was indeed faulty. My patch (and the image sniffing code of course) are working with the server fixed, thank you for the catch.

Now the last bits to write/change/discuss :

- how to propagate the sniffed content type when we drop and recreate a channel (because we can't really seek in the middle of a OGG packet anyway). I suppose saving the content type in the MediaResource and setting the content type before recreating the channel will do the trick (according to [1] and [2]).
- where to put the code (would content/media/mediasniffer be acceptable ?), and fiddle with the build system to get it do what I want (the code is in sniffer/ as per now, it is obviously incorrect, but I copied image/).
- exactly which types do we want to sniff for ? I have implemented the sniffing spec for ogg, webm, wave, mp4, and added detection for  mp3 (with and without ID3v2 metadata container, ID3v1 files being at the end of the file, hence of no interest for us), but could add some more if needed.

[1] : http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIChannel.idl#92
[2] : http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#887
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-15 12:35:19 PDT
> I suppose saving the content type in the MediaResource and setting the content type

That will work if you set it in onStartRequest.  It won't work if you set it before asyncOpen and the server is explicitly returning application/octet-stream, sadly.
Comment 23 Paul Adenot (:padenot) 2012-06-22 18:02:15 PDT
Created attachment 635987 [details] [diff] [review]
Patch v0: sniff to get the media type when we have no content-type or if the content-type is application/octet-stream

I'm not sure about the location I put the code in. I picked it kind of randomly by searching stuff in mxr.

The code is pretty straightforward, the sniffing patterns and algorithms are taken from the mime sniff spec [1] when available, and from the file(1) command otherwise (for the mp3 in particular). We might want to complete/finish that spec, that is incomplete for the media types (the pattern for mp3 is marked as TODO).

[1]: http://mimesniff.spec.whatwg.org/
Comment 24 Paul Adenot (:padenot) 2012-06-22 18:05:09 PDT
Created attachment 635989 [details] [diff] [review]
Patch v0: don't try to resniff when recreating a channel.

This is not necessary, but sniffing for a media type when seeking in the middle of a bitstream is kind of a lost cause.
Comment 25 Paul Adenot (:padenot) 2012-06-22 18:09:31 PDT
Created attachment 635991 [details] [diff] [review]
Patch v0: add some tests for the sniffing. and files too.

This patch adds the tests, both in the "no mimetype" and "application/octet-stream" case. It also add mp3 (with and without id3 tags) and mp4 files to tests those when we have gstreamer.
Comment 26 Chris Pearce (:cpearce) 2012-06-24 17:38:51 PDT
Comment on attachment 635987 [details] [diff] [review]
Patch v0: sniff to get the media type when we have no content-type or if the content-type is application/octet-stream

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

Overall, it looks good to me! My comments are mostly trivial changes to make the code more readable, the approach I believe is fine.

::: toolkit/components/mediasniffer/nsMediaSniffer.cpp
@@ +15,5 @@
> +NS_IMPL_CLASSINFO(nsMediaSniffer, NULL, 0, NS_MEDIA_SNIFFER_CID)
> +NS_IMPL_ISUPPORTS1(nsMediaSniffer, nsIContentSniffer)
> +
> +nsMediaSniffer::nsMediaSnifferEntry nsMediaSniffer::sSnifferEntries[] = {
> +// The string oggS, followed by the null byte.

I'd indent the entries here, since they're inside {}.

@@ +30,5 @@
> +
> +PRUint32 nsMediaSniffer::sSnifferEntriesCount =
> +    sizeof(sSnifferEntries) / sizeof(nsMediaSnifferEntry);
> +
> +static bool MatchesMP4(const PRUint8* data, PRUint32 length)

Add a comment explaining that you're following the algorithm at http://mimesniff.spec.whatwg.org/#signature-for-mp4 here.

@@ +32,5 @@
> +    sizeof(sSnifferEntries) / sizeof(nsMediaSnifferEntry);
> +
> +static bool MatchesMP4(const PRUint8* data, PRUint32 length)
> +{
> +  if (length <= 12) {

It's not clear what 12 is here for. Presumably it's some sort of lower bound on the amount of data we need to successfully sniff. You should make this a #define or a "static const unsigned" value with a meaningful name to make its purpose clear.

@@ +35,5 @@
> +{
> +  if (length <= 12) {
> +    return false;
> +  }
> +  // Conversion to big endian.

You're not converting *to* big endian here, you're converting *from* big endian *to* host byte order. Host byte order may or may not be big endian, depending on what machine the code is running on. Typically byte order won't matter; endianness is handled transparently by the hardware in bit shift operations, it only matters if you start messing the octets in the int itself.

@@ +39,5 @@
> +  // Conversion to big endian.
> +  PRUint32 boxSize = (PRUint32)(data[3] | data[2] << 8 | data[1] << 16 | data[0] << 24);
> +
> +  // Boxsize should be evenly divisible by 4.
> +  if (boxSize % 4) {

You also should return here if length<boxSize, otherwise in the loop i<=boxSize below, you could end up trying to read after data[length], which will either be reading garbage or cause a protection fault.

@@ +53,5 @@
> +  for (PRUint32 i = 2; i <= boxSize / 4 - 1 ; i++) {
> +    if (i == 3) {
> +      continue;
> +    }
> +    // The string mp4.

s/mp4/"mp4"/

@@ +65,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsMediaSniffer::GetMIMETypeFromContent(nsIRequest* request,
> +                                      const PRUint8* data,

The 3 lines below the first should line up with the '(', i.e. they should be indented by one more space.

Also your arguments here (and elsewhere) should follow the Mozilla standard aArgumentNamingConvention please.

Also, please make as many of them const as possible, even the plain-old-data types; in general it reduces code clarity if you change the value of arguments to functions unless they're out params, so make the arguments const so that can't happen.

@@ +73,5 @@
> +  nsCOMPtr<nsIHttpChannel> channel(do_QueryInterface(request));
> +  if (!channel) {
> +    return NS_ERROR_NO_INTERFACE;
> +  }
> +

The spec specifies that n (the length of data) should be capped at 512 bytes, so please enforce that.

@@ +74,5 @@
> +  if (!channel) {
> +    return NS_ERROR_NO_INTERFACE;
> +  }
> +
> +  for (PRUint32 i = 0; i < sSnifferEntriesCount; ++i) {

Use NS_ARRAY_LENGTH(sSnifferEntries) rather than storing nsMediaSniffer::sSnifferEntriesCount and making it public.

@@ +75,5 @@
> +    return NS_ERROR_NO_INTERFACE;
> +  }
> +
> +  for (PRUint32 i = 0; i < sSnifferEntriesCount; ++i) {
> +    if (length < sSnifferEntries[i].mLength) {

if (length < sSnifferEntries[i].mLength || sSnifferEntries[i].mLength == 0)

Also, because you're nesting your loops here and they're not small, I think it would be clearer to either create a const nsMediaSnifferEntry* (or reference) to reduce the number of array indexes scattered around all over the place, or rename i and j so that their purpose is more obvious.

@@ +79,5 @@
> +    if (length < sSnifferEntries[i].mLength) {
> +      continue;
> +    }
> +    bool matched = false;
> +    for (PRUint32 j = 0; j < sSnifferEntries[i].mLength; ++j) {

j < sSnifferEntries[i].mLength && j < length;

@@ +84,5 @@
> +      if ((sSnifferEntries[i].mMask[j] & data[j]) != sSnifferEntries[i].mPattern[j]) {
> +        matched = false;
> +        break;
> +      } else {
> +        matched = true;

Initialize matched to true above, then you don't need this branch (provided you |continue| when sSnifferEntries[i].mLength==0 above).

@@ +99,5 @@
> +    return NS_OK;
> +  }
> +
> +  // Could not sniff the media type.
> +  return NS_ERROR_NOT_AVAILABLE;

Doesn't the spec require us to assign application/octet-stream to the mime type if we don't get a match?

::: toolkit/components/mediasniffer/nsMediaSniffer.h
@@ +20,5 @@
> +#define NS_MEDIA_SNIFFER_CONTRACTID "@mozilla.org/media/sniffer;1"
> +
> +class nsMediaSniffer : public nsIContentSniffer
> +{
> +

Delete this unnecessary blank line please.

@@ +33,5 @@
> +
> +  struct nsMediaSnifferEntry {
> +    const PRUint8* mMask;
> +    const PRUint8* mPattern;
> +    PRUint32 mLength;

Can mLength be const?

::: toolkit/components/mediasniffer/nsMediaSnifferModule.cpp
@@ +5,5 @@
> +
> +#include "mozilla/ModuleUtils.h"
> +
> +#include "nsMediaSniffer.h"
> +

I'm not really qualified to review this contract/xpcom stuff.
Comment 27 Chris Pearce (:cpearce) 2012-06-24 17:41:45 PDT
Comment on attachment 635989 [details] [diff] [review]
Patch v0: don't try to resniff when recreating a channel.

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

Looks good, again just needs trivial changes.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2373,5 @@
>  {
>    NS_ASSERTION(mLoadingSrc, "mLoadingSrc must already be set");
>    NS_ASSERTION(mDecoder == nsnull, "Shouldn't have a decoder");
>  
> +  aChannel->GetContentType(mMimeType);

This sets mMimeType = mMimeType.

Lets keep the old code using the GetContentType() accessor. Acessors are good, because they centralize access to the underlying data, allow the underlying structure to change without large changes to the code base.

Also, does it make sense to assert that !contentType.IsEmpty() ? The sniffer should assign application/octect-stream if it doesn't find a match right?

::: content/media/MediaResource.cpp
@@ +696,5 @@
> +                              nsnull,
> +                              loadFlags);
> +
> +  // We have cached the Content-Type, which should not change. Give a hint to
> +  // the channel to avoid a sniffing failure.

Why would we expect sniffing to fail when we recreate the channel? Because we're usually requesting a byte range which doesn't start at offset 0? Your comment should make it clear why we expect failure.

In general, add comments when the code isn't obvious enough by itself to make it clear what we're doing, and why we're doing it.
Comment 28 Chris Pearce (:cpearce) 2012-06-24 17:50:32 PDT
Comment on attachment 635991 [details] [diff] [review]
Patch v0: add some tests for the sniffing. and files too.

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

::: content/media/test/test_media_sniffer.html
@@ +40,5 @@
> +  ok(true, "The media loads when served without a Content-Type.");
> +  checkApplicationOctetStream(t);
> +}
> +
> +function onerror(e) {

Are you building with gstreamer enabled? The mp4/mp3 files should be failing to load on builds without gstreamer... Did you push this to Try?
Comment 29 Paul Adenot (:padenot) 2012-06-24 18:14:15 PDT
(In reply to Chris Pearce (:cpearce) from comment #28)
> Comment on attachment 635991 [details] [diff] [review]
> Patch v0: add some tests for the sniffing. and files too.
> 
> Review of attachment 635991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/test_media_sniffer.html
> @@ +40,5 @@
> > +  ok(true, "The media loads when served without a Content-Type.");
> > +  checkApplicationOctetStream(t);
> > +}
> > +
> > +function onerror(e) {
> 
> Are you building with gstreamer enabled? The mp4/mp3 files should be failing
> to load on builds without gstreamer... Did you push this to Try?

I've tested both on a gstreamer enabled build, and on a default build, both works (i.e. the test is green).

According to the logic in content/media/manifest.js, the media that we can't play are skipped (in the same fashion as if we were compiling without, say, OGG support), thus are not passed to the |startTest| function. Or maybe I don't understand you remark.
Comment 30 Chris Pearce (:cpearce) 2012-06-24 19:06:43 PDT
Comment on attachment 635991 [details] [diff] [review]
Patch v0: add some tests for the sniffing. and files too.

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

Ah yes, of course. Thanks!
Comment 31 Paul Adenot (:padenot) 2012-06-25 15:50:26 PDT
(In reply to Chris Pearce (:cpearce) from comment #27)
> Comment on attachment 635989 [details] [diff] [review]
> Patch v0: don't try to resniff when recreating a channel.
> 
> Review of attachment 635989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, again just needs trivial changes.
> 
> ::: content/html/content/src/nsHTMLMediaElement.cpp
> @@ +2373,5 @@
> >  {
> >    NS_ASSERTION(mLoadingSrc, "mLoadingSrc must already be set");
> >    NS_ASSERTION(mDecoder == nsnull, "Shouldn't have a decoder");
> >  
> > +  aChannel->GetContentType(mMimeType);
> 
> This sets mMimeType = mMimeType.
> 
> Lets keep the old code using the GetContentType() accessor. Acessors are
> good, because they centralize access to the underlying data, allow the
> underlying structure to change without large changes to the code base.

I'm not too sure of what you are asking here. I'm still using |GetContentType()|, but merely caching the result for later use. I may be missing something, though.

> Also, does it make sense to assert that !contentType.IsEmpty() ? The sniffer
> should assign application/octect-stream if it doesn't find a match right?
Yes.
Comment 32 Chris Pearce (:cpearce) 2012-06-25 16:08:11 PDT
(In reply to Paul ADENOT (:padenot) from comment #31)
> (In reply to Chris Pearce (:cpearce) from comment #27)
> > Comment on attachment 635989 [details] [diff] [review]
> > Patch v0: don't try to resniff when recreating a channel.
> > 
> > Review of attachment 635989 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good, again just needs trivial changes.
> > 
> > ::: content/html/content/src/nsHTMLMediaElement.cpp
> > @@ +2373,5 @@
> > >  {
> > >    NS_ASSERTION(mLoadingSrc, "mLoadingSrc must already be set");
> > >    NS_ASSERTION(mDecoder == nsnull, "Shouldn't have a decoder");
> > >  
> > > +  aChannel->GetContentType(mMimeType);
> > 
> > This sets mMimeType = mMimeType.
> > 
> > Lets keep the old code using the GetContentType() accessor. Acessors are
> > good, because they centralize access to the underlying data, allow the
> > underlying structure to change without large changes to the code base.
> 
> I'm not too sure of what you are asking here. I'm still using
> |GetContentType()|, but merely caching the result for later use. I may be
> missing something, though.

Oops, I misread your code and thought you were calling nsHTMLMediaElement::GetContentType() here. My mistake. Please ignore this comment; your change here is fine.
Comment 33 Paul Adenot (:padenot) 2012-06-26 11:02:57 PDT
Created attachment 636790 [details] [diff] [review]
Addressed cpearce's comments.

I found a cool header file with all the mime types #defined, so I replaced the hard coded strings by the defines, and addressed all the other remarks.

This is green on try (with the patch for bug 767087 applied), https://tbpl.mozilla.org/?tree=Try&rev=af53f8131c39.

I need someone to review the xpcom registration bits also, I think. And the build system bits, because I've no idea what I'm doing here.
Comment 34 Chris Pearce (:cpearce) 2012-06-26 17:07:31 PDT
Comment on attachment 636790 [details] [diff] [review]
Addressed cpearce's comments.

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

bsmedberg can review the XPCOM/toolkit stuff.

::: toolkit/components/mediasniffer/nsMediaSniffer.cpp
@@ +78,5 @@
> +  const PRUint32 clampedLength = NS_MIN(aLength, MAX_BYTES_SNIFFED);
> +
> +  for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(sSnifferEntries); ++i) {
> +    const nsMediaSnifferEntry& currentEntry = sSnifferEntries[i];
> +    if (clampedLength < sSnifferEntries[i].mLength) {

s/sSnifferEntries[i]/currentEntry/

Same thing below as well too.

@@ +83,5 @@
> +      continue;
> +    }
> +    bool matched = false;
> +    for (PRUint32 j = 0; j < sSnifferEntries[i].mLength; ++j) {
> +      if ((currentEntry.mMask[j] & aData[j]) != currentEntry.mPattern[j]) {

I had a comment about this branch and the "matched" variable, can you address it please?
Comment 35 Paul Adenot (:padenot) 2012-06-27 09:47:41 PDT
Created attachment 637164 [details] [diff] [review]
Patch v2: addressed last comments
Comment 36 Paul Adenot (:padenot) 2012-06-27 09:49:07 PDT
Comment on attachment 637164 [details] [diff] [review]
Patch v2: addressed last comments

bsmedbergs, could you review the xpcom registration/build system bits of this patch ?
Comment 37 Chris Pearce (:cpearce) 2012-06-27 16:29:50 PDT
Comment on attachment 637164 [details] [diff] [review]
Patch v2: addressed last comments

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

Looks great. But you forgot to remove sSnifferEntriesCount. r+ with sSnifferEntriesCount removed.

::: toolkit/components/mediasniffer/nsMediaSniffer.h
@@ +37,5 @@
> +    const char* mContentType;
> +  };
> +
> +  static nsMediaSnifferEntry sSnifferEntries[];
> +  static PRUint32 sSnifferEntriesCount;

Remove sSnifferEntriesCount...
Comment 38 Paul Adenot (:padenot) 2012-07-03 17:06:07 PDT
Created attachment 638916 [details] [diff] [review]
Removed sSnifferEntriesCount.

Carrying forward r+.
Comment 39 Paul Adenot (:padenot) 2012-07-09 15:40:57 PDT
Created attachment 640392 [details] [diff] [review]
Patch v1: rephrased comment.

Here is a better attempt at a comment.
Comment 40 Chris Pearce (:cpearce) 2012-07-09 15:59:03 PDT
Comment on attachment 640392 [details] [diff] [review]
Patch v1: rephrased comment.

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

This is identical to attachment 635989 [details] [diff] [review]. Did you forget to qref?

It would also be handy if you could label your patches "Patch 1 v2: Description" to make it clear the order in which they're applied, and to make it clearer which patch you're updating and how many times it's been revised.
Comment 41 Paul Adenot (:padenot) 2012-07-10 09:33:58 PDT
Created attachment 640638 [details] [diff] [review]
Avoid sniffing when recreating a channel. r=
Comment 42 Paul Adenot (:padenot) 2012-07-10 09:35:36 PDT
Comment on attachment 640638 [details] [diff] [review]
Avoid sniffing when recreating a channel. r=

Ah, I took the previous patch from my outdated gstreamer tree. Here is the right one.
Comment 43 Paul Adenot (:padenot) 2012-08-07 15:55:49 PDT
Created attachment 649848 [details] [diff] [review]
Sniff types of media files that are served with no Content-Type

(Adding r=cpearce to the commit message).
Comment 44 Paul Adenot (:padenot) 2012-08-07 15:56:13 PDT
Created attachment 649849 [details] [diff] [review]
Avoid sniffing when recreating a channel.

(Adding r=cpearce to the commit message).
Comment 45 Paul Adenot (:padenot) 2012-08-07 15:56:49 PDT
Created attachment 649850 [details] [diff] [review]
Sniff types of media files that are served with no Content-Type -- Tests

(Adding r=cpearce to the commit message).
Comment 46 Matthew Gregan [:kinetik] 2012-08-07 16:39:48 PDT
I was going to land this, but the following assertion is firing when running the content/media mochitests:

###!!! ASSERTION: When recreating a channel, we should know the Content-Type.: '!contentType.IsEmpty()', file /home/kinetik/mozilla-inbound/content/media/MediaResource.cpp, line 705

I saw two while test_streams_element_capture was running, but running that test individually doesn't cause the assertion to fire.
Comment 47 Paul Adenot (:padenot) 2012-08-07 18:04:59 PDT
Created attachment 649907 [details] [diff] [review]
Fix an assertion failure when the decoder was cloned. r=

Thanks, I could reproduce, and this patch fixes it.

The mistake was that I forgot to set the content-type when cloning the decoder
instead of using a channel to initialize it a new decoder and getting the
content-type at the same time.
Comment 48 Paul Adenot (:padenot) 2012-08-07 19:55:00 PDT
Created attachment 649938 [details] [diff] [review]
Fix an assertion failure when the decoder was cloned.
Comment 50 Peter Van der Beken [:peterv] 2012-08-08 03:29:23 PDT
Comment on attachment 649848 [details] [diff] [review]
Sniff types of media files that are served with no Content-Type

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

::: toolkit/components/mediasniffer/nsMediaSniffer.cpp
@@ +17,5 @@
> +static const unsigned MP4_MIN_BYTES_COUNT = 12;
> +// The maximum number of bytes to consider when attempting to sniff a file.
> +static const PRUint32 MAX_BYTES_SNIFFED = 512;
> +
> +NS_IMPL_CLASSINFO(nsMediaSniffer, NULL, 0, NS_MEDIA_SNIFFER_CID)

This is incomplete, if you really want to implement classinfo you need a GetInterfacesHelper and it needs to be returned from your QI, probably by changing NS_IMPL_ISUPPORTS1 to NS_IMPL_ISUPPORTS1_CI. But given that the classinfo is not even hooked up you probably don't need it.

As is this patch breaks my build with

Undefined symbols for architecture x86_64:
  "nsMediaSniffer_GetInterfacesHelper(unsigned int*, nsID***)", referenced from:
      knsMediaSnifferClassInfoData in nsMediaSniffer.o
Comment 51 Ed Morley [:emorley] 2012-08-08 05:13:35 PDT
Sorry backed out for causing bug 781141, the first time the new tests ran (given that we've only just got the Android tests really green, so do not wish to regress that). (Also comment 50):

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea9a4023efa
Comment 52 Paul Adenot (:padenot) 2012-08-09 16:05:35 PDT
I'm confused, the tests are passing pretty reliably on try, the only modification being that comment 50 has been addressed, and I added instrumentation, which does not want to show up because it's in a sjs file: 

https://tbpl.mozilla.org/?tree=Try&rev=1d1503e547d3
Comment 53 Ed Morley [:emorley] 2012-08-18 06:30:29 PDT
Sorry I didn't see your reply, since I don't CC on bugs when marking after merges/sheriffing/backouts, otherwise I get thousands of additional bugmails a month, 99.9% of which are not relevant and not easy to filter out.

This was backed out since one of the tests it added failed on it's very first run (as explained in bug 781141, mentioned in my backout comment). This failure is likely only intermittent, but we are toughening up on adding new intermittent failures, otherwise our efforts to get on top of the massive backlog of existing [oranges] is somewhat pointless if we add them as fast as fixing :-)
Comment 54 Paul Adenot (:padenot) 2012-09-04 16:18:25 PDT
I've rebased this and pushed to try [1], with an additional patch to address comment 50 remark.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=f54aef7bcadb
Comment 55 Paul Adenot (:padenot) 2012-09-04 16:24:06 PDT
Created attachment 658289 [details] [diff] [review]
Sniff types of media files that are served with no Content-Type

(rebased)
Comment 56 Paul Adenot (:padenot) 2012-09-04 16:24:26 PDT
Created attachment 658290 [details] [diff] [review]
Avoid sniffing when recreating a channel.
Comment 57 Paul Adenot (:padenot) 2012-09-04 16:24:49 PDT
Created attachment 658291 [details] [diff] [review]
Sniff types of media files that are served with no Content-Type -- Tests
Comment 58 Paul Adenot (:padenot) 2012-09-04 16:25:15 PDT
Created attachment 658292 [details] [diff] [review]
Fix an assertion failure when the decoder was cloned.
Comment 59 Paul Adenot (:padenot) 2012-09-04 16:29:43 PDT
Created attachment 658294 [details] [diff] [review]
Remove the classinfo stubs. r=
Comment 60 Paul Adenot (:padenot) 2012-09-04 16:31:24 PDT
Comment on attachment 658294 [details] [diff] [review]
Remove the classinfo stubs. r=

Not sure if this fixes the build for you, it worked with and without this patch for me.
Comment 61 Paul Adenot (:padenot) 2012-09-04 17:51:14 PDT
So I forgot to compile after rebasing and before pushing to try, [1] is the right try push.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=55171d7fece9
Comment 62 Paul Adenot (:padenot) 2012-09-05 12:27:59 PDT
Created attachment 658587 [details] [diff] [review]
Remove the classinfo stubs.

added r=peterv.
Comment 63 Paul Adenot (:padenot) 2012-09-05 12:30:25 PDT
Alright, this is green on try (see comment 60), I guess we can land, in that order, top patch first.
Comment 65 Paul Adenot (:padenot) 2012-09-05 15:36:14 PDT
Created attachment 658668 [details] [diff] [review]
Avoid sniffing when recreating a channel.

Forgot to upload this patch last time, that rebases over nsnull/nsCAutoString
changes.
Comment 66 Ryan VanderMeulen [:RyanVM] 2012-09-05 15:37:26 PDT
And backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c580e01ae0c
Comment 69 Masatoshi Kimura [:emk] 2012-12-21 03:25:04 PST
*** Bug 589564 has been marked as a duplicate of this bug. ***
Comment 70 Paul Adenot (:padenot) 2013-01-10 08:22:44 PST
*** Bug 546129 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.