Last Comment Bug 793372 - [DeviceStorage]: recognize 3gp videos and m3u and pls playlists
: [DeviceStorage]: recognize 3gp videos and m3u and pls playlists
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: David Flanagan [:djf]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 796818
  Show dependency treegraph
 
Reported: 2012-09-22 00:10 PDT by David Flanagan [:djf]
Modified: 2012-11-05 16:29 PST (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
patch (802 bytes, patch)
2012-09-22 00:33 PDT, David Flanagan [:djf]
doug.turner: review+
Details | Diff | Splinter Review
revised patch that also adds .pls files (805 bytes, patch)
2012-09-24 21:31 PDT, David Flanagan [:djf]
no flags Details | Diff | Splinter Review
patch version 3 (1.05 KB, patch)
2012-09-25 11:26 PDT, David Flanagan [:djf]
no flags Details | Diff | Splinter Review
patch version 4 (944 bytes, patch)
2012-09-26 16:26 PDT, David Flanagan [:djf]
no flags Details | Diff | Splinter Review
recognize 3gp videos and m3u and pls playlists (1.08 KB, patch)
2012-10-11 15:49 PDT, David Flanagan [:djf]
doug.turner: review+
bajaj.bhavana: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description David Flanagan [:djf] 2012-09-22 00:10:39 PDT
In order to support playlists in the Gaia music app, the app needs to be able to read playlist files like .m3u (http://en.wikipedia.org/wiki/M3U) files. So we need to add that extension to the list for the "music" type.
Comment 1 David Flanagan [:djf] 2012-09-22 00:33:02 PDT
Created attachment 663656 [details] [diff] [review]
patch

Doug,

The attached patch adds the .m3u extension to devicestorage.properties.

Is that enough to get .m3u files returned? How do they get a mime type mapped to them? Where is that handled? I'm not sure I care about that, though. For the Gaia music app I'm okay just looking at the file extension.
Comment 2 Doug Turner (:dougt) 2012-09-23 17:02:22 PDT
Comment on attachment 663656 [details] [diff] [review]
patch

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

This patch allows device storage to return m3u files.  it doesn't not allow gecko to play them, or support them natively.

If you want to map mime types to file extensions, that would be a different bug.  Dave Hylands has some experience with that.
Comment 3 David Flanagan [:djf] 2012-09-24 21:31:17 PDT
Created attachment 664353 [details] [diff] [review]
revised patch that also adds .pls files

Sorry Doug, but could you review it again. I've just added one more playlist file extension.

Also, I just noticed that the line for pictures ends with semicolon but the line for movies does not.  I have a terminate semicolon for music now.  Is that harmless?
Comment 4 Doug Turner (:dougt) 2012-09-25 09:18:56 PDT
Comment on attachment 664353 [details] [diff] [review]
revised patch that also adds .pls files

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

could you add a semi after the videos line?
Comment 5 David Flanagan [:djf] 2012-09-25 11:26:32 PDT
Created attachment 664588 [details] [diff] [review]
patch version 3

This version actually removes the trailing semicolons, based on the example in filepicker.properties

And in addition to adding .pls to music, it also adds 3gp to photos since the camera will store videos of that type in the same directory as pictures.
Comment 6 David Flanagan [:djf] 2012-09-25 12:23:52 PDT
For what its worth, I built gecko with these changes (comments, removed trailing semicolons) and the Gaia media apps still work, so it doesn't look like I broke anything. 

I'll remove the .3gp in order to land the playlist changes if that would make it quicker.
Comment 7 David Flanagan [:djf] 2012-09-26 16:26:38 PDT
Created attachment 665172 [details] [diff] [review]
patch version 4

I lost the argument about video files in pictures type, so I've taken that out.

The patch now does these things:

1) Adds a comment at the top of the file
2) Removes trailing semicolons
3) Adds *.m3u and *.pls playlist extensions to the music types
Comment 8 David Flanagan [:djf] 2012-09-26 16:28:29 PDT
Version 3 of the patch is obsolete, but I forgot to mark it as obsolete when I attached the new version, and now I can't figure out how to do that.
Comment 9 David Flanagan [:djf] 2012-10-11 15:49:26 PDT
Created attachment 670579 [details] [diff] [review]
recognize 3gp videos and m3u and pls playlists

Let's get this reviewed and landed ASAP. Its now blocking playback of videos recorded by the camera!
Comment 10 David Flanagan [:djf] 2012-10-11 15:50:47 PDT
changing the bug title to reflect the new main point, which is getting recorded videos to play.
Comment 11 David Flanagan [:djf] 2012-10-11 15:55:21 PDT
nominating blocking, since we need this to play videos from the camera
Comment 12 Doug Turner (:dougt) 2012-10-11 20:48:33 PDT
Comment on attachment 670579 [details] [diff] [review]
recognize 3gp videos and m3u and pls playlists

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

::: toolkit/content/devicestorage.properties
@@ +1,4 @@
> +# Extensions we recognize for DeviceStorage storage areas
> +pictures=*.jpe; *.jpg; *.jpeg; *.gif; *.png; *.bmp
> +music=*.mp3; *.ogg; *.m4a; *.m4b; *.m4p; *.m4v; *.m4r; *.3gp; *.mp4; *.aac; *.m3u; *.pls
> +videos=*.avi; *.divx; *.flv; *.m4v; *.mkv; *.mov; *.mp4; *.mpeg; *.mpg; *.ogm; *.ogv; *.ogx; *.rm; *.rmvb; *.smil; *.webm; *.wmv; *.xvid; *.3gp

go ahead and add trailing semicolons to each line.
Comment 13 David Flanagan [:djf] 2012-10-12 00:40:51 PDT
Doug,

I think the semicolons are separators, not terminators.  filepicker.properties does not have them, which is why I removed them here. (See comments 5 & 6).

So I'm going to ignore that review comment and request checkin.
Comment 14 David Flanagan [:djf] 2012-10-12 00:54:12 PDT
Chris,

this patch is ready to land, if you want to shepard it through
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-10-13 05:50:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56bbbca15e5
https://hg.mozilla.org/releases/mozilla-aurora/rev/2875cacede43

(Leaving open until this lands on m-c)
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-10-13 16:42:41 PDT
https://hg.mozilla.org/mozilla-central/rev/e56bbbca15e5
Comment 17 David Scravaglieri [:scravag] 2012-11-05 13:14:58 PST
Comment on attachment 670579 [details] [diff] [review]
recognize 3gp videos and m3u and pls playlists

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Comment 18 David Flanagan [:djf] 2012-11-05 14:28:16 PST
David,

This landed 3+ weeks ago. We don't need the approval flag now, do we?
Comment 19 bhavana bajaj [:bajaj] 2012-11-05 16:28:47 PST
(In reply to David Flanagan [:djf] from comment #18)
> David,
> 
> This landed 3+ weeks ago. We don't need the approval flag now, do we?

Comment 15 suggests this patch has already landed on aurora , hence you would not need the approval flag at this time
Comment 20 bhavana bajaj [:bajaj] 2012-11-05 16:29:14 PST
Comment on attachment 670579 [details] [diff] [review]
recognize 3gp videos and m3u and pls playlists

Already landed in aurora

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