Closed Bug 1093219 Opened 5 years ago Closed 5 years ago

[Music] m3u parser for playlist

Categories

(Firefox OS Graveyard :: Gaia::Music, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S1 (5dec)

People

(Reporter: hub, Assigned: hub)

References

Details

(Whiteboard: [ft:media] [2.2 target])

Attachments

(1 file)

We need to write a m3u parser (read and write) for the playlist support.
Assignee: nobody → hub
Status: NEW → ASSIGNED
Current work in progress. Basic parsing.
Target Milestone: --- → 2.1 S9 (21Nov)
Attachment #8519018 - Flags: review?(dflanagan)
Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

I've left a lot of comments on github. This needs more work, and more thought about how it is going to integrate with the rest of the music app. For example, is there actually any need to parse the EXTINF comments in an m3u file, or can we just ignore those?
Attachment #8519018 - Flags: review?(dflanagan) → review-
Whiteboard: [ft:media] [2.2 target]
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

David,

I think I addressed all the issues. Including simplifying the model to only keep what we need.

Notable change I always write the #EXTM3U magic at the beginning.

Tests have been updated.

Thank you kindly.
Attachment #8519018 - Flags: review- → review?(dflanagan)
Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

This is a lot better than it was. The parse() function is broken, however, and I've left various other comments on github.

So one more round of changes, then we should start thinking about how to integrate this with the metadata parsing code and how to handle playlists in the app once they've been parsed. How will be query the mediadb for a list of all the playlists it knows about, for example.

RTL fixes should probably come first, though.
Attachment #8519018 - Flags: review?(dflanagan) → review-
Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

So I updated the PR with all the comments. Added some more test.

I'll wait for confirmation of Gaia-try, but we should be alright with the tests in CI.

(I'll squash the commits into one when ready to land)
Attachment #8519018 - Flags: review- → review?(dflanagan)
Comment on attachment 8519018 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25941

The code looks okay except that the test playlist file is missing from the PR. I'm guessing you just forgot to 'git add' that new file.

Does it make sense to land this playlist data structure and parser without hooking it up to the metadata parser? If you want to get it landed (once you add the missing test file) that is okay with me.

It might make more sense, though, to integrate it with the metadata parser before landing so it is not just sitting unused in the music app.
Attachment #8519018 - Flags: review?(dflanagan) → review+
As I commented on github, the file is not needed as the mock for the device storage API feed the bytes to the reader. Maybe this makes a hole in the test suite.

As for landing it, since we are positive to have the feature this cycle, it will be used. Therefore I don't see the need to hold it back. Surely there will be changes down the line as we use it in the app.
Merged
https://github.com/mozilla-b2g/gaia/commit/1fff49c664f905f11a86426a9835e6df6b58e825
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.