1170680 - Do not add non-animated images to the visible list in response to UNLOCKED_DRAW

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

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

We do not currently get the full benefits of the image locking improvements made for bug 1166136 so far, because we still end up drawing images for the low-res displayport during fast scrolling, and drawing them in this way ends up locking them. Although they get unlocked the next time we calculate image visibility, on low-memory devices (e.g. a Flame limited to 319MB), we can end up running out of image memory during a single paint pass on the low-res displayport in bad cases. And unfortunately the Gallery app is such a bad case.

Comment 1

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

Attachment: Do not lock images in response to UNLOCKED_DRAW

Here's the patch.

This is the only consumer of UNLOCKED_DRAW, so we could potentially remove
everything UNLOCKED_DRAW-related now, but because this is intended for B2G 2.2
uplift, I'd prefer to do that part in a followup.

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c83da277e63

Comment 3

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

OK, after discussion on IRC with Timothy, it's become clear that we still want to add animated images to the visible images list promptly, so they start animating without delay.

I've updated the summary of the bug to reflect this new approach: now we'll add images to the visible list in response to UNLOCKED_DRAW if they're animated, but not otherwise.

Comment 4

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

Attachment: Do not add non-animated images to the visible list in response to UNLOCKED_DRAW

OK, this patch implements the new approach discussed in comment 3.

There's no downside to adding animated images to the visible list, really, since
animated images are always locked anyway.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f811580f003e

Comment 6

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

Attachment: Do not add non-animated images to the visible list in response to UNLOCKED_DRAW

Whoops. Forgot an initial null check.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcaa39e3daca

Comment 8

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

This patch triggers a failure in layout/reftests/image-element/bug-364968.html. I've seen this fail due to many of my try pushes lately with many different patches. I suspect we simply have a bug related to sync decoding and -moz-element() which I haven't been able to track down yet. Given the importance of this patch and the tight timelimit before B2G 2.2 ships, I've decided to mark the test random in this bug and reenable it in bug 1170871.

Comment 9

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

Attachment: Do not add non-animated images to the visible list in response to UNLOCKED_DRAW

This version of the patch is identical except that it disables
layout/reftests/image-element/bug-364968.html.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a65f5274bda2

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #8)
> This patch triggers a failure in
> layout/reftests/image-element/bug-364968.html. I've seen this fail due to
> many of my try pushes lately with many different patches. I suspect we
> simply have a bug related to sync decoding and -moz-element() which I
> haven't been able to track down yet. Given the importance of this patch and
> the tight timelimit before B2G 2.2 ships, I've decided to mark the test
> random in this bug and reenable it in bug 1170871.

This seems like it's probably due to the fact that "ImageIsAnimated" returns false if it encounters a non-success return codes and RasterImage returns a not available return code when the image hasn't been decoded yet (ie it doesn't know if its animated).

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a65f5274bda2

Comment 13

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

(In reply to Timothy Nikkel (:tn) from comment #11)
> This seems like it's probably due to the fact that "ImageIsAnimated" returns
> false if it encounters a non-success return codes and RasterImage returns a
> not available return code when the image hasn't been decoded yet (ie it
> doesn't know if its animated).

As we discussed on IRC, that doesn't explain this test failure. It is something that we should probably address, though.

Comment 14

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

Comment on attachment 8614460 [details] [diff] [review]
Do not add non-animated images to the visible list in response to UNLOCKED_DRAW

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not sure, but it was exposed by the fact that we enabled image locking on B2G in bug 1148696.
User impact if declined: Images may fail to decode due to low memory, causing blank thumbnails in the gallery which may persist across reboots (making this effectively a data loss issue). This patch is one of several that together fix bug 1166136, which is nominated for blocking 2.2, and bug 1164164, which is a 2.2 blocker.
Testing completed: Extensively tested locally. On mozilla-central.
Risk to taking this patch (and alternatives if risky): This is a very simple, low risk patch.
String or UUID changes made by this patch: None.

Comment 15

[Josh Cheng [:josh]]

Comment on attachment 8614460 [details] [diff] [review]
Do not add non-animated images to the visible list in response to UNLOCKED_DRAW

Approving as this causing Gallery locking issue of bug 1166136.

Comment 16

[Josh Cheng [:josh]]

Hi Eric,
Could you help to find someone perform regression on Gallery app for 2.2? There has been lots of dependencies for bug 1166136 landed on 2.2.
Thanks!

Comment 17

[Eric Chang [:ericcc] [:echang]]

Sure, we will start regression check on today's build on Gallery and Camera this afternoon.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b1faf262373a