Unify text-shadows with the blur filter. #2556

glennw · 2018-03-22T06:48:05Z

This removes any special code for text shadows. A text shadow is
now simply a Picture with text runs and/or line decorations that
has a blur filter applied to it.

This is possible due to the recent work related to removing the line
decoration and cached text run primitives.

This greatly simplifies the shadow code, and a lot of the functionality
of the text shadows (e.g. fast path shadows) just fall out of the
existing Picture functionality for blur filters.

Next step is to expand the primitive types that can run through the
shadow paths, which will then allow us to unify the drop-shadow
filter to be a simple shadow + blur filter.

NOTE: The PictureKind enum now has only a single kind (Image). I could
have removed that in this PR, and added the fields to Picture
directly. However, that will result in a lot of indentation
changes. So I'll do that as a follow up PR to make this one
easier to review.

This change is

glennw · 2018-03-22T06:48:19Z

This PR also includes #2552. It may be easier to review this PR after that lands.

The try run has several orange items, but I think they are all OK:

Wr5 is 2x new PASS.
R3 is 1x new PASS.
R4 is 2x new PASS.
rust item looks to be an unrelated build failure.
mda2 looks like an unrelated intermittent.
wpt2 looks like an unrelated intermittent.

@staktrace, does that look correct to you?

r? @mrobinson or @kvark

glennw · 2018-03-22T06:48:36Z

With try run link:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b31e1906983c87cc73b8b9496d1de0036650eb9

staktrace · 2018-03-22T12:24:40Z

@glennw I agree with your try push assessment, I think this is fine to land.

staktrace · 2018-03-22T16:11:15Z

Surprise! The unexpected-pass results are all actually from turning off mipmaps, not from this PR. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b114b7b9e9565b2f03470931c6574040e354aa10

glennw · 2018-03-22T21:00:22Z

@staktrace I thought that might actually be the case here :)

glennw · 2018-03-22T21:03:25Z

I've rebased this on top of master, now down to one commit with +269 / -410 lines :)

Gankra · 2018-03-26T18:53:02Z

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

webrender/src/display_list_flattener.rs, line 179 at r1 (raw file):

    /// when the shadow stack is empty.
    pending_shadow_contents: Vec<(PrimitiveIndex, ScrollNodeAndClipChain, LayerPrimitiveInfo)>,

🎉

webrender/src/display_list_flattener.rs, line 941 at r1 (raw file):

                    .iter()
                    .map(|cs| cs.offset(&shadow.offset))
                    .collect();

I don't recall us handling local clip sources around here before, why now?

webrender/src/display_list_flattener.rs, line 957 at r1 (raw file):

        }

        if container.is_visible() {

Hmm, is it actually possible for a primitive to be in a non-visible container while the shadow isn't? (is a shadow ever in a different container from its contents?)

PrimitiveContainers are a new notion to me so idk what they represent (kvark says they're the new Picture, but I still see Pictures are alive and well?)

Is it accurate to say they're just temporary metadata used during flattening? so e.g. the fact that we do tons of non-recycled calls to create_shadow is "fine"? If so I'd prefer to call create_shadow something that indicates it's uh, softer, than it sounds. with_shadow?

webrender/src/display_list_flattener.rs, line 1419 at r1 (raw file):

        // Create a picture that the shadow primitives will be added to. If the
        // blur radius is 0, the code in Picture::prepare_for_render will
        // detect this and mark the picture do be drawn directly into the

do -> to

webrender/src/display_list_flattener.rs, line 1449 at r1 (raw file):

    pub fn pop_all_shadows(&mut self) {
        assert!(self.shadow_stack.len() > 0, "popped shadows, but none were present");
        self.shadow_stack.clear();

😭

webrender/src/picture.rs, line 193 at r1 (raw file):

                        let inflate_size = blur_radius * BLUR_SAMPLE_SCALE;
                        local_content_rect.inflate(inflate_size, inflate_size)
                    }

Why is FilterOp::Blur handling deferred to prepare_prim_for_render, but not e.g. DropShadow?

webrender/src/tiling.rs, line 409 at r1 (raw file):

                                        task_id,
                                    });
                                }

Why isn't this just if let PictureKind::Image { frame_output_pipeline_id: Some(pipeline_id), .. } = pic.kind { self.outputs.push(...) }?

wrench/reftests/text/blurred-shadow-local-clip-rect-ref.png, line 0 at r1 (raw file):
All the content seems to be shifting down a ~pixel here. That seems suspicious?

wrench/reftests/text/shadow-clip.yaml, line 15 at r1 (raw file):

          type: "shadow"
          bounds: [0, 0, 200, 200]
          blur-radius: 2

why was this necessary?

Comments from Reviewable

Gankra · 2018-03-26T18:54:45Z

r=me with comments addressed (mostly just questions to make sure I understand what this does)

glennw · 2018-03-26T21:26:25Z

Review status: all files reviewed at latest revision, 8 unresolved discussions.

webrender/src/display_list_flattener.rs, line 941 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

I don't recall us handling local clip sources around here before, why now?

