Closed Bug 859711 Opened 11 years ago Closed 11 years ago

[DeviceStorage/Audio/Video] The Filter Types in DeviceStorage doesn't be all allowed by ExternalHelperAppService.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18+ verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 verified)

RESOLVED FIXED
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 + verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- verified

People

(Reporter: mchen, Assigned: mchen)

References

Details

Attachments

(5 files, 3 obsolete files)

The filter types in DeviceStorage is listed at
  http://mxr.mozilla.org/mozilla-central/source/toolkit/content/devicestorage.properties

The extention name to MineType is listed at 
  http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#387
  http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#455

Some types in DeviceStorage are not supported by ExternalHelperAppService yet, so these files will be listed into music app (app parses meta data succesfully then allow to play) but can't really to play by audio tag. Ex: m4r, m4p, m4a (in m-c but b2g18), ogx, aac.

So either we modify DeviceStorage to types we needs or add unsupported type into minetype list.
Oh, the owner of device storage would be Doug Tuner, so add Doug into cc list.
Hi Sandip,

Does any document list what audio/video extension name we suppport on b2g1.1 and b2g 1.0.1?
Then we can sync these between uriloader and devicestorage.

Thanks.
Flags: needinfo?(skamat)
I think nsMediaSniffer should be changed also to check more mimetype.
It returns "application/x-javascript-config" mimetype from those files, when I try to play the contents.
(So....MediaElement cannot create proper player)
blocking-b2g: --- → leo?
Hi Chris,

The list as below is the types filtered by DeviceStroage and some extension name are not supported by Gecko now. So may I know what is the supporting extension name from Gecko's point of view? Then we can decide remove them from filter type or add them into Gecko. Thanks.

Ex: m4r, m4p, m4a (in m-c but b2g18), ogx, aac are not supported by gecko now.

1 # Extensions we recognize for DeviceStorage storage areas
2 pictures=*.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp;
3 music=*.mp3; *.ogg; *.m4a; *.m4b; *.m4p; *.m4r; *.3gp; *.mp4; *.aac; *.m3u; *.pls; *.opus;
4 videos=*.mp4; *.mpeg; *.mpg; *.ogv; *.ogx; *.webm; *.3gp; *.ogg;
Flags: needinfo?(chris.double)
What's the user-facing result here?  Do we communicate the "this type is unsupported" or "can't be played" message?  If so, this would not be a blocker but a future enhancement/improvement.
Keywords: qawanted
The problem is that music list up some of those types but it cannot be played.
(The music application freeze)

Need to select one.
1. Do not list up those files that have problem.
2. Add new mimetype and support sniffing for those files.
Hi Leo,

Could you provide some sample file here for testing?

Thanks.
The problem of this attach file is that nsMediaSniffer [1] used a keyword "mp4" to match the field of "compatible_brands" in mp4 header and this will be a key to recognize whether the MimeType of this file is video/mp4 or not. 

But actually there can be three types of "compatible_brands" at least.
  1. ismo
  2. iso2
  3. mp41

So maybe we can fix it in nsMediaSniffer or [2] in m-c already add extension name of m4a into extraMimeEntries then we just asked for uplifting ti. 

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/mediasniffer/nsMediaSniffer.cpp#61

