1370682 - Implement rounded corners clipping in webrender

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Our layers API does not have a way to specify clips with rounded rectangles. We only have ClipRect and a way to set a mask layer. So if you have an active layer with a rounded clip, we create a mask layer for that.

We'd like to avoid the mask layers with WebRender. WebRender allows us to set clips with rounded corners, so we should make use of that functionality.

As a first step, we probably need to add an API to Layer that allows us to specify rounded rectangle clips. Then we need to teach FrameLayerBuilder to have a mode that turns DisplayItemClips with rounded rectangles into rounded rectangle layer clips. And then we need to teach WebRenderLayerManager to turn those clips into rounded WebRender clips.

There's a sketch of the first two pieces in the advanced-layers repo.

This change should improve our performance on http://output.jsbin.com/surane/quiet where we currently create and destroy tons of mask layer images.

Comment 1

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #0)
> There's a sketch of the first two pieces in the advanced-layers repo.

This code is at https://hg.mozilla.org/users/bschouten_mozilla.com/advanced-layers/rev/19535bcdfdea and at https://hg.mozilla.org/users/bschouten_mozilla.com/advanced-layers/file/f418dadba7b2/layout/painting/FrameLayerBuilder.cpp#l6355

However, it only handles the case of a single rounded corner clip. Unfortunately, the clip can be an intersection of multiple rounded corner clips. The API should handle that case without falling back to a much slower path.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

AL never made use of this API, so it didn't make it over to bug 1365879. But if it comes back in some form it'd be great if layers-based compositors could make use of it as well.

Comment 3

[Matt Woodrow (:mattwoodrow)]

If you wanted, you could make FLB always set the rounded rect clips, and move mask layer creation to be something that happens in the LayerManager/Compositor (with WR implementing it natively instead).

That'd be nice since it'd reduce the FLB complexity instead of adding to it. More short term work though.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> If you wanted, you could make FLB always set the rounded rect clips, and
> move mask layer creation to be something that happens in the
> LayerManager/Compositor (with WR implementing it natively instead).
> 
> That'd be nice since it'd reduce the FLB complexity instead of adding to it.
> More short term work though.

I think I prefer this approach.

To get some nice things immediately I'm thinking of just not translating the mask layer for display item layers and building the webrender clip from the display item directly. See bug 1375059

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

What's this story like in the layers-free world?

Comment 6

[Markus Stange [:mstange]]

layers-free is getting basic clipping in bug 1386483 but it currently ignores rounded corners. So rounded corners clipping still needs to be hooked up somewhere; it could be done in this bug but it won't go in FrameLayerBuilder.

Comment 7

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

(In reply to Markus Stange [:mstange] from comment #6)
> layers-free is getting basic clipping in bug 1386483 but it currently
> ignores rounded corners. So rounded corners clipping still needs to be
> hooked up somewhere; it could be done in this bug but it won't go in
> FrameLayerBuilder.

I'm working on this, will use this bug for it.

Comment 8

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

Attachment: Bug 1370682 - Allow passing in LayoutDeviceSize to wr::ToBorderRadius for convenience.

There's no real reason to restrict this to LayerSize, since we use
use Layer and LayoutDevice coordinate spaces interchangeably for
webrender. With the templating of the function the compiler doesn't know
what type to use for {0, 0} so I added a new function to deal with that.

Review commit: https://reviewboard.mozilla.org/r/166800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166800/

Comment 9

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

Attachment: Bug 1370682 - Send DisplayItemClip's RoundedRect clips to WR.

This was spot-tested by taking the
layout/reftests/border-radius-clipping-1.html testcase, removing the
border, and fiddling with the border-radius values. Without this patch,
the div always appears with rectangular corners but with this patch the
border-radius value is respected correctly.

Review commit: https://reviewboard.mozilla.org/r/166802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166802/

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8895593 [details]
Bug 1370682 - Allow passing in LayoutDeviceSize to wr::ToBorderRadius for convenience.

https://reviewboard.mozilla.org/r/166800/#review172330

Looks good to me.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8895593 [details]
Bug 1370682 - Allow passing in LayoutDeviceSize to wr::ToBorderRadius for convenience.

https://reviewboard.mozilla.org/r/166800/#review172348

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8895594 [details]
Bug 1370682 - Send DisplayItemClip's RoundedRect clips to WR.

https://reviewboard.mozilla.org/r/166802/#review172350

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5936d7c91ba
Allow passing in LayoutDeviceSize to wr::ToBorderRadius for convenience. r=jrmuizel,mstange
https://hg.mozilla.org/integration/autoland/rev/972371dc461b
Send DisplayItemClip's RoundedRect clips to WR. r=mstange

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage: missing return statement at WebRenderTypes.h:361:

https://hg.mozilla.org/integration/autoland/rev/7376d8576c0214a2f1fd74a5b8a9a9badfc3b463
https://hg.mozilla.org/integration/autoland/rev/a02b6d2c495b811d87fb681ec87c357bd9d616b7

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=972371dc461b919ef2f19e7d1db85d32124ee391&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122322306&repo=autoland

> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/webrender/WebRenderTypes.h:361:1: error: no return statement in function returning non-void [-Werror=return-type]

Comment 16

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

I really need to change my local mozconfig to build with -Werror...

Comment 17

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

Comment on attachment 8895593 [details]
Bug 1370682 - Allow passing in LayoutDeviceSize to wr::ToBorderRadius for convenience.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166800/diff/1-2/

Comment 18

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

Comment on attachment 8895594 [details]
Bug 1370682 - Send DisplayItemClip's RoundedRect clips to WR.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166802/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d403daf53fea
Allow passing in LayoutDeviceSize to wr::ToBorderRadius for convenience. r=jrmuizel,mstange
https://hg.mozilla.org/integration/autoland/rev/7543262c4791
Send DisplayItemClip's RoundedRect clips to WR. r=mstange

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d403daf53fea
https://hg.mozilla.org/mozilla-central/rev/7543262c4791