868333 - Improve mp4 sniffing

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

Two things to do here:
- Update our mp4 sniffing algorithm so it matches the spec's [1]
- Investigate why it fails on certain files [2], that don't even have the "mp4" string. Are these file valid, and common?

[1]: http://mimesniff.spec.whatwg.org/#signature-for-mp4
[2]: File attached on bug https://bugzilla.mozilla.org/show_bug.cgi?id=859711

Comment 1

[Marco Chen [:mchen]]

Hi Paul,

Currently we set MimeType to video/mp4 for content mapped to mp4 signature in nsMediaSniffer.cpp. But for m4a and m4r, is it better to set as audio/mp4?

Comment 2

[Marco Chen [:mchen]]

To add Dominic into this discussion. He is the owner on Gaia music app.

Comment 3

[Paul Adenot (:padenot)]

I won't have time to work on this at the moment, and I don't expect to have time in the near future.

Comment 4

[John Lin [:jhlin][:jolin]]

Plan to ignore 'isom/iso2' when matching MP4 and add another matching function to map 'M4A' to 'audio/mp4' according to [1].

[1] http://www.ftyps.com/index.html

Comment 5

[John Lin [:jhlin][:jolin]]

Attachment: bug868333-more-mp4-file-types.patch

- add m4a in MIME type list.
- stop matching 'iso?' and match 'M4A' and '3gp?' instead.

Comment 6

[John Lin [:jhlin][:jolin]]

Treeherder result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=903870c7fba3

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8622380 [details] [diff] [review]
bug868333-more-mp4-file-types.patch

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

I have to admit I don't know what makes sense to do here anymore, I've been out of this for quite a while.

Chris, would you mind having a look ?

Comment 8

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

Comment on attachment 8622380 [details] [diff] [review]
bug868333-more-mp4-file-types.patch

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

I will bump this to Ralph... Ralph feel free to bump it on if you need to.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8622380 [details] [diff] [review]
bug868333-more-mp4-file-types.patch

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

Approach is fine but I'm not satisfied with some of the details. Please address comments and let me see it again.

Have you talked to the spec authors about this extension?

::: netwerk/mime/nsMimeTypes.h
@@ +81,5 @@
>  #define AUDIO_MP4                           "audio/mp4"
>  #define AUDIO_AMR                           "audio/amr"
>  #define AUDIO_FLAC                          "audio/flac"
>  #define AUDIO_3GPP                          "audio/3gpp"
> +#define AUDIO_M4A                           "audio/x-m4a"

Do we need to use the x- version here? https://tools.ietf.org/html/rfc6381 refers to audio/mp4 for any mpeg-1,2,4 audio codecs in the mp4 encapsulation.

