Missing album covers in tile view

VERIFIED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::Music
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: hub, Assigned: dkuo)

Tracking

({regression})

unspecified
2.2 S14 (12june)
ARM
Gonk (Firefox OS)
regression
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
Created attachment 8610767 [details]
Screenshot

Similar to Bug 1167336

I miss some album covers in the tile view. I have a large number of albums in my collection - the are simply not displayed.
The fact that we don't have a title also show that it is an album that have actual art.

See attached screenshot.
(Reporter)

Comment 1

3 years ago
This is flame, git master.

Gecko: a9bac08932d5a6fe5993b750bef70d489061dc6a
Gaia: 74e4793d4a8618255c4a255fa13ff11a97f7cc72
I'm going to try to fix this, with Jim as my backup in case it turns out to be harder than expected.

Jim points out that the app currently uses background-image so that it can use background-size:cover to force the album art to be square.  I think we can get the same thing using object-fit:cover on an img element, so I think we'll be okay.

The first step is to be able to reproduce this bug, so I need a simple way to get a bunch of test files on the phone where each one looks like it is from a different album.
Assignee: nobody → dflanagan
Created attachment 8610906 [details] [review]
work in progress patch
The attachment includes a new script that you can run like this:

$ cd gaia/apps/music/test/music/fake_albums/
$ ./createFakeAlbums.sh 200

It will push 200 mp3 files to your phone. Each one will be a separate album, and each one will have album art.

This makes it easy to reproduce this bug.

The patch also includes another commit that converts the tiles view to use an img instead of background-image. But it turns out that the tiles view layout is quite complicated and I'm not confident that I've done a good job converting it. This was a lot harder than I expected, and I don't really understand what a lot of the existing CSS is for, so I'm afraid I'm breaking stuff.

I'm going to nominate this a a 2.2 blocker and am hoping that Dominic or Jim can take it from me.  We should probably also fix createListElement() in utils.js to use img as well.

Dominic or Jim: can one of you take this to fix this week? (See bug 1167336 for more context, but basically, for scrolling lists of images we need to use <img> instead of background-image, or gecko may not always be able to display the images)
blocking-b2g: --- → 2.2?
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(dkuo)
(Reporter)

Comment 5

3 years ago
Let me know if you need to test something with my setup.
(Assignee)

Comment 6

3 years ago
It feels strange that gecko can use <img> to display lots of album covers but background-image, hope it's because gecko is unable to uplift some fixes so we have to use the <img> approach for 2.2. We did use <img> to display album covers before(1.x), we changed to background-image intentionally because it seems saved more memory for us at that time?, so these changes are actually the original approach, just let everyone to know about this.

