Closed Bug 868333 Opened 7 years ago Closed 5 years ago

Improve mp4 sniffing

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: padenot, Assigned: jhlin)

References

Details

Attachments

(1 file, 4 obsolete files)

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
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?
To add Dominic into this discussion. He is the owner on Gaia music app.
I won't have time to work on this at the moment, and I don't expect to have time in the near future.
Assignee: padenot → nobody
Blocks: 1171751
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
Assignee: nobody → jolin
- add m4a in MIME type list.
- stop matching 'iso?' and match 'M4A' and '3gp?' instead.
Attachment #8622380 - Flags: review?(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 ?
Attachment #8622380 - Flags: review?(padenot) → review?(cpearce)
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.
Attachment #8622380 - Flags: review?(cpearce) → review?(giles)
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.
Attachment #8622380 - Flags: review?(giles)
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.
Attachment #8622380 - Attachment is obsolete: true
Attachment #8625646 - Flags: review?(giles)
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.
Attachment #8625646 - Flags: review?(giles) → review+
carry r+: rillian
Attachment #8625646 - Attachment is obsolete: true
Attachment #8625894 - Flags: review+
Attachment #8626325 - Attachment is obsolete: true
Attachment #8626326 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ac775c9f8ed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1211802
Depends on: 1269260
Depends on: 1272468
You need to log in before you can comment on or make changes to this bug.