1168549 - Missing album covers in tile view

Status: VERIFIED FIXED

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

Description

[Hubert Figuiere [:hub]]

Attachment: 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.

Comment 1

[Hubert Figuiere [:hub]]

This is flame, git master.

Gecko: a9bac08932d5a6fe5993b750bef70d489061dc6a
Gaia: 74e4793d4a8618255c4a255fa13ff11a97f7cc72

Comment 2

[David Flanagan [:djf]]

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.

Comment 3

[David Flanagan [:djf]]

Attachment: work in progress patch

Comment 4

[David Flanagan [:djf]]

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)

Comment 5

[Hubert Figuiere [:hub]]

Let me know if you need to test something with my setup.

Comment 6

[Dominic Kuo [:dkuo]]

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.

Comment 7

[David Flanagan [:djf]]

(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

[Jim Porter (:squib)]

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

[Hema Koka [:hema]]

Blocking Reason: poor ux with missing album covers

Comment 10

[William Hsu [:whsu]]

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

Comment 11

[William Hsu [:whsu]]

Please add "verifyme" on the keywords field if you need QA to verify patches.
Many thanks.

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] dominickuo:bug-1168549 > mozilla-b2g:master

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] dominickuo:v2.2-bug-1168549 > mozilla-b2g:v2.2

Comment 14

[Dominic Kuo [:dkuo]]

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

Comment 15

[David Flanagan [:djf]]

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.

Comment 16

[Jim Porter (:squib)]

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...

Comment 17

[Hubert Figuiere [:hub]]

I just tried the pull request on master, and I'm still missing images like I originally reported.

Comment 18

[Dominic Kuo [:dkuo]]

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!

Comment 19

[Dominic Kuo [:dkuo]]

(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>.

Comment 20

[Dominic Kuo [:dkuo]]

(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!

Comment 21

[David Flanagan [:djf]]

(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

[Jim Porter (:squib)]

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...

Comment 23

[David Flanagan [:djf]]

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

Comment 24

[Josh Cheng [:josh]]

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?

Comment 25

[Dominic Kuo [:dkuo]]

(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.

Comment 26

[Dominic Kuo [:dkuo]]

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

Carry on the r+ from the v2.2 patch.

Comment 27

[Josh Cheng [:josh]]

Thanks Dominic!

Comment 28

[Dominic Kuo [:dkuo]]

Autolander is temporary disabled, will land it manually.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Dominic Kuo [:dkuo]]

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

Comment 31

[Dominic Kuo [:dkuo]]

master: https://github.com/mozilla-b2g/gaia/commit/e0fbadeb78a96137f071d9be7a47ef9fe882d17f

Comment 32

[Dominic Kuo [:dkuo]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/8fc797527a3eca7665bc1d1828848f2fb77ca99f

Comment 33

[Josh Cheng [:josh]]

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

Hi Dominic,
Approving for 2.2 as CC is closing and this is blocking.
Please help to complete master patch when you have time.
Thanks!

Comment 34

[Jim Porter (:squib)]

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.

Comment 35

[Dominic Kuo [:dkuo]]

(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!

Comment 36

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

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