Closed
Bug 1169879
Opened 10 years ago
Closed 10 years ago
Use only the critical displayport when computing image visibility
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(1 file)
3.14 KB,
patch
|
tnikkel
:
review+
jocheng
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
Currently PresShell::MarkImagesInSubtreeVisible(), which is used to compute which images are visible, bases its calculations on the displayport for any nsIScrollableFrame which has one. It uses nsLayoutUtils::GetDisplayPort() to determine the displayport of the scrollable frame.
The problem is that GetDisplayPort() returns the full displayport - the one used for painting low-precision buffers - and not the critical displayport. The full displayport can be *huge* when gfxPrefs::UseLowPrecisionBuffer() is enabled. When scrolling, it can easily be well over 10,000 CSS pixels in height, and even when stationary it's still thousands of CSS pixels high. This is compounded by the fact that we may triple the size of whatever GetDisplayPort() returns (via ExpandRectToNearlyVisible).
The result is that we end up considering most or all of the thumbnails in the B2G Gallery app visible, even with huge numbers of images in the Gallery.
We need to fix this ASAP, as it's causing serious issues on B2G. Long term, I'd like to explore a notion of image locking that would interact with things like the low resolution displayport better - I have some ideas, but they're not fully baked yet. But in the short term, I think it's clear that the best option is to only lock inside the critical displayport. We will still lock images in the low-res displayport *while we're drawing it*, because they'll get locked via OnUnlockedDraw(). But we'll unlock those images very quickly, as soon as we recompute image visibility, and I think that's for the best until we have a better solution in place.
Assignee | ||
Comment 1•10 years ago
|
||
Here's the patch.
Again, we can explore better ways to integrate the low-res displayport with
image locking in the future. But for now we need to make this change to make
things sane again and fix the image-locking-related memory issues that B2G is
currently experiencing.
Attachment #8613199 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•10 years ago
|
||
Updated•10 years ago
|
Attachment #8613199 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Comment 4•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8613199 [details] [diff] [review]
Use only the critical displayport when computing image visibility
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.
Attachment #8613199 -
Flags: approval-mozilla-b2g37?
Updated•10 years ago
|
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
Updated•10 years ago
|
Attachment #8613199 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
blocking-b2g: --- → 2.2+
Comment 6•10 years ago
|
||
2.2+ as this causing function broken for Gallery app.
Comment 7•10 years ago
|
||
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•