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

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::Music
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: squib, Assigned: hub)

Tracking

(Blocks: 1 bug)

unspecified
2.2 S3 (9jan)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 wontfix, b2g-v2.1 wontfix, b2g-v2.2 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

4 years ago
I'll take it.
Assignee: nobody → hub
Depends on: 841949
(Reporter)

Comment 2

4 years ago
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.
(Reporter)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Bug 1065858 is phase 2.
(Assignee)

Updated

4 years ago
Blocks: 1065858
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1093232
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1102603
(Assignee)

Updated

3 years ago
Summary: [music] Support album art in Vorbis comments → [music] Support album art in Vorbis comments (ogg and opus files)
(Assignee)

Comment 7

3 years ago
Also we'll do Flac at the same time.
Depends on: 1039639
(Assignee)

Comment 8

3 years ago
Created attachment 8534141 [details] [review]
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.
Attachment #8534141 - Flags: feedback?(squibblyflabbetydoo)
(Assignee)

Comment 9

3 years ago
Also I haven't tested Opus.
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Depends on: 947884
(Assignee)

Updated

3 years ago
No longer depends on: 947884
(Assignee)

Updated

3 years ago
Attachment #8534141 - Flags: feedback?(squibblyflabbetydoo) → review?(squibblyflabbetydoo)
(Reporter)

Comment 10

3 years ago
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?
Attachment #8534141 - Flags: review?(squibblyflabbetydoo) → review-
(Assignee)

Comment 11

3 years ago
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.
Attachment #8534141 - Flags: review- → review?(squibblyflabbetydoo)
(Assignee)

Comment 12

3 years ago
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
Blocks: 1118021
(Assignee)

Comment 13

3 years ago
On the other hand we reuse the binary parsing :-D
(Reporter)

Comment 14

3 years ago
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.
Attachment #8534141 - Flags: review?(squibblyflabbetydoo) → review-
(Assignee)

Comment 15

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

addressed comments again
Attachment #8534141 - Flags: review- → review?(squibblyflabbetydoo)
(Reporter)

Comment 16

3 years ago
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!
Attachment #8534141 - Flags: review?(squibblyflabbetydoo) → review+
(Assignee)

Comment 17

3 years ago
Merged

https://github.com/mozilla-b2g/gaia/commit/e3b73d0ad66eda4948dcd3657b8f5411e8678d7b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Summary: [music] Support album art in Vorbis comments (ogg and opus files) → [music] Support album art in Vorbis comments (ogg files)
(Assignee)

Updated

3 years ago
status-b2g-v2.0: --- → wontfix
status-b2g-v2.1: --- → wontfix
status-b2g-v2.2: --- → fixed
Target Milestone: --- → 2.2 S3 (9jan)
(Assignee)

Comment 18

3 years ago
Quick note about Opus: I need to find real world sample / use case to determine its status. Will file bugs accordingly.
You need to log in before you can comment on or make changes to this bug.