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 Please n-i to RyanVM, jcristau, or pascal)]

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 Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 11

[jim sanders]

When you convert video from a source that uses DisplayPort or Mini DisplayPort to DVI or HDMI (for example, a computer to a monitor), you will need to consider the difference between passive and active adapters.

If the source supports dual-mode DisplayPort (also known as DP++), then you can use a passive adapter because the source can perform the conversion. If the source does not support DP++, then you need to use an active converter, which includes additional chips to perform the conversion.

Thunderbolt ports support DP++ natively. To run multiple monitors from the same computer, refer to the Active Adapters section below.

Passive Adapters

Passive adapters are less expensive since they do not need to include additional chips. A quick way to check whether you can use a passive adapter is to see if the DisplayPort or Mini DisplayPort source has the DP++ symbol above it. The symbol is a D with a P inside of it, with two + signs to the left, one on top of the other.

Active Adapters

Active adapters use additional chips to make the conversion inside the adapter, regardless of whether the source supports DP++. This means that active adapters are more expensive than passive adapters.

If you want to use multiple monitors with the same computer, you should use an active adapter because some video cards cannot run the maximum number of monitors while using DP++. This is especially true if the computer has more than one DisplayPort or Mini DisplayPort connection. Check with your video card manufacturer to confirm which type of adapter you need for the setup that you would like to run.

Note: You should use passive adapters with StarTech.com MST hubs, except when you are converting from DisplayPort to VGA, since that type of conversion requires active adapters.  For more information on using adapters with StarTech.com MST hubs, refer to the following FAQ: http://appyspot.com/

Surface Dock

If you experience issues when you connect passive video signal adapters to the Surface Dock, Microsoft recommends that you use active video signal converters instead. StarTech.com offers an active video signal converter for DVI monitors (MDP2DVIS) and for HDMI monitors (MDP2HD4KS).

Note: This issue does not apply to the Surface Pro 3 Docking Station.