Closed Bug 1031107 Opened 5 years ago Closed 5 years ago

Always use display ports when tiling

Categories

(Core :: Layout, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(8 files, 5 obsolete files)

5.19 KB, patch
kats
: review+
Details | Diff | Splinter Review
1.47 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.33 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.65 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.61 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.59 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.07 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
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.
To clarify, I mean when using tiling on desktop and we don't have APZ.
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.
Assignee: nobody → matt.woodrow
Attachment #8446897 - Flags: review?(bugmail.mozilla)
When doing hit testing with IGNORE_ROOT_SCROLL_FRAME, we expect to build display items that outside of the viewport.
Attachment #8446898 - Flags: review?(tnikkel)
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.
Attachment #8446900 - Flags: review?(tnikkel)
When we have non-overlay scrollbars, their area isn't included within the display port bounds. We still need to include them though.
Attachment #8446902 - Flags: review?(tnikkel)
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.
Attachment #8446903 - Flags: review?(roc)
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.
Attachment #8446915 - Flags: review?(tnikkel)
We need this so we still get scrollbars on desktop.
Attachment #8446917 - Flags: review?(tnikkel)
Attachment #8446898 - Flags: review?(tnikkel) → review+
Attachment #8446900 - Flags: review?(tnikkel) → review+
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 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 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 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.
Attachment #8446918 - Flags: review?(tnikkel) → review+
(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 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 ?
Attachment #8446917 - Flags: review?(tnikkel) → review+
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
Attachment #8446897 - Flags: review?(bugmail.mozilla) → review+
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.
It looks like these patches only change behaviour when AsyncPanZoomEnabled() is false, so that should be good then.
Interestingly, it appears that gfxPrefs::AsyncPanZoomEnabled() is false for Android and B2G desktop builds on tinderbox. Is that expected?
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.
Attachment #8446915 - Flags: review?(tnikkel)
Attachment #8446902 - Flags: review?(tnikkel)
Attachment #8446902 - Attachment is obsolete: true
Attachment #8456551 - Flags: review?(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 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.
(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.
(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).
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.
Oops, forgot about that.
Attachment #8456552 - Attachment is obsolete: true
Attachment #8456552 - Flags: review?(tnikkel)
Attachment #8460672 - Flags: review?(tnikkel)
Attachment #8456551 - Attachment is obsolete: true
Attachment #8456551 - Flags: review?(tnikkel)
Attachment #8460686 - Flags: review?(tnikkel)
Attachment #8460686 - Flags: review?(tnikkel) → review+
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?
Oops, misread boundedRect and thought that's what it was doing already.
Attachment #8460672 - Attachment is obsolete: true
Attachment #8460672 - Flags: review?(tnikkel)
Attachment #8461218 - Flags: review?(tnikkel)
Attachment #8461218 - Flags: review?(tnikkel) → review+
Hi Matt, is this something that either QA should be testing and verifying, or are there any automated tests for these changes? Thanks!
Flags: needinfo?(matt.woodrow)
Flags: in-testsuite?
Depends on: 1043961
(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.
Flags: needinfo?(matt.woodrow)
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.