Differentiate between no critical display port and empty critical display port in ClientTiledPaintedLayer

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: jnicol, Assigned: jnicol)

Tracking

(Blocks 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46 fixed, firefox47 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
Currently the logic in ClientTiledPaintedLayer treats an empty mPaintData.mCriticalDisplayPort to mean that there is no critical display port - i.e. that the entire visible region should be painted.

However, mPaintData.mCriticalDisplayPort will be set to empty in BeginPaint() if either the displayport or composition bounds fall entirely outwith the layer bounds. So if, for example, a layer begins much further down the page than what is currently on screen, we will render the entire layer.

We need to differentiate between the display port being empty, and there not being a display port.
Assignee

Comment 1

3 years ago
Currently the logic in ClientTiledPaintedLayer treats an empty critical
display port to mean that there is no critical display port, i.e. that
the entire visible region should be painted. However, the critical
displayport should be, and is, empty if either the display port or
composition bounds are entirely outwith the layer's bounds. We want to
render none of the layer in this case, not all of it.

Change BasicTiledLayerPaintData::mCriticalDisplayPort's type to a
Maybe<LayerIntRect>, and differentiate between it being not set and it
being an empty rect.

Review commit: https://reviewboard.mozilla.org/r/36065/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36065/
Attachment #8722520 - Flags: review?(bugmail.mozilla)
Comment on attachment 8722520 [details]
MozReview Request: Bug 1250517 - Differentiate between no critical display port and empty critical display port in ClientTiledPaintedLayer; r?kats

https://reviewboard.mozilla.org/r/36065/#review32687

Nice find, thanks!
Attachment #8722520 - Flags: review?(bugmail.mozilla) → review+
Assignee

Comment 3

3 years ago
Requesting checkin please

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57ed1bd51607 (patch only affects tiling, so OS X and android)
Keywords: checkin-needed

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/00cda66bd400
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8722520 [details]
MozReview Request: Bug 1250517 - Differentiate between no critical display port and empty critical display port in ClientTiledPaintedLayer; r?kats

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Excessive memory usage, possibly crashes
[Describe test coverage new/current, TreeHerder]: Nightly, automated tests
[Risks and why]: Low, but it does touch a sensitive area
[String/UUID change made/needed]: None
Attachment #8722520 - Flags: approval-mozilla-beta?
Attachment #8722520 - Flags: approval-mozilla-aurora?
Marking affected for 45 and 46.  As I understand this from discussion on irc all branches are affected and this isn't a recent regression. The fix may help us decrease OOM crashes on fennec.
Comment on attachment 8722520 [details]
MozReview Request: Bug 1250517 - Differentiate between no critical display port and empty critical display port in ClientTiledPaintedLayer; r?kats

May help with OOM crashes for android and mac. 
Let's try this on aurora.
Attachment #8722520 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8722520 [details]
MozReview Request: Bug 1250517 - Differentiate between no critical display port and empty critical display port in ClientTiledPaintedLayer; r?kats

As discussed on IRC, too late for 45.
Attachment #8722520 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.