Closed Bug 1169880 Opened 5 years ago Closed 5 years ago
Recompute image visibility on a timer if layout or style flushes have occurred
Layout or style changes can cause images that were previously visible to stop being visible. This can happen, for example, because new elements are inserted into the document that push the element containing the image out of the visible region, or because of changes to CSS properties like |visibility|. Unfortunately, I'm not aware of a general way in which we can precisely detect when changes like these will change an image's visibility. To make sure that our image visibility data doesn't become too far out of date, then, we need to periodically recompute it if there's a reasonable chance that something could have changed.
Here's the patch. We schedule an image visibility update in the refresh driver if: - A layout or style flush has occurred. - Enough time has passed since the last time we did it. (The default is 100ms.) - The refresh driver isn't throttled. (i.e., we don't do this for background tabs, in which all images should already be unlocked.) - We're not still in paint supression. This patch fixes the testcase in bug 1168985, but it's more general. It helps a great deal with memory usage of the Gallery app during the scanning process, which is what the testcase in bug 1168985 was meant to simulate.
Attachment #8613200 - Flags: review?(tnikkel)
Comment on attachment 8613200 [details] [diff] [review] Recompute image visibility on a timer if layout or style flushes have occurred 100ms feels a little low to me. I base this on nothing in particular :) If we are doing ticks more than every 100ms anyway we might want to back off on image visibility updates (and let other work happen) more so than just limiting them to 100ms.
Attachment #8613200 - Flags: review?(tnikkel) → review+
Agreed on the timer interval. I tested that 1000ms was fine both for the desktop testcase and for Gallery scanning on the device, and it passed with flying colors, so this version of the patch changes the default interval to 1000ms.
Attachment #8613200 - Attachment is obsolete: true
It occured to me that this rate limiting doesn't take into account other image visibility updates that are triggered via other means (scrolling, display port margins changing). We should probably do that.
(In reply to Timothy Nikkel (:tn) from comment #6) > It occured to me that this rate limiting doesn't take into account other > image visibility updates that are triggered via other means (scrolling, > display port margins changing). We should probably do that. Yep, I'll file a followup to clear mNeedToRecomputeVisibility if something else calls ScheduleImageVisibilityUpdate(). Should be straightforward.
Comment on attachment 8613205 [details] [diff] [review] Recompute image visibility on a timer if layout or style flushes have occurred 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 fairly low-risk patch. String or UUID changes made by this patch: None.
Attachment #8613205 - Flags: approval-mozilla-b2g37?
2.2+ as this causing function broken for Gallery app.
Attachment #8613205 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.