Closed Bug 1593574 Opened 5 months ago Closed 3 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: gw, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

No description provided.

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).

Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(jmuizelaar)

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...)

Flags: needinfo?(jmuizelaar)
Flags: needinfo?(nical.bugzilla)

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.

Assignee: nobody → jmuizelaar
Priority: -- → P3

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

Flags: needinfo?(jmuizelaar)

(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.

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?

Flags: needinfo?(aosmond)

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).

Flags: needinfo?(aosmond)

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

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

Flags: needinfo?(gwatson)

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?

Flags: needinfo?(gwatson)

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.

(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.

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.

Flags: needinfo?(jmuizelaar)

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

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.

Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e2326c4433c
Note that themed backgrounds are snapped. r=mattwoodrow
Attachment #9106095 - Attachment description: Bug 1593574. Try to be more opaque → Bug 1593574. Create an opaque surface for fallback when possible.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffe6b31202b5
Create an opaque surface for fallback when possible. r=mattwoodrow
Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.