::: toolkit/components/mediasniffer/nsMediaSniffer.cpp
@@ +41,5 @@
> +// For a complete list of file types, see http://www.ftyps.com/index.html
> +nsMediaSnifferEntry sFtypEntries[] = {
> +  PATTERN_ENTRY("\xFF\xFF\xFF", "mp4", VIDEO_MP4), // Could be mp41 or mp42.
> +  PATTERN_ENTRY("\xFF\xFF\xFF", "3gp", VIDEO_3GPP), // Could be 3gp4, 3gp5, ...
> +  PATTERN_ENTRY("\xFF\xFF\xFF", "M4A", AUDIO_M4A)

Checking against "M4A " would be more robust if there's no version number.

Do we need to handle M4P?

@@ +47,2 @@
>  
> +static bool MatchesBrands(const uint8_t aData[4], const char** aMimeType)

nsACString& aMimeType

You're returning pointers to static strings here, so you could use AssignLiteral and avoid the copy, but there's no way to enforce that through to the caller. Better to pass through a reference to the actual output string and call Assign() or AssignLiteral as appropriate within your new code.

@@ +47,4 @@
>  
> +static bool MatchesBrands(const uint8_t aData[4], const char** aMimeType)
> +{
> +  for (uint32_t i = 0; i < mozilla::ArrayLength(sFtypEntries); ++i) {

ArrayLength returns a size_t. Use that for i to avoid overflow. Sadly 'auto' isn't smart enough here.

@@ +49,5 @@
> +{
> +  for (uint32_t i = 0; i < mozilla::ArrayLength(sFtypEntries); ++i) {
> +    const auto& currentEntry = sFtypEntries[i];
> +    bool matched = true;
> +    for (uint32_t j = 0; j < currentEntry.mLength; ++j) {

MOZ_ASSERT(currentEntry.mLength <= mozilla::ArrayLength(aData),
           "Pattern is too large to match brand strings.");

::: toolkit/components/mediasniffer/nsMediaSniffer.h
@@ +17,5 @@
>  
>  // ed905ba3-c656-480e-934e-6bc35bd36aff
>  #define NS_MEDIA_SNIFFER_CID \
>  {0x3fdd6c28, 0x5b87, 0x4e3e, \
>  {0x8b, 0x57, 0x8e, 0x83, 0xc2, 0x3c, 0x1a, 0x6d}}

Do you need to update the contract id with the code change? Not sure if it's behaviour or just interface.

Comment 10

[John Lin [:jhlin][:jolin]]

Attachment: Support more MP4 family file types in media sniffer.

Address review comments:

(In reply to Ralph Giles (:rillian) from comment #9)
> Comment on attachment 8622380 [details] [diff] [review]
> bug868333-more-mp4-file-types.patch
> 
> Review of attachment 8622380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Approach is fine but I'm not satisfied with some of the details. Please
> address comments and let me see it again.
> 
> Have you talked to the spec authors about this extension?
> 
> ::: netwerk/mime/nsMimeTypes.h
> @@ +81,5 @@
> >  #define AUDIO_MP4                           "audio/mp4"
> >  #define AUDIO_AMR                           "audio/amr"
> >  #define AUDIO_FLAC                          "audio/flac"
> >  #define AUDIO_3GPP                          "audio/3gpp"
> > +#define AUDIO_M4A                           "audio/x-m4a"
> 
> Do we need to use the x- version here? https://tools.ietf.org/html/rfc6381
> refers to audio/mp4 for any mpeg-1,2,4 audio codecs in the mp4 encapsulation.
> 
> ::: toolkit/components/mediasniffer/nsMediaSniffer.cpp
> @@ +41,5 @@
> > +// For a complete list of file types, see http://www.ftyps.com/index.html
> > +nsMediaSnifferEntry sFtypEntries[] = {
> > +  PATTERN_ENTRY("\xFF\xFF\xFF", "mp4", VIDEO_MP4), // Could be mp41 or mp42.
> > +  PATTERN_ENTRY("\xFF\xFF\xFF", "3gp", VIDEO_3GPP), // Could be 3gp4, 3gp5, ...
> > +  PATTERN_ENTRY("\xFF\xFF\xFF", "M4A", AUDIO_M4A)
> 
> Checking against "M4A " would be more robust if there's no version number.
> 
> Do we need to handle M4P?
> 

 Okay. Output 'audio/mp4' for 'M4A ' and 'M4P ' rather than 'audio/x-m4a'.

> @@ +47,2 @@
> >  
> > +static bool MatchesBrands(const uint8_t aData[4], const char** aMimeType)
> 
> nsACString& aMimeType
> 
> You're returning pointers to static strings here, so you could use
> AssignLiteral and avoid the copy, but there's no way to enforce that through
> to the caller. Better to pass through a reference to the actual output
> string and call Assign() or AssignLiteral as appropriate within your new
> code.

 Done. Pass output string to matching function and assign value when matched.

> 
> @@ +47,4 @@
> >  
> > +static bool MatchesBrands(const uint8_t aData[4], const char** aMimeType)
> > +{
> > +  for (uint32_t i = 0; i < mozilla::ArrayLength(sFtypEntries); ++i) {
> 
> ArrayLength returns a size_t. Use that for i to avoid overflow. Sadly 'auto'
> isn't smart enough here.
> 

 Fixed.

> @@ +49,5 @@
> > +{
> > +  for (uint32_t i = 0; i < mozilla::ArrayLength(sFtypEntries); ++i) {
> > +    const auto& currentEntry = sFtypEntries[i];
> > +    bool matched = true;
> > +    for (uint32_t j = 0; j < currentEntry.mLength; ++j) {
> 
> MOZ_ASSERT(currentEntry.mLength <= mozilla::ArrayLength(aData),
>            "Pattern is too large to match brand strings.");
> 

 Done.

> ::: toolkit/components/mediasniffer/nsMediaSniffer.h
> @@ +17,5 @@
> >  
> >  // ed905ba3-c656-480e-934e-6bc35bd36aff
> >  #define NS_MEDIA_SNIFFER_CID \
> >  {0x3fdd6c28, 0x5b87, 0x4e3e, \
> >  {0x8b, 0x57, 0x8e, 0x83, 0xc2, 0x3c, 0x1a, 0x6d}}
> 
> Do you need to update the contract id with the code change? Not sure if it's
> behaviour or just interface.

 It seems okay because CID was never changed in previous patches.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8625646 [details] [diff] [review]
Support more MP4 family file types in media sniffer.

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

Looks good now. Thanks for revising. r=me

::: toolkit/components/mediasniffer/nsMediaSniffer.cpp
@@ +59,5 @@
> +        break;
> +      }
> +    }
> +    if (matched) {
> +      aSniffedType.AssignASCII(currentEntry.mContentType);

I think you can AssignLiteral here like the old code, assuming the mContentType are static strings. I don't know how to static_assert that though, so copying is also fine.

Comment 12

[John Lin [:jhlin][:jolin]]

Attachment: Support more MP4 family file types in media sniffer.

carry r+: rillian

Comment 13

[John Lin [:jhlin][:jolin]]

Attachment: Support more MP4 family file types in media sniffer.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e86670c8a30

Comment 14

[John Lin [:jhlin][:jolin]]

Attachment: Support more MP4 family file types in media sniffer.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac775c9f8ed

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ac775c9f8ed