I am waiting for Jim's review in another bug so I have time to take this, I will work on 2.2 first because I really think we should file a gecko bug to fix the background-image issue for master.
Assignee: dflanagan → dkuo
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(dkuo)
(In reply to Dominic Kuo [:dkuo] from comment #6)

> 
> I am waiting for Jim's review in another bug so I have time to take this, I
> will work on 2.2 first because I really think we should file a gecko bug to
> fix the background-image issue for master.

Thanks, Dominic.

Seth Fowler is aware of the background-image issue. He's overhauling the way <img> elements are handled right now and plans to fix background-image eventually, but I don't know when, so I suspect we'll need to fix this in 3.0 as well.

If I understand the issue correctly, background-image has always had this problem. What has changed in 2.2 and 3.0 is that Gecko is now more aggressive about limiting the amount of image memory that gets used.  You can ask :seth for details if you need them.

Comment 8

3 years ago
Dominic: Even though this is fixing a Gecko bug, I think using an <img> tag makes more sense in the long run. It means that our HTML is more semantic, so it will be easier to understand what's happening. It should also simplify some of our code, since it's easier to set the src on an <img> tag than it is to set the background-image on a <div>.

Comment 9

3 years ago
Blocking Reason: poor ux with missing album covers
blocking-b2g: 2.2? → 2.2+

Updated

3 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
Keywords: regression

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 10

3 years ago
Added test case. (In-MozTrap+)
- https://moztrap.mozilla.org/manage/cases/?filter-id=16261#caseversion-id-177899
Flags: in-moztrap+

Comment 11

3 years ago
Please add "verifyme" on the keywords field if you need QA to verify patches.
Many thanks.
Created attachment 8612368 [details] [review]
[gaia] dominickuo:bug-1168549 > mozilla-b2g:master
Created attachment 8612817 [details] [review]
[gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2
(Assignee)

Comment 14

3 years ago
Comment on attachment 8612817 [details] [review]
[gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2

David,

this is the patch for v2.2, basically the patch is about to use <img> to replace all the background-images in music, then add the css property |object-fit: cover|[1] to every cover images, also I did a little bit code refactoring on the functions for displaying images, some redundant code are removed.

Note that I found an issue in the player view, the cover image seems need about 0.5-1 second to actually apply the css |object-fit: cover| property, that means you can see a flicker from the original size to |object-fit: cover|, I guess it's a gecko issue and probably I have think of some way to avoid it.

I am still working on the patch for master branch because Jim has landed some changes about the cover images, and the patch will be different.

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit
Attachment #8612817 - Flags: review?(dflanagan)
Thanks Dominic. Does the switch actually fix this missing album art images in tiles view?

If the flicker is bad in the player view, then it would probably be better to continue to use background-image for that.  The cases we're worried about here are lists where there are many images, like the tiles view.  It should be fine to keep using background-image for the player view.

Jim: can you take this review, please? I'm swamped with gallery bugs.
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(dkuo)

Comment 16

3 years ago
Comment on attachment 8612817 [details] [review]
[gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2

I haven't thought very closely about this yet, but there are some problems either way. See GitHub. It'd be nice if we could make this code more similar to how it is now with background-images, but maybe that's impossible...
Flags: needinfo?(squibblyflabbetydoo)
Attachment #8612817 - Flags: review?(dflanagan) → review-
(Reporter)

Comment 17

3 years ago
I just tried the pull request on master, and I'm still missing images like I originally reported.
(Assignee)

Comment 18

3 years ago
Comment on attachment 8612817 [details] [review]
[gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2

Jim, I have addressed those issues on github, would you please review it again? thanks!
Attachment #8612817 - Flags: review- → review?(squibblyflabbetydoo)
(Assignee)

Comment 19

3 years ago
(In reply to David Flanagan [:djf] from comment #15)
> Thanks Dominic. Does the switch actually fix this missing album art images
> in tiles view?

Yes, at least for v2.2 because I haven't tried master yet.

> If the flicker is bad in the player view, then it would probably be better
> to continue to use background-image for that.  The cases we're worried about
> here are lists where there are many images, like the tiles view.  It should
> be fine to keep using background-image for the player view.

I couldn't find any way to fix the flicker problem in the player view, so let's keep what it is instead of replacing it to <img>.
Flags: needinfo?(dkuo)
(Assignee)

Comment 20

3 years ago
(In reply to Hubert Figuiere [:hub] from comment #17)
> I just tried the pull request on master, and I'm still missing images like I
> originally reported.

Hub, my patch for master is still a WIP, but it should fix the problem because I have replaced the background-images to <img>, anyway, I will re-test it after the master patch is ready to review, and will let you know if I also have the same issue or not, thanks for noticing it!
(In reply to Hubert Figuiere [:hub] from comment #17)
> I just tried the pull request on master, and I'm still missing images like I
> originally reported.

Also: there were some gecko bugs that caused problems even after switching from background-image to img. Seth fixed almost all of those, and the fixes have landed on master and uplift to 2.2 has been requested. See bugs 1169879, 880, and 881. Those fixes, plus moving away from background-image should mostly resolve this. (Though as you can see in bug 1166136, I am still seeing some issues when I have 500 images in gallery).

Comment 22

3 years ago
Comment on attachment 8612817 [details] [review]
[gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2

Assuming this passes tests and all, r=me. Maybe we should try to figure out why things are messed up for the player view; it might be a platform issue that needs fixing...
Attachment #8612817 - Flags: review?(squibblyflabbetydoo) → review+
Comment on attachment 8612817 [details] [review]
[gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): image lib changes in 2.2 and master have caused this regression, we think.
[User impact] if declined: users with large image collections will not be able to see all of their album art
[Testing completed]: locally
[Risk to taking this patch] (and alternatives if risky): not very risky
[String changes made]: none
Attachment #8612817 - Flags: approval-gaia-v2.2?(jocheng)
Attachment #8612817 - Flags: approval-gaia-v2.2?(doliver)

Comment 24

3 years ago
Comment on attachment 8612817 [details] [review]
[gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2

Hi David,
I suppose this will also land on master?
Attachment #8612817 - Flags: approval-gaia-v2.2?(jocheng) → approval-gaia-v2.2+
(Assignee)

Comment 25

3 years ago
(In reply to Josh Cheng [:josh] from comment #24)
> Comment on attachment 8612817 [details] [review]
> [gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2
> 
> Hi David,
> I suppose this will also land on master?

Josh, the answer is yes, I just finished the master patch and will landed after it passed the tests. Note that the master patch is the same as the v2.2, just need to solve the conflicts and I am carrying on the r+ to it as well, in next comment.
(Assignee)

Comment 26

3 years ago
Comment on attachment 8612368 [details] [review]
[gaia] dominickuo:bug-1168549 > mozilla-b2g:master

Carry on the r+ from the v2.2 patch.
Attachment #8612368 - Flags: review+

Comment 27

3 years ago
Thanks Dominic!
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 28

3 years ago
Autolander is temporary disabled, will land it manually.
Keywords: checkin-needed
Attachment #8612817 - Flags: approval-gaia-v2.2?(doliver)
The master patch has Gij failures, so I only landed the v2.2 patch, which was green.

v2.2: https://github.com/mozilla-b2g/gaia/commit/b92e782ca12397acc3eb52f2e237522c0213f4e0
status-b2g-v2.2: affected → fixed
Flags: needinfo?(dkuo)
Target Milestone: --- → 2.2 S14 (12june)
(Assignee)

Comment 30

3 years ago
Just now I found a minor issue while I am addressing the Gij failures on master, so revert the v2.2 patch first, will re-land it after I fixed it.

reverted on v2.2: https://github.com/mozilla-b2g/gaia/commit/b96e657ce2822df5da5da1a8ba91c38ad3281bc9
status-b2g-v2.2: fixed → affected
(Assignee)

Comment 31

3 years ago
master: https://github.com/mozilla-b2g/gaia/commit/e0fbadeb78a96137f071d9be7a47ef9fe882d17f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
Flags: needinfo?(dkuo)
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
See Also: → bug 1171366
Comment hidden (typo)

Updated

3 years ago
Flags: needinfo?(dkuo)

Comment 34

3 years ago
Dominic: I just noticed that this patch adds a fade-in animation for the album art on the Tiles view. Since we re-display the Tiles view from scratch every 25 songs while we're scanning, this means that you'll see the same album art fade in a bunch of times in a row. I think we should disable the animation on the Tiles view, at least until we have a smarter way of showing newly-added albums to the Tiles view.
Flags: needinfo?(dkuo)
(Assignee)

Comment 35

3 years ago
(In reply to Jim Porter (:squib) from comment #34)
> Dominic: I just noticed that this patch adds a fade-in animation for the
> album art on the Tiles view. Since we re-display the Tiles view from scratch
> every 25 songs while we're scanning, this means that you'll see the same
> album art fade in a bunch of times in a row. I think we should disable the
> animation on the Tiles view, at least until we have a smarter way of showing
> newly-added albums to the Tiles view.

Indeed, I agree with you. My intention was to let the album art in music have same animation, but I didn't consider the re-render logic in Tiles view. I will file a followup to patch it, thanks for catching this!
Flags: needinfo?(dkuo)
(Assignee)

Updated

3 years ago
Blocks: 1172403
Keywords: verifyme
This issue is verified fixed on the latest 3.0 and 2.2 Nightly Flame builds.

Actual Results: All album covers were seen 10/10 times (5 each).

Environmental Variables:
Device: Flame 3.0 KK 319MB Full Flash
BuildID: 20150618010201
Gaia: b404c41c5471c31610e64defb74ec066b411e724
Gecko: a3f280b6f8d5
Gonk: a4f6f31d1fe213ac935ca8ede7d05e47324101a4
Version: 41.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Environmental Variables:
Device: Flame 2.2 KK 319MB Full Flash
BuildID: 20150618002507
Gaia: 3414b07dc489976bf510fd8042c0af3b1192c160
Gecko: a2db74491088
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.