Closed Bug 1250517 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: jnicol, Assigned: jnicol)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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+
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
https://hg.mozilla.org/mozilla-central/rev/00cda66bd400
Status: NEW → RESOLVED
Closed: 8 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-
You need to log in before you can comment on or make changes to this bug.