1031107 - Always use display ports when tiling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

If we set 0-sized display port margins when tiling, then the display port will get rounded out to the nearest tile boundary. This means that we'll always paint whole tile increments when scrolling.

If we can't paint whole tiles in under 16ms then this puts us at risk of being janky, but it increases average throughput and lowers out average CPU/battery usage.

This is likely to be a huge win for direct2d (where paint time isn't overly dependent on number of pixels filled), and more of a tradeoff for software backends.

Ideally we could also make the scrolling layer changes on the main thread and avoid display list building for the intermediate paints.

Comment 1

[Matt Woodrow (:mattwoodrow)]

To clarify, I mean when using tiling on desktop and we don't have APZ.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Fix DoLogTestDataForPaint when using temporary layer managers

We hit asserts in this code when taking snapshots on desktop. The root pres context layer manager isn't necessarily the same one as we're drawing with, so we end up calling StartPaint and LogTestDataForPaint on different layer managers.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Don't restrict out-of-flow frames dirty area to the viewport when not painting

When doing hit testing with IGNORE_ROOT_SCROLL_FRAME, we expect to build display items that outside of the viewport.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Skip display port visibility when computing plugin geometry

We want to compute the real visibility of plugins, not what fits within the display port.

We might need to change this when we get APZ, but I don't want to try fix it within this bug.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Union the display port dirty area in nsSubDocumentFrame

When we have non-overlay scrollbars, their area isn't included within the display port bounds. We still need to include them though.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Disable display list flattening for RecomputeVisibilty

nsDisplayScrollLayer really doesn't like it if we try to merge twice, which can happen if we get one of them forced into an inactive layer (CSS blend modes!).

This will be rendered obsolete by bug 1022612, so might not need to land it.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Subtract from the visible region in nsDisplayScrollLayer if we're not using APZ

If we don't have APZ enabled then we want to reduce the visible region as much as possible, otherwise we fail the overlapping layers mochitest.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Don't set ignore scroll frame without APZ

We need this so we still get scrollbars on desktop.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8: Set display ports on tiled scrollable layers

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8446915 [details] [diff] [review]
Part 6: Subtract from the visible region in nsDisplayScrollLayer if we're not using APZ

We should do the same for nsDisplaySubDocument::ComputeVisibility to keep them in sync at least.

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8446902 [details] [diff] [review]
Part 4: Union the display port dirty area in nsSubDocumentFrame

Hmm, this could end up setting a really big dirty rect when the APZC velocity is high. In that case APZC sets a display port that is biased in the direction of the velocity enough that it doesn't intersect the dirty rect at all. Would it be better to do something in scroll frames which makes the dirty rect include the scrollbars when building the scrollbars if the scroll frame has a display port?

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8446917 [details] [diff] [review]
Part 7: Don't set ignore scroll frame without APZ

Why don't we get scrollbars? We should be adding them in here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2434  Do they just get placed wrongly?

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8446918 [details] [diff] [review]
Part 8: Set display ports on tiled scrollable layers

You do intend for this to only set display port margins for the root content doc root scroll frame and scroll frames with will-change: scroll, correct? That looks like what this will do, not all active scroll frames like the comment says.

Comment 14

[Markus Stange [:mstange]]

(In reply to Timothy Nikkel (:tn) from comment #12)
> Comment on attachment 8446917 [details] [diff] [review]
> Part 7: Don't set ignore scroll frame without APZ
> 
> Why don't we get scrollbars? We should be adding them in here
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp#2434  Do they just get placed wrongly?

See also bug 1013377 and bug 1013474 comment 2.

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8446917 [details] [diff] [review]
Part 7: Don't set ignore scroll frame without APZ

Okay, can you make the same change to nsDOMWindowUtils::SetDisplayPortForElement ?

Comment 16

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

Comment on attachment 8446897 [details] [diff] [review]
Part 1: Fix DoLogTestDataForPaint when using temporary layer managers

Review of attachment 8446897 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +2213,4 @@
>     * Log a key/value pair for APZ testing during a paint.
>     * @param aPresShell The pres shell that identifies where to log to. The data
>     *                   will be written to the APZTestData associated with the
>     *                   pres shell's layer manager.

Update docs

Comment 17

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

I notice a few of these patches using gfxPrefs::AsyncPanZoomEnabled(). I just want to make sure this doesn't inadvertently affect B2G, because there AsyncPanZoomEnabled() will return true in the root process even though we're not actually doing async scrolling. Part 7 is the only one that might potentially come into play here but I'm not sure.

Comment 18

[Timothy Nikkel (:tnikkel)]

It looks like these patches only change behaviour when AsyncPanZoomEnabled() is false, so that should be good then.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Interestingly, it appears that gfxPrefs::AsyncPanZoomEnabled() is false for Android and B2G desktop builds on tinderbox. Is that expected?

Comment 20

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

Yeah. On Android we're using the java pan/zoom controller code which uses different machinery, so AsyncPanZoomEnabled() is false. On B2G desktop builds APZC is disabled by default on most tests. There's a R-oop test that has APZC enabled; the harness explicitly enables the pref when running the test. The Linux Ripc test also has APZC enabled, but that's about it AFAIK.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Union the display port dirty area in nsSubDocumentFrame v2

Comment 22

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Subtract from the visible region in nsDisplayScrollLayer if we're not using APZ v2

Comment 23

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8456551 [details] [diff] [review]
Part 4: Union the display port dirty area in nsSubDocumentFrame v2

Hmm, is this actually going to do what you want? The first version of this patch was in nsSubDocumentFrame so it affected root scroll frames. This version will affect only non-root scroll frames because we will use the AppendScrollPartsTo calls further up in the "IgnoreScrollFrame" if for root scroll frames with display ports.

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8456552 [details] [diff] [review]
Part 6: Subtract from the visible region in nsDisplayScrollLayer if we're not using APZ v2

I think we need to restrict "removed" to inside the scrollport, otherwise we could be removing things from the visible region outside the scrollport that we don't actually cover.

Comment 25

[Matt Woodrow (:mattwoodrow)]

(In reply to Timothy Nikkel (:tn) from comment #23)
> Comment on attachment 8456551 [details] [diff] [review]
> Part 4: Union the display port dirty area in nsSubDocumentFrame v2
> 
> Hmm, is this actually going to do what you want? The first version of this
> patch was in nsSubDocumentFrame so it affected root scroll frames. This
> version will affect only non-root scroll frames because we will use the
> AppendScrollPartsTo calls further up in the "IgnoreScrollFrame" if for root
> scroll frames with display ports.

Ignore root scroll frame is false for desktop. I guess that probably won't always be true though.

Comment 26

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> Ignore root scroll frame is false for desktop. I guess that probably won't
> always be true though.

Is it? Part 8 calls nsLayoutUtils::SetDisplayPortMargins, which calls aPresShell->SetIgnoreViewportScrolling(true).

Comment 27

[Matt Woodrow (:mattwoodrow)]

Part 7 disables that for desktop. It'll stop being true when we enable APZ for desktop though I guess, so I should try fix it differently.

Comment 28

[Timothy Nikkel (:tnikkel)]

Oops, forgot about that.

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Subtract from the visible region in nsDisplayScrollLayer if we're not using APZ v3

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Union the display port dirty area in nsSubDocumentFrame v3

Comment 31

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8460672 [details] [diff] [review]
Part 6: Subtract from the visible region in nsDisplayScrollLayer if we're not using APZ v3

boundedRect will contain the content in the display port, so could be larger than the scrollport. Can we just use GetBounds to get the scrollport area to subtract from?

Comment 32

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Subtract from the visible region in nsDisplayScrollLayer if we're not using APZ v4

Oops, misread boundedRect and thought that's what it was doing already.

Comment 33

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5cfc71b3d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ced57523b1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/23f8d7d74536
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0c799f15bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9304ad42cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/50623b494578
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9922138347

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a5cfc71b3d3
https://hg.mozilla.org/mozilla-central/rev/6ced57523b1a
https://hg.mozilla.org/mozilla-central/rev/23f8d7d74536
https://hg.mozilla.org/mozilla-central/rev/5b0c799f15bf
https://hg.mozilla.org/mozilla-central/rev/bd9304ad42cb
https://hg.mozilla.org/mozilla-central/rev/50623b494578
https://hg.mozilla.org/mozilla-central/rev/0a9922138347

Comment 35

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Hi Matt, is this something that either QA should be testing and verifying, or are there any automated tests for these changes? Thanks!

Comment 36

[Matt Woodrow (:mattwoodrow)]

(In reply to Liz Henry :lizzard from comment #35)
> Hi Matt, is this something that either QA should be testing and verifying,
> or are there any automated tests for these changes? Thanks!

No, this is just a performance thing for desktop tiling, which isn't enabled yet.