Because we now draw text decorations as a clip source - this has only landed recently.

webrender/src/display_list_flattener.rs, line 957 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

Hmm, is it actually possible for a primitive to be in a non-visible container while the shadow isn't? (is a shadow ever in a different container from its contents?)

PrimitiveContainers are a new notion to me so idk what they represent (kvark says they're the new Picture, but I still see Pictures are alive and well?)

Is it accurate to say they're just temporary metadata used during flattening? so e.g. the fact that we do tons of non-recycled calls to create_shadow is "fine"? If so I'd prefer to call create_shadow something that indicates it's uh, softer, than it sounds. with_shadow?

A PrimitiveContainer is just an enum used to handle pushing all of the primitive types. In this case, the is_visible just checks if alpha > 0, which is what we used to do manually in the add_text and add_line functions.

webrender/src/display_list_flattener.rs, line 1419 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

do -> to

Done.

webrender/src/display_list_flattener.rs, line 1449 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

😭

Is that a good reaction, or an issue?

webrender/src/picture.rs, line 193 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

Why is FilterOp::Blur handling deferred to prepare_prim_for_render, but not e.g. DropShadow?

Because the DropShadow code is much broken. I'm working on fixing the drop shadow code as a follow up patch.

webrender/src/tiling.rs, line 409 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

Why isn't this just if let PictureKind::Image { frame_output_pipeline_id: Some(pipeline_id), .. } = pic.kind { self.outputs.push(...) }?

I got a rustc compile error, since there's just the one PictureKind now (I'm intending to remove that enum as a follow up, but left it here for now to avoid a heap of indentation changes).

wrench/reftests/text/blurred-shadow-local-clip-rect-ref.png, line at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

All the content seems to be shifting down a ~pixel here. That seems suspicious?

I didn't look into that diff too closely, since the try run passed. I can try to find the exact cause though, if you like?

wrench/reftests/text/shadow-clip.yaml, line 15 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

why was this necessary?

A blur radius of 1 was producing a fractional offset in the primitive, due to the halving of the blur radius, and then scaling by 3 for the blur range. This was producing pixel perfect results on all the hardware I tested, but OSMesa seemed to be rounding it incorrectly, causing the test to fail. Since it only affected OSMesa, and the blur radius being 2 doesn't affect what this test is checking, I used a whole number to work around that for now.

Comments from Reviewable

Gankra · 2018-03-26T23:09:44Z

Review status: 11 of 12 files reviewed at latest revision, 3 unresolved discussions.

webrender/src/display_list_flattener.rs, line 1449 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Is that a good reaction, or an issue?

tears of joy!

webrender/src/tiling.rs, line 409 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

I got a rustc compile error, since there's just the one PictureKind now (I'm intending to remove that enum as a follow up, but left it here for now to avoid a heap of indentation changes).

Yes that's why I moved the Some() match inline, which makes the whole pattern fallible.

wrench/reftests/text/blurred-shadow-local-clip-rect-ref.png, line at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

I didn't look into that diff too closely, since the try run passed. I can try to find the exact cause though, if you like?

It's also possible github is just being dumb here. But wait, how did you get a passing try and a diff?

Comments from Reviewable

This removes any special code for text shadows. A text shadow is now simply a Picture with text runs and/or line decorations that has a blur filter applied to it. This is possible due to the recent work related to removing the line decoration and cached text run primitives. This greatly simplifies the shadow code, and a lot of the functionality of the text shadows (e.g. fast path shadows) just fall out of the existing Picture functionality for blur filters. Next step is to expand the primitive types that can run through the shadow paths, which will then allow us to unify the drop-shadow filter to be a simple shadow + blur filter. NOTE: The PictureKind enum now has only a single kind (Image). I could have removed that in this PR, and added the fields to Picture directly. However, that will result in a lot of indentation changes. So I'll do that as a follow up PR to make this one easier to review.

glennw · 2018-03-26T23:17:31Z

Review status: 10 of 12 files reviewed at latest revision, 2 unresolved discussions.

webrender/src/tiling.rs, line 409 at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

Yes that's why I moved the Some() match inline, which makes the whole pattern fallible.

Ah, I misread your comment. Fixed now.

wrench/reftests/text/blurred-shadow-local-clip-rect-ref.png, line at r1 (raw file):

Previously, Gankro (Alexis Beingessner) wrote…

It's also possible github is just being dumb here. But wait, how did you get a passing try and a diff?

Oh, I meant I hadn't looked too closely at the wrench reference image difference, since the gecko try had passed. I can take a closer look at this difference before we merge though, if you prefer, to see if it's actually a bug with the previous image or the new one?

Comments from Reviewable

Gankra · 2018-03-26T23:24:40Z

Review status: 10 of 12 files reviewed at latest revision, 1 unresolved discussion.

wrench/reftests/text/blurred-shadow-local-clip-rect-ref.png, line at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Oh, I meant I hadn't looked too closely at the wrench reference image difference, since the gecko try had passed. I can take a closer look at this difference before we merge though, if you prefer, to see if it's actually a bug with the previous image or the new one?

