1593574 - Scroll bar images don't have opacity marked correctly in webrender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Comment 1

[Glenn Watson [:gw]]

I'm not too sure how these are implemented in Gecko with WR - are they blobs? Would it be feasible to determine when these are opaque and tag them as such? (this would significantly improve the efficiency of tile occlusion culling I am working on).

Comment 2

[Jeff Muizelaar [:jrmuizel]]

They are not implemented as blobs and currently all non-blob fallback is marked as transparent. I'll attach a completely untested patch that should make them opaque if the display item thinks it's opaque (I don't know if scrollbars actually know...)

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1593574. Create an opaque surface for fallback when possible.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Mstange points out https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/widget/windows/nsNativeThemeWin.cpp#2702-2710 which suggests that this patch has a decent chance of working.

Comment 5

[Glenn Watson [:gw]]

I tested locally with this patch and it works! (at least, it makes the occlusion culling work more efficiently, I don't know if it breaks anything else).

On my 4k screen, this further reduced the allocated tile count from 34 down to 29 (by occluding all background tiles that the scrollbars cover).

So, if this passes a try run, we should definitely get it merged.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

This fails on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4fae3fa5c1bfe61f4087798dbeb5f1a799319a3&selectedJob=274638024

Comment 7

[Glenn Watson [:gw]]

I think this may be having a reasonable impact on performance on Windows + DirectComposite mode. Do we know what causes the try failures?

Comment 8

[Markus Stange [:mstange]]

(In reply to Glenn Watson [:gw] from comment #7)

Do we know what causes the try failures?

There's a black pixel row at the bottom of a scrollbar thumb. It looks like theme rendering isn't filling the entire surface. Maybe it's snapping the fill rect differently.

Comment 9

[Glenn Watson [:gw]]

Jeff, Andrew, this could be a significant memory (and perhaps performance) win, if we're able to solve it. Any thoughts on how much work would be to get this passing on try?

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Okay, so it is the slow path, and presumably this ClearRect

https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/gfx/layers/wr/WebRenderCommandBuilder.cpp#2019

causes us to draw the black line.

We intersect the opaque rect with the paint bounds, but realistically, the actual surface area created is defined by the dtRect, which is snapped to outside pixels. Presumably this is to be conservative -- this means we might get an extra row/column on any/all sides if the items inside snap differently when they paint.

As for a fix, my expectation then would be you can only be opaque if the opaque rect contains the dtRect, not the paintBounds. It is possible the dtRect will need to be calculated more accurately as well to maximize the odds we don't add that extra row (that may be non-trivial).

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Let's see about confirming that theory: https://treeherder.mozilla.org/#/jobs?repo=try&revision=950ff17de2134bd896071c01909ce8e28622adbf

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

That appears to work, although it may mean we are overly conservative and are never opaque :).

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Glenn, the scrollbars on Mac are transparent. Do we have a plan to deal with this problem there?

Comment 14

[Glenn Watson [:gw]]

I don't think this is an issue on Mac.

The opaque background rect on Mac should extend to cover the entire window, which will allow those tiles to be occluded. It should only be an issue on Linux / Windows, where the opaque background rect is clipped to the visible area excluding the scrollbar region.

I think this was the case when I last checked, but it would be worth verifying. If you enable picture caching debugging on a simple scrollable page, do you see any fixed position tiles in the debugger or are they all occluded?

Comment 15

[Glenn Watson [:gw]]

I did some measurements with the original patch applied, and it gives a significant improvement to time spent in DWM process when DirectComposition is enabled (~29% -> 24% on a 4k screen with HD530 GPU). So it's definitely worth prioritizing this and finding a solution to get it over the line.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to Andrew Osmond [:aosmond] from comment #10)

Okay, so it is the slow path, and presumably this ClearRect

https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/gfx/layers/wr/WebRenderCommandBuilder.cpp#2019

causes us to draw the black line.

We intersect the opaque rect with the paint bounds, but realistically, the actual surface area created is defined by the dtRect, which is snapped to outside pixels. Presumably this is to be conservative -- this means we might get an extra row/column on any/all sides if the items inside snap differently when they paint.

As for a fix, my expectation then would be you can only be opaque if the opaque rect contains the dtRect, not the paintBounds. It is possible the dtRect will need to be calculated more accurately as well to maximize the odds we don't add that extra row (that may be non-trivial).

I'm concerned that with this approach that we'll sometimes have a transparent scrollbar. I'd prefer something more reliable. I'll think about how we can get that.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I looked into this some more and scrollbars which are represented as nsDisplayThemedBackground items decide their opaqueness based on a single bool: https://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#4971 we then compute the size of the surface they draw to in two completely different ways. I wonder if instead we can just ask the nsDisplayThemedBackground item the size it wants and only create a surface of that size.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

My newest version of the patch passes without reftest failures. https://treeherder.mozilla.org/#/jobs?repo=try&revision=60fdcf8cf3475afb1e56d35dee30d7e970756757

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1593574. Note that themed backgrounds are snapped.

When drawing themed backgrounds we snap them therefore we can expose
that in GetOpaqueRegion(). This will help WebRender fallback choose
an appropriately sized surface for drawing them.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e2326c4433c
Note that themed backgrounds are snapped. r=mattwoodrow

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/3e2326c4433c

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffe6b31202b5
Create an opaque surface for fallback when possible. r=mattwoodrow

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ffe6b31202b5