Closed Bug 1071433 Opened 5 years ago Closed 5 years ago

[gallery]share with ringtone is seen in gallery

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ankit93040, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(Whiteboard: [g+][LibGLA, Dev, B] )

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

Gallery -> share -> ringtones


Actual results:

ringtone is seen in share list


Expected results:

share shouldn't show ringtone app
Whiteboard: [g+][LibGLA, Dev, B]
Flags: needinfo?(pdahiya)
Debugged and the issue here is in gallery and video app, file type for 3gp video now shows as audio/3gpp making ringtones share activity show up for file types audio/*. 

It should be video/3gpp, looks like regression as I am not seeing this issue on my hamachi build id 20140905040204.
Flags: needinfo?(pdahiya)
Investigated and it appears file type returned from device storage for 3gp video is coming as audio/3gpp instead of video/3gpp

Gallery-> Open video -> Click Info
https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/info.js#L13
https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/gallery.js#L249

Setting need info flag for dhylands to help debug this issue at gecko level in device storage APIs. Thanks
Flags: needinfo?(dhylands)
Yeah - I think that this is a known issue.

The problem is that device storage is based purely on the extension, and .3gp files can be both video and/or audio.

And Device Storage has no way to tell which one it is.

Perhaps, if we're enumerating 'music' then it should return the audio tag, and if we're enumerating 'videos', it should return the video tag?

If we enumerate 'sdcard', it should probably return video (since that's the more complicated one) and leave it to the app to query the metadata to figure out if it really is video or not?
Flags: needinfo?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #3)
> Yeah - I think that this is a known issue.
> 
> The problem is that device storage is based purely on the extension, and
> .3gp files can be both video and/or audio.
> 
> And Device Storage has no way to tell which one it is.
> 
> Perhaps, if we're enumerating 'music' then it should return the audio tag,
> and if we're enumerating 'videos', it should return the video tag?
> 
When you say enumerating, is it enumerating mediaDb or device storage. I assume you mean updating device storage get call to return correct file type. Please correct if I am wrong.

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/info.js#L13

I see BuildID:20140905040204 , hamachi returning video/3gpp as file types for all our camera recorded videos, is there a recent change in gecko device storage because of which file type for caemra recorded videos has started showing up as audio/3gpp.


> If we enumerate 'sdcard', it should probably return video (since that's the
> more complicated one) and leave it to the app to query the metadata to
> figure out if it really is video or not?
Flags: needinfo?(dhylands)
In ds-test an empty file named foo.3gp shows up as mime-type audio/3gpp

I was referring to enumerating in device storage.

I was suggesting that perhaps storage.enumerate or storage.get when storage comes from navigator.getDeviceStorage('music') should return audio/3gpp for *.3gp files, and that navigator.getDeviceStorage('videos') should return video/3gpp for the same files.

DeviceStorageTypeChecker::GetTypeFromFileName checks Video before Music:
http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp?from=GetTypeFromFileName#356

But it looks like DeviceStorageFile::CalculateMimeType calls mimeService->GetTypeFromFile.

It looks like this table is being used:
http://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#552,561

Looking back in time, it looks like bug 1064127 added the audio entry for *.3gp files.
Flags: needinfo?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #5)
> In ds-test an empty file named foo.3gp shows up as mime-type audio/3gpp
> 
> I was referring to enumerating in device storage.
> 
> I was suggesting that perhaps storage.enumerate or storage.get when storage
> comes from navigator.getDeviceStorage('music') should return audio/3gpp for
> *.3gp files, and that navigator.getDeviceStorage('videos') should return
> video/3gpp for the same files.
> 
> DeviceStorageTypeChecker::GetTypeFromFileName checks Video before Music:
> http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/
> nsDeviceStorage.cpp?from=GetTypeFromFileName#356
> 
> But it looks like DeviceStorageFile::CalculateMimeType calls
> mimeService->GetTypeFromFile.
> 
> It looks like this table is being used:
> http://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#552,561
> 
> Looking back in time, it looks like bug 1064127 added the audio entry for
> *.3gp files.

Thanks Dave, its very helpful. I see https://bugzilla.mozilla.org/show_bug.cgi?id=1064127#c11 is suggesting a fix in gecko. Setting NI flag for Makoto Kato as suggested gecko fix should fix this bug in gallery app.
Flags: needinfo?(mkato)
I should remove file ext for auto/3gpp or modify order for it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mkato)
Assignee: nobody → m_kato
Blocks: 1064127
Component: General → Video/Audio
Product: Firefox OS → Core
Attached patch Remove audio/3gpp associates (obsolete) — Splinter Review
This is a regression by bug 1064127.

If file extension is 3gp, gallery apps detect to audio/3gpp incorrectly.   Since audio/3gpp support is only certified app, it is unnecessary for association.
Attachment #8496707 - Flags: review?(roc)
Comment on attachment 8496707 [details] [diff] [review]
Remove audio/3gpp associates

sorry, I need test
Attachment #8496707 - Flags: review?(roc)
ah, I should change order for this lists.
Attachment #8496707 - Attachment is obsolete: true
Comment on attachment 8496713 [details] [diff] [review]
Change order of audio/3gpp

This is a regression by bug 1064127.

Change order of audio/3gpp for Gallery apps.  3gp file should detects as video/3gpp for apps.
Attachment #8496713 - Flags: review?(roc)
Comment on attachment 8496713 [details] [diff] [review]
Change order of audio/3gpp

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

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +559,5 @@
>    { AUDIO_WAV, "wav", "Waveform Audio" },
>    { VIDEO_3GPP, "3gpp,3gp", "3GPP Video" },
>    { VIDEO_3GPP2,"3g2", "3GPP2 Video" },
> +#ifdef MOZ_WIDGET_GONK
> +  // AUDIO_3GPP has to set after VIDEO_3GPP for Gallery apps

This should explain in more detail why the order has to be this way.

Also, I think I'm not the right reviewer for this code.
Attachment #8496713 - Flags: review?(roc) → review-
Hello Makoto, Could you please kindly share if any update from your side ? 
Thank you very much !
Flags: needinfo?(mkato)
Flags: needinfo?(m_kato)
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #14)
> Hello Makoto, Could you please kindly share if any update from your side ? 
> Thank you very much !

Could you explain why 3gp file detects audio/3gpp?  I need more dtail by comment #13.
Dones't GAIA analyze file data that 3gp file is audio or video?
Flags: needinfo?(ryang)
Flags: needinfo?(mkato)
Flags: needinfo?(m_kato)
To set review again, I want to need GAIA team comment.

I think that I may change order or remove audio/3gp entry from uriloader since audio/3gpp supports certified app only.
Hi Gary, 

Could you please kindly provide your comments per comment 15 and feedback on the patch ? 
Thank you very much!
Flags: needinfo?(ryang) → needinfo?(gchen)
Hi Rachelle,
   Sorry late for reply, actually I am not right person either.
   Please refer to below MDN page, maybe we can find someone who is in `docshell` component.
   https://wiki.mozilla.org/Modules/Core#docshell
Flags: needinfo?(gchen)
Hi Boris, would you mind helping review the patch in comment 11& comment12 ?
Thank you very much !
Flags: needinfo?(bzbarsky)
What is there to review?  The patch is clearly correct if we want those extensions to map to the video MIME type, but needs the comments roc asked for in comment 13.  I guess you can consider me as delegating the review to roc, since he's already done it.

But more generally, this system sounds kind of broken: we shouldn't be deciding stuff based on the extension if the extension is known to map to two different kinds of things.  So we can do the patch here as a quick fix, but presumably we need a separate bug for a longer-term and more correct fix in whatever code actually cares about the difference and can tell the two types of files apart.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #20)
> What is there to review?  The patch is clearly correct if we want those
> extensions to map to the video MIME type, but needs the comments roc asked
> for in comment 13.  I guess you can consider me as delegating the review to
> roc, since he's already done it.

Thanks, bz.  I will send a review for quick fix since gaia needs fix.
 
> But more generally, this system sounds kind of broken: we shouldn't be
> deciding stuff based on the extension if the extension is known to map to
> two different kinds of things.  So we can do the patch here as a quick fix,
> but presumably we need a separate bug for a longer-term and more correct fix
> in whatever code actually cares about the difference and can tell the two
> types of files apart.

I will file a new bug for it.
Attached patch quick fixSplinter Review
Comment on attachment 8506061 [details] [diff] [review]
quick fix

quick fix per comment #20
Attachment #8506061 - Flags: review?(bzbarsky)
Blocks: 1083728
Comment on attachment 8506061 [details] [diff] [review]
quick fix

That comment is ... borderline useless, sorry.  ;)

How about:

  The AUDIO_3GPP has to come after the VIDEO_3GPP entry because the Gallery app
  on Firefox OS depends on the "3gp" extension mapping to the "video/3gpp" MIME
  type.

Assuming that's what the actual dependency is of course; please adjust if it's a dependency on the other extension, say.

r=me with that fixed.
Attachment #8506061 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/03f1d37d1fb1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.