[2] http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#509
Attached audio test aac
Attached video test m4v
(In reply to Marco Chen [:mchen] from comment #9)
> Created attachment 741723 [details]
> M4A file can be listed but be playable
> 
> The problem of this attach file is that nsMediaSniffer [1] used a keyword
> "mp4" to match the field of "compatible_brands" in mp4 header and this will
> be a key to recognize whether the MimeType of this file is video/mp4 or not. 
> 
> But actually there can be three types of "compatible_brands" at least.
>   1. ismo
>   2. iso2
>   3. mp41
> 
> So maybe we can fix it in nsMediaSniffer or [2] in m-c already add extension
> name of m4a into extraMimeEntries then we just asked for uplifting ti. 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/
> mediasniffer/nsMediaSniffer.cpp#61
> 
> [2]
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#509

Right.
As I told in Bug 865114, m4a file problem is simply solved by add routine like this in MedisSniffer.

// The string "m4a".
if (aData[4*i]	 == 0x4D &&
    aData[4*i+1] == 0x34 &&
    aData[4*i+2] == 0x41) {
  return MATCH_M4A;
}

when it return MATCH_M4A, in GetMIMETypeFromContent()
if (res == MATCH_M4A) {
   aSniffedType.AssignLiteral(AUDIO_MP4);
   return NS_OK;
}

I already checked it's working.
But I'm not sure about other types.
(Other types are not listed up in Music app because of its metadata filtering)
(In reply to Marco Chen [:mchen] from comment #8)
> Hi Leo,
> 
> Could you provide some sample file here for testing?
> 
> Thanks.

I have three kind of test clip among them.
m4a is already attached by you and I uploaded other two.
Hi Leo,

I can't see attached aac or m4v files be listed in music app. And I found that they are filtered by music app. May I confirm with you?
The discussion above answers the qawanted request, although I'll note this was an inappropriate case for using qawanted - the reporter is a better person to ask in this case.
Keywords: qawanted
(In reply to Marco Chen [:mchen] from comment #14)
> Hi Leo,
> 
> I can't see attached aac or m4v files be listed in music app. And I found
> that they are filtered by music app. May I confirm with you?

Yes, right.
Music cannot parse other file format.

But..it's possible to enable it by adding sniff function on nsMediaSniffer.
Moreover, other mimetype like mkv, ts, k3g, etc that can be parsed by android extractor, also can be played.
I already tested it.

The coverage of extension is up to you.
I want just one rule.
The files listed up by app, should be played well if you don't want any notification popup like "Cannot play this contents".
blocking-b2g: leo? → tef?
Whiteboard: [tef-triage]
Our understanding is that this is a request to support new extensions which were not planned to be supported in leo. (see comment 5 for supported formats)
We will not block the release on the increased scope.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Flags: needinfo?(skamat)
Whiteboard: [tef-triage]
(In reply to Peter Dolanjski [:pdol] from comment #17)
> Our understanding is that this is a request to support new extensions which
> were not planned to be supported in leo. (see comment 5 for supported
> formats)
> We will not block the release on the increased scope.

leo triage disagrees with this analysis. m4a files to my understanding was in scope and I believe was working previously. And the end result is that the files are showing up being unable to be played. Renom.

I want the list of supported of formats by Sandip to be able to clarify this, cause there's missing information to make the call here.
blocking-b2g: - → tef?
Flags: needinfo?(skamat)
(In reply to Jason Smith [:jsmith] from comment #18)
> m4a files to my understanding was
> in scope and I believe was working previously. And the end result is that
> the files are showing up being unable to be played. Renom.

You're right, Jason.  I misread based on the comment about mkv, ts, k3g formats.
I'll let Sandip chime in on supported formats.  Thanks for catching.
Attached patch Support mp4 on isom&iso2 (obsolete) — Splinter Review
Attachment #744567 - Flags: review?(chris.double)
Comment on attachment 744567 [details] [diff] [review]
Support mp4 on isom&iso2

Paul's the media sniffer guru, I defer to him.
Attachment #744567 - Flags: review?(chris.double) → review?(paul)
Flags: needinfo?(chris.double)
(In reply to Marco Chen [:mchen] from comment #5)
> The list as below is the types filtered by DeviceStroage and some extension
> name are not supported by Gecko now. So may I know what is the supporting
> extension name from Gecko's point of view? Then we can decide remove them
> from filter type or add them into Gecko. Thanks.
> 
> Ex: m4r, m4p, m4a (in m-c but b2g18), ogx, aac are not supported by gecko
> now.
> 
> 1 # Extensions we recognize for DeviceStorage storage areas
> 2 pictures=*.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp;
> 3 music=*.mp3; *.ogg; *.m4a; *.m4b; *.m4p; *.m4r; *.3gp; *.mp4; *.aac;
> *.m3u; *.pls; *.opus;
> 4 videos=*.mp4; *.mpeg; *.mpg; *.ogv; *.ogx; *.webm; *.3gp; *.ogg;

I can only comment on the ones I was involved with supporting. .ogv, .webm, .ogg, mp4 (on some platforms), .mp3 (on some platforms), .wav, .oga.

I'm not aware of, or sure of, what image formats or other music formats it supports.
Comment on attachment 744567 [details] [diff] [review]
Support mp4 on isom&iso2

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

We should just implement the last version of the spec [1], which is more robust than randomly adding major_brands, and would fix the issue just as well.

[1]: http://mimesniff.spec.whatwg.org/#signature-for-mp4
Attachment #744567 - Flags: review?(paul) → review-
blocking-b2g: tef? → tef+
Seems like Marco's working on this so assigning to him.  Please de/re-assign if this is incorrect.
Assignee: nobody → mchen
(In reply to Paul Adenot (:padenot) from comment #23)
> Comment on attachment 744567 [details] [diff] [review]
> Support mp4 on isom&iso2
> 
> Review of attachment 744567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should just implement the last version of the spec [1], which is more
> robust than randomly adding major_brands, and would fix the issue just as
> well.
> 
> [1]: http://mimesniff.spec.whatwg.org/#signature-for-mp4

Hi Paul,

> We should just implement the last version of the spec [1], which is more robust than 
> randomly adding major_brands, and would fix the issue just as well.
I am not adding to filter major_brands but compatible_brand

1. I saw the code on nsMediaSniffer for mp4 is based on spec you said already.

2. Please refer to the attach file - "M4A file can be listed but be playable" and it's info is listed as below

   Major_verion: M4A
   Minor_version: null
   Compatible_Brand: isom & iso2.

   There is no any signature called mp4, so this file can not be recognized by sniffer now and that is why I post this patch.
Flags: needinfo?(paul)
On gaia side, the metadata.js from gaia/app/music/js used main_version in ftyp to recognize MP4 files. And it accepts M4A, M4B, mp42.

I think we should sync sniffer between Gaia & Gecko or there will be an issue of Gaia can list files in playlist but Gecko can't play it.
Hi Chris,

To info you again, and thanks for the reply in advance.

One of the attached file format from Leo is XXX.aac. As I knew we can support audio track with AAC encoding format on m4a extension name but we don't support it on aac extension name. Could you help to confirm this?
Flags: needinfo?(chris.double)
(In reply to Marco Chen [:mchen] from comment #27)
> Hi Chris,
> 
> To info you again, and thanks for the reply in advance.
> 
> One of the attached file format from Leo is XXX.aac. As I knew we can
> support audio track with AAC encoding format on m4a extension name but we
> don't support it on aac extension name. Could you help to confirm this?

As far as I know that is the case but I don't know what changes have been made by B2G to support formats specific to that platform.
Flags: needinfo?(chris.double)
Comment on attachment 744567 [details] [diff] [review]
Support mp4 on isom&iso2

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

Based on IRC discussion with mchen, we are landing this, but I filed bug 868333 to implement a more future proof solution.
Attachment #744567 - Flags: review- → review+
Flags: needinfo?(paul)
This is a patch for adding m4a extension name into extraMimeEntries for mapping to video/mp4. This is used when nsMediaSniffer can't recognize some kind of mp4 file.

Because this is added into m-c by bug 799315, here just extract what we need then porting to b2g18 branch.
Attachment #745076 - Flags: review?(paul)
Comment on attachment 745076 [details] [diff] [review]
Patch for b2g18 : Add m4a to extraMimeType

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

I guess your commit message should sqy "Add mapping of between m4a extension name and audio/mp4 in extraMimeEntries for b2g18 branch." instead of video/mp4.
Attachment #745076 - Flags: review?(paul) → review+
Add reviewer name and approval reason.
Attachment #744567 - Attachment is obsolete: true
Attachment #746193 - Flags: review+
Add reviewer name and approval reason.
Attachment #745076 - Attachment is obsolete: true
Attachment #746194 - Flags: review+
Lost to add the define of AUDIO_MP4 so add it into patch.
Attachment #746194 - Attachment is obsolete: true
Attachment #746262 - Flags: review+
Attachment #746262 - Attachment is patch: true
Hi Leo,

1. The patch here will solve the issue on m4a file.
2. FxOS doesn't plan to support aac extension file name.
3. m4v support will be added by https://bugzilla.mozilla.org/show_bug.cgi?id=847006#c19

So after landing the patch, this bug will be closed.
Flags: needinfo?(leo.bugzilla.gecko)
Keywords: checkin-needed
Given that we're heading to land, not going to worry about the needinfo request anymore.
Flags: needinfo?(skamat)
https://hg.mozilla.org/mozilla-central/rev/a4b5223f39b7
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: needinfo?(leo.bugzilla.gecko)
This bug is no longer reproduces
As per comment 36
The attached mp4a file in visible and playable in "Music" app

Environmental  Variables:
Inari Build ID: 20130531070208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/be5c2ee11d02
Gaia: e7114bdf4078274fc127a3b2a58dad91d6884219

Environmental  Variables:
Unagi Build ID: 20130531070205
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4f318822e72c
Gaia: e1c59baed29e4665d1da9392f86239272073f07a
You need to log in before you can comment on or make changes to this bug.