1414125 - Shadows Mishandle Clips on WebRender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Tommy Kuo (away forever...)]

When Bug 1399310 is landed, the shadow seems to be clipped with the wrong region. The issue on GitHub is [1].

[1]: https://github.com/servo/webrender/issues/1928

Comment 1

[Ethan Lin[:ethlin]]

Attachment: testcase

Comment 2

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1414125 - Use local clip instead of ClipNode in TextDrawTarget;

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

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

The root cause of this bug is that we push a clip when drawing the decoration line under the text selection status to cut the decoration line with the right length[1].
However, in webrender, shadow elements inherit the same clip with the original elements which leads the shadow to apply the wrong clip region when doing fast-shadow.

There are several ways to fix this problem:

1. Skip the fast shadow in webrender if the shadow has vertical(horizontal) offset when the text is in vertical(horizontal) format. This might affect the performance of webrender since shadow with offset might be very common.

2. Remove the clip region for the decoration line, draw the decoration line with the exact length the text segment needs. With this solution, we don't have to change the behavior in webrender, but the clip problem in webrender remains.

3. Adjust the local clip rect of PrimitiveInfo when adding shadow line primitive and apply the clip outside of the shadow to it. it seems the right in this text decoration case, but I am not sure if this is correct behavior.[2]

Glenn, Alexis, do you have any thoughts about these proposals?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1399310
[2] https://reviewboard.mozilla.org/r/210856/diff/1#index_header

Comment 4

[Aria Beingessner [:Gankra]]

Yeah glenn and I were discussing this before the holidays but I don't think we ever came to a conclusion. I'm personally a fan of some version of (3) as we also need it for Bug 1403261. Note however that whenever you get one of these weird clips, it was pushed *inside* the shadow, so that gives you a hint it should be adjusted. However this information is lost to the backend, as "push clip" isn't an actual display item type.

Comment 5

[Kevin Chen[:kechen] (UTC + 8)]

I've created a pull request of proposal 3 on webrender github[1].

[1] https://github.com/servo/webrender/pull/2278

Comment 6

[Glenn Watson [:gw]]

There's ongoing discussion in the GH bug. From discussing with Alexis, it sounds like the "correct" solution is probably making use of the local clip rect functionality in the API for this. That would make it quite simple for WR to adjust the local clip rects internally based on the current shadow context, without introducing any complexity involved in creating extra clip-scroll nodes. I believe Gankro is planning to discuss this with Martin this week, to work out if that seems reasonable to all.

Comment 7

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Use local clip instead of ClipNode in TextDrawTarget.

Hello Alexis,

Does the fix meet the proposal you mentioned in [1]?
If this patch is correct, I will add some reftests for wrench and move the WR part of codes to the WR repo.


[1] https://github.com/servo/webrender/pull/2278

Comment 8

[Aria Beingessner [:Gankra]]

Comment on attachment 8942904 [details] [diff] [review]
Use local clip instead of ClipNode in TextDrawTarget.

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

Right approach, just needs tweaks.

::: gfx/webrender/src/frame_builder.rs
@@ +1357,3 @@
>              let prim_index = self.create_primitive(
>                  &info,
>                  Vec::new(),

This should be submitted as a PR to webrender and then upstreamed in a webrender update, not submitted here.

(but yeah this seems right)

::: layout/generic/TextDrawTarget.h
@@ +287,4 @@
>  
>    // Computed facts
>    wr::LayerRect mBoundsRect;
> +  LayoutDeviceRect mClipRect;

Any reason to "cache" this when it needs to go through ToRelativeLayoutRect anyway? I'd rather just have a method for like `ClipRect()` that grabs the back of the stack and applies the ToRelativeLayoutRect transform.

@@ +287,5 @@
>  
>    // Computed facts
>    wr::LayerRect mBoundsRect;
> +  LayoutDeviceRect mClipRect;
> +  std::vector<LayoutDeviceRect> mClipStack;

This should be changed to nsTArray, and not std::vector (it's safer)

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8940627 [details]
Bug 1414125 - Use local clip instead of ClipNode in TextDrawTarget;

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

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

Here is the try result[1].
With this patch and the fix in [2], the result seems good.
I've already removed the fail-if tags which cause the two unexpected-passes in the last patch.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf264fc9dca734cbd5ae96a0869d5533c85472fd&selectedJob=156765226
[2] https://github.com/servo/webrender/pull/2278

Comment 11

[Aria Beingessner [:Gankra]]

Comment on attachment 8940627 [details]
Bug 1414125 - Use local clip instead of ClipNode in TextDrawTarget;

https://reviewboard.mozilla.org/r/210856/#review219344

Comment 12

[Aria Beingessner [:Gankra]]

We should split the reftest changes out so that we can do the following:

* land the local rect changes (no effect)
* land the wr changes upstream
* in the wr update, kats applies the reftest changes

Comment 13

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

The WR changes by themselves didn't affect any reftest results. So we can land this after that WR update gets merged, and no need to split out the reftest changes.

Comment 14

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

Rebased and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cffd83d29b0cc10b1b8de96528965f020dfb77ab

Comment 15

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

With moar fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0b72d4372fcc0e329decd5b1e206cbb912ea9e7

Comment 16

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

That looks good. Lee, any objections to me landing this?

Comment 17

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

s/Lee/Gankro/, whoops!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9032b4b612
Use local clip instead of ClipNode in TextDrawTarget; r=Gankro

Comment 19

[Dorel Luca [:dluca]]

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