If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[music] Use full-size album art in the player UI

RESOLVED FIXED in Firefox OS master

Status

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

People

(Reporter: squib, Assigned: squib)

Tracking

(Blocks: 1 bug)

unspecified
NGA S1 (29May)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(b2g-master fixed)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
djf
: review+
Details | Review | Splinter Review
(Assignee)

Description

3 years ago
Currently, we use thumbnail-sized images in the music player. This looks bad on high-resolution devices, since we're scaling up from 300x300 pixels. Instead, we should use the full-size version of the art so that things look prettier.

Now would also be a good time to improve the transition between tracks, especially when they're in the same album. There's no need for a transition in that case.
(Assignee)

Comment 1

3 years ago
Created attachment 8557189 [details] [review]
Fix it

David: Does this seem like a sensible way to improve the loading of album art in the player view? We're using an offscreen Image object to get the onload event, but I think I did things right so that Gecko will decode the image at the right size.

Of course, I'm also open to further improvements here...
Attachment #8557189 - Flags: feedback?(dflanagan)
Comment on attachment 8557189 [details] [review]
Fix it

f- because I think that the combination of background-image plus an offscreen image will cause the image to be decoded at full size. See my comments on github.

Sometimes I worry about keeping too many blob urls or references to blobs around. Gallery seems to get away with it for all of its thumbnails. I don't know whether there is anything different about these blob urls that use blobs sliced out of much larger blobs.  At some point it might be good to do some memory profiling and make sure that this album art cache isn't causing a lot of memory to be allocated.  And if so, you could perhaps convert to a cache that flushes the least recently used items.
Attachment #8557189 - Flags: feedback?(dflanagan) → feedback-
(Assignee)

Updated

3 years ago
Blocks: 1122648
(Assignee)

Comment 3

3 years ago
Comment on attachment 8557189 [details] [review]
Fix it

Ok, this should be done. Only the last three commits in the PR matter; the others are only there because this work revealed a few bugs in the lazy loading for JS files in the music app.

I've opted to remove the "offscreen images" entirely in this PR. For the player view, they weren't actually helping. Things still look the same even without waiting for an offscreen image to load (in fact, I think that code wasn't doing what we expected anymore, since Gecko downsamples images now).

For the sublist view, since we're coming from a list view, we should already have the thumbnail cached, so we don't even need to fade in. This is pretty much how all the other list-based views work anyway.
Attachment #8557189 - Flags: feedback- → review?(dflanagan)
(Assignee)

Comment 4

3 years ago
I added a bonus commit to this which makes album art crossfade when a new album starts. It's a lot smoother looking!
(Assignee)

Updated

2 years ago
Duplicate of this bug: 981653
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1139605
Comment on attachment 8557189 [details] [review]
Fix it

Jim,

This looks good to me. Various comments on github, including an idea for resolving your album art race condition. But pretty much everything I had to say was an optional suggestion.

Sorry that this review took so long.
Attachment #8557189 - Flags: review?(dflanagan) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
http://docs.taskcluster.net/tools/task-graph-inspector/#UB3Du4bdSKa6GVqxKOXWuw

The pull request failed to pass integration tests. It could not be landed, please try again.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
http://docs.taskcluster.net/tools/task-graph-inspector/#ZhpN1XvVQnyvh01wiflXrw

The pull request failed to pass integration tests. It could not be landed, please try again.
(Assignee)

Comment 10

2 years ago
One last try and then I'll just land it myself.
Keywords: checkin-needed

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
http://docs.taskcluster.net/tools/task-graph-inspector/#pJyxFA7eSJKofOncEnnQdQ

The pull request failed to pass integration tests. It could not be landed, please try again.
Try run here looks good: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=944df3b020a5f40716ded346b6afaf5b45e66a93

Let's land for now. (Not sure what's causing the autoland issues, but will look into it)

In master: https://github.com/mozilla-b2g/gaia/commit/4b192bc9c2a9e343418b39aed5dc7b38b949373d
status-b2g-master: --- → fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Depends on: 1170003

Updated

2 years ago
Target Milestone: --- → NGA S1 (29May)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 962439
(Assignee)

Updated

2 years ago
Blocks: 1159028
You need to log in before you can comment on or make changes to this bug.