1163187 - Remove -moz-crisp-edges anti-optimization

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

In bug 943845, Vivien added code to Gallery to determine when the user was scrolling the thumbnails and set image-rendering: -moz-crisp-edges in that case.

I never really understood why we were doing that. But apparently it fixed some visual jank when scrolling.  Possibly related to the fact that the gallery app was still using the old visibility_monitor.js module to manually manage a viewport and hide and show thumbnails. That was back when we were still using background-image.

Anyway, it now turns out that that former optimization is now an anti-optimization that is causing severe memory performance issues.  So this bug is for removing that code.

And we're going to have to uplift this to 2.2 as part of resolving bug 1161734

Comment 1

[David Flanagan [:djf]]

Nominating for 2.2 since it blocks a 2.2+ bug.

Comment 2

[David Flanagan [:djf]]

Vivien: bug 943845 was yours, so I'm setting needinfo so you know about this image memory issue caused by -moz-crisp-edges.

Did you ever end up using -moz-crisp-edges and scroll_detector.js in any other apps? Would you like me to remove those?

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1163187 > mozilla-b2g:master

Comment 4

[David Flanagan [:djf]]

Comment on attachment 8603598 [details] [review]
[gaia] davidflanagan:bug1163187 > mozilla-b2g:master

Vivien: this patch removes your v1.4 -moz-crisp-edges scrolling optimization from bug 943845. Seth Fowler says that -moz-crisp-edges causes gecko to use more image memory that it would otherwise.

Without this patch, when I have lots of images in Gallery, when I launch gallery and scroll quickly to the bottom, I sometimes see gray squares where thumbnails should be. And they remain gray forever.  (Either becase there is not enough memory to decode them or because of some kind of race condition with the change of the image-rendering attribute, perhaps)

With the patch, when I scroll quickly to the bottom, I see blurred images as I scroll quicklky. When I reach the bottom, there are lots of gray squares, but within a few seconds they are replaced by thumbnails.

I think we need to land this as part of resolving bug 1161734. Asking for your review in case you feel strongly that we need to retain the -moz-crisp-edges code.

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to David Flanagan [:djf] from comment #4)
> Vivien: this patch removes your v1.4 -moz-crisp-edges scrolling optimization
> from bug 943845. Seth Fowler says that -moz-crisp-edges causes gecko to use
> more image memory that it would otherwise.

To clarify this: -moz-crisp-edges forbids us from using high-quality scaling to display the image. That's the kind of scaling we can do ahead of time, before we draw. We actually now use downscale-during-decode to scale images to the size we're going to draw them at as we're decoding them. That improves performance and saves memory.

Since -moz-crisp-edges forbids us from using the high-quality scaled version, we have to redecode the image at its full intrinsic size. Because we draw the intrinsic size version while scrolling and the downscaled version when we're not scrolling, often both within a short period of time, I suspect we get tricked into thinking that both versions are visible, and so both versions stick around, whether we're scrolling or not.

There should be no performance advantage to -moz-crisp-edges - in fact, it is almost guaranteed to be slower unless we're drawing the image at exactly its intrinsic size, since because we're forbidden from using the high-quality pre-scaled version of the image, it forces us to scale *every time we draw*, which has a cost. And there is a significant cost in terms of memory consumption to using -moz-crisp-edges.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8603598 [details] [review]
[gaia] davidflanagan:bug1163187 > mozilla-b2g:master

APZ has improved a lot since this has been introduced. I don't think this is needed anymore, so no I don't feel strongly against removing it :)

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8603598 [details] [review]
[gaia] davidflanagan:bug1163187 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): This is an old optimization that is now not required. It appears that it contributes to the problems in 2.2+ blocking bug 1161734

[User impact] if declined: missing or flickering images in gallery, more frequent OOMs.

[Testing completed]: locally

[Risk to taking this patch] (and alternatives if risky): not risky.

[String changes made]: none

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8603598 [details] [review]
[gaia] davidflanagan:bug1163187 > mozilla-b2g:master

Keven: are you the right person to ask for uplift approval? This blocks a CAF 2.2 blocker.

Comment 9

[David Flanagan [:djf]]

Dominic or Jim: Music also has this out-of-date optimization that we should probably remove.  Either of you want to take it?

Comment 10

[David Flanagan [:djf]]

Nevermind. Hub is going to do this for Music.

Comment 11

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/fdc331adfa64d4dc7d2b2dfee8a8889388480d7b

Comment 12

[David Flanagan [:djf]]

Comment on attachment 8603598 [details] [review]
[gaia] davidflanagan:bug1163187 > mozilla-b2g:master

Sorry Keven: I see that Josh is the right person to ask for uplift approval.

Comment 13

[Josh Cheng [:josh]]

Comment on attachment 8603598 [details] [review]
[gaia] davidflanagan:bug1163187 > mozilla-b2g:master

Approving given this contribute to 2.2+ CAF blocking bug 1161734

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/33d0321458320453e18d7b1d8fc86c10b86a64a2