907929 - [music] Support album art in Vorbis comments (ogg files)

Status: RESOLVED FIXED

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

Description

[Jim Porter (:squib)]

Right now, we don't support album art embedded in Vorbis comments, and there's a comment in the metadata parser that expresses some confusion as to how it would be handled. While I'm not certain of the exact implementation, we can see what other media players do and then follow along with that. This should be pretty straightforward once we take a look at some sample files.

Comment 1

[Hubert Figuiere [:hub]]

I'll take it.

Comment 2

[Jim Porter (:squib)]

I'd recommend working on this a bit later, actually. I haven't had a chance to yet, but I'm going to write up a message about "phase 2" of the metadata improvements. Like with phase 1 (bug 841949), I'd like to land all the improvements in one go so that we don't force people to sit through the music app reparsing all their files. The main things I want for phase 2 are: vorbis album art (this bug), album artist, proper support for multi-value fields (i.e. store them as an array in the DB), genres, and probably ReplayGain support.

I don't think we should try to do this in phase 1, since we should try to land bug 841949 sooner, rather than later.

Comment 3

[Jim Porter (:squib)]

Sorry, to be clearer: I foresee two times people will have to deal with a full reparse of their music collection: bug 841949, and the "phase 2" bug, which I haven't filed yet.

Comment 4

[Hubert Figuiere [:hub]]

Bug 1065858 is phase 2.

Comment 7

[Hubert Figuiere [:hub]]

Also we'll do Flac at the same time.

Comment 8

[Hubert Figuiere [:hub]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26660

If we can get FLAC support in first, I'll port it over to be used by FLAC too.

Comment 9

[Hubert Figuiere [:hub]]

Also I haven't tested Opus.

Comment 10

[Jim Porter (:squib)]

Comment on attachment 8534141 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26660

This looks mostly-correct, but I have a few comments on GitHub that should be addressed. It also looks like tests aren't being run, since they're commented out?

Comment 11

[Hubert Figuiere [:hub]]

Comment on attachment 8534141 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26660

re-requesting review.

Removed FLAC support as it is broken at the source: the sample I made doesn't conform to VorbisComment. That was the test not run.

Addressed comments in Ogg support. This should be good to go if you feel it is.

Comment 12

[Hubert Figuiere [:hub]]

Yep, my sample file actually uses a separate metadata block for the cover art. So it is out of scope for this bug. See bug 1118021

Comment 13

[Hubert Figuiere [:hub]]

On the other hand we reuse the binary parsing :-D

Comment 14

[Jim Porter (:squib)]

Comment on attachment 8534141 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26660

This looks mostly good, but the changes to the test utilities concern me. Currently, the unit tests are testing the case for blobs that have no filename (e.g. things from the open activity). I think this is what we should be testing here, since that's easier to accidentally regress (QA is less likely to check that album art works from an open activity than for the main app).

I plan on testing external artwork for "real" files, but that'll go somewhere else, I think. Look for a few bugs to be filed on improving the unit tests; I've already got some small patches in progress.

There are a couple other minor comments on GitHub too.

Comment 15

[Hubert Figuiere [:hub]]

Comment on attachment 8534141 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26660

addressed comments again

Comment 16

[Jim Porter (:squib)]

Comment on attachment 8534141 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26660

r=me with two tiny editorial changes. Thanks!

Comment 17

[Hubert Figuiere [:hub]]

Merged

https://github.com/mozilla-b2g/gaia/commit/e3b73d0ad66eda4948dcd3657b8f5411e8678d7b

Comment 18

[Hubert Figuiere [:hub]]

Quick note about Opus: I need to find real world sample / use case to determine its status. Will file bugs accordingly.