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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jamie Nicol [:jnicol]]

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.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: MozReview Request: Bug 1250517 - Differentiate between no critical display port and empty critical display port in ClientTiledPaintedLayer; r?kats

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/

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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!

Comment 3

[Jamie Nicol [:jnicol]]

Requesting checkin please

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

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00cda66bd400

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00cda66bd400

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

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 8

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20e336e42594

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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.