I'd appreciate it, but not a blocker if you think you have better stuff to do.

Comments from Reviewable

glennw · 2018-03-26T23:31:01Z

I'll take a look today, and see what causes the difference. r=you if it turns out to be expected? (otherwise if there's a bug I'll put up an extra commit + gecko try).

Gankra · 2018-03-27T00:05:04Z

yep, r=me with the image investigated

glennw · 2018-03-27T01:39:03Z

@gankro You're quite right - that reference image was incorrectly clipped. I guess we don't have Gecko test coverage for that. Pushed a commit that restores the local rect inflate for blurs, will kick off a gecko try shortly.

glennw · 2018-03-27T02:03:01Z

Try run here https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cabd0b6e5a2c58b75f4fd20d55a0c782166b97 has one new PASS and one new FAIL.

I'll investigate the failure today.

glennw · 2018-03-27T02:17:06Z

Hmmm, seems to not reproduce locally so far 😞

It's needed when the contents of the blur are clipped. Also update the reference images with minor differences.

glennw · 2018-03-27T04:57:19Z

Latest try run (not quite complete yet):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51d30f477a9adbf31ba1e3053275b5177a0e06e

There is a new PASS in R7. The other orange results look unrelated, so I think this is ready to merge now.

glennw · 2018-03-27T05:36:40Z

Try run looks good now.

@bors-servo r=Gankro 🚀

bors-servo · 2018-03-27T05:36:41Z

📌 Commit 2afe3d0 has been approved by Gankro

bors-servo · 2018-03-27T05:36:44Z

⌛ Testing commit 2afe3d0 with merge 9893ea1...

Unify text-shadows with the blur filter. This removes any special code for text shadows. A text shadow is now simply a Picture with text runs and/or line decorations that has a blur filter applied to it. This is possible due to the recent work related to removing the line decoration and cached text run primitives. This greatly simplifies the shadow code, and a lot of the functionality of the text shadows (e.g. fast path shadows) just fall out of the existing Picture functionality for blur filters. Next step is to expand the primitive types that can run through the shadow paths, which will then allow us to unify the drop-shadow filter to be a simple shadow + blur filter. NOTE: The PictureKind enum now has only a single kind (Image). I could have removed that in this PR, and added the fields to Picture directly. However, that will result in a lot of indentation changes. So I'll do that as a follow up PR to make this one easier to review.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2556)

bors-servo · 2018-03-27T06:20:02Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: Gankro
Pushing 9893ea1 to master...

glennw force-pushed the remove-text-shadow3 branch from e89ed9b to 5b8a81e Compare March 22, 2018 21:02

glennw force-pushed the remove-text-shadow3 branch from 5b8a81e to 3f20fa6 Compare March 26, 2018 21:26

glennw force-pushed the remove-text-shadow3 branch from 3f20fa6 to e36c071 Compare March 26, 2018 23:15

Restore the local rect inflation for blurs.

2afe3d0

It's needed when the contents of the blur are clipped. Also update the reference images with minor differences.

glennw force-pushed the remove-text-shadow3 branch from 9bd1296 to 2afe3d0 Compare March 27, 2018 04:01

bors-servo merged commit 2afe3d0 into servo:master Mar 27, 2018

bors-servo mentioned this pull request Mar 27, 2018

Decompose repeated images and gradients into brush segments in most cases. #2572

Closed

4 tasks

glennw deleted the remove-text-shadow3 branch March 27, 2018 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify text-shadows with the blur filter. #2556

Unify text-shadows with the blur filter. #2556

glennw commented Mar 22, 2018 •

edited by larsbergstrom

glennw commented Mar 22, 2018

glennw commented Mar 22, 2018

staktrace commented Mar 22, 2018

staktrace commented Mar 22, 2018

glennw commented Mar 22, 2018

glennw commented Mar 22, 2018

Gankra commented Mar 26, 2018

Gankra commented Mar 26, 2018

glennw commented Mar 26, 2018

Gankra commented Mar 26, 2018

glennw commented Mar 26, 2018

Gankra commented Mar 26, 2018

glennw commented Mar 26, 2018

Gankra commented Mar 27, 2018 •

edited

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

Unify text-shadows with the blur filter. #2556

Unify text-shadows with the blur filter. #2556

Conversation

glennw commented Mar 22, 2018 • edited by larsbergstrom

glennw commented Mar 22, 2018

glennw commented Mar 22, 2018

staktrace commented Mar 22, 2018

staktrace commented Mar 22, 2018

glennw commented Mar 22, 2018

glennw commented Mar 22, 2018

Gankra commented Mar 26, 2018

Gankra commented Mar 26, 2018

glennw commented Mar 26, 2018

Gankra commented Mar 26, 2018

glennw commented Mar 26, 2018

Gankra commented Mar 26, 2018

glennw commented Mar 26, 2018

Gankra commented Mar 27, 2018 • edited

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

glennw commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

glennw commented Mar 22, 2018 •

edited by larsbergstrom

Gankra commented Mar 27, 2018 •

edited