Recompute image visibility on a timer if layout or style flushes have occurred

RESOLVED FIXED in Firefox 41, Firefox OS v2.2

Status

()

Core
Layout: Images
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8613200 [details] [diff] [review]
Recompute image visibility on a timer if layout or style flushes have occurred

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)
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e140de0e2a7
(Assignee)

Updated

2 years ago
Blocks: 1168985
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+
(Assignee)

Comment 4

2 years ago
Created attachment 8613205 [details] [diff] [review]
Recompute image visibility on a timer if layout or style flushes have occurred

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

Updated

2 years ago
Attachment #8613200 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb3bd9478fd
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.
Flags: needinfo?(seth)
https://hg.mozilla.org/mozilla-central/rev/bbb3bd9478fd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 8

2 years ago
(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.
Flags: needinfo?(seth)
(Assignee)

Comment 9

2 years ago
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?
(Assignee)

Updated

2 years ago
Blocks: 1170276

Comment 10

2 years ago
2.2+ as this causing function broken for Gallery app.
blocking-b2g: --- → 2.2+
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed

Updated

2 years ago
Attachment #8613205 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a4a5092bd193
status-b2g-v2.2: affected → fixed
Depends on: 1217230

Updated

2 years ago
Depends on: 1242256
You need to log in before you can comment on or make changes to this bug.