Add drop shadow support. #2091

mephisto41 · 2017-11-23T09:15:45Z

The major changes here are using hash map to store render_task_id in PicturePrimitive. In order to support drop shadow, we need add a primitive to multiple pictures. So, if we only stored one render_task_id, the result is wrong. Hence, I use a hash map to store multiple render_task_ids and use parent PrimitiveIndex as key.

Another change is brush_image now has third configuration which is take color target as source, but only use alpha of this target and multiply color as result. This is because drop shadow is similar to box shadow which take a alpha mask and a color, but drop shadow's alpha mask is the alpha channel of color target.

This change is

bors-servo · 2017-11-24T04:15:04Z

☔ The latest upstream changes (presumably #2094) made this pull request unmergeable. Please resolve the merge conflicts.

glennw · 2017-11-24T10:06:21Z

I have some concerns over the hashmap - there's various assumptions built in that a primitive only exists in one picture. Sorry I didn't have a chance to write up more detailed notes yet - I'll write up some more info next week.

glennw · 2017-11-27T03:37:40Z

Reviewed 9 of 13 files at r1.
Review status: 9 of 13 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

glennw · 2017-11-27T03:52:49Z

Instead of having to draw the primitives in a drop shadow twice, how about something like this:

The code for a drop shadow filter is very similar to a normal blur filter - that is, a normal Picture where the render_task_id for the picture points to the render task that contains the blur result, and the blur task points to a task which draws the contents of the Picture we are drop-shadowing.

Then, the tricky part is being able to draw the normal Picture contents as well, since that will be done in a much earlier pass. However, if we extend the functionality of SourceTexture::CacheRGBA8 slightly, we can make it so that it's possible to select an intermediate surface from any previous pass. Then, we can store that contents render task in the Picture as well (we could rename the readback_render_task_id field to secondary_render_task_id perhaps). Finally, when we come to add a DropShadow picture to the batches, we first add the main render task (which has the blur result) and then another instance with the secondary render task (which is the main contents).

This would mean that we only ever have to draw the contents of a drop-shadow element once, which I think could be a good performance win in most cases.

Although it might be a little complex to implement the support for SourceTexture::CacheRGBA8 mentioned above, I think it will make the rest of the patch a lot simpler. It's also something that might come in useful in the future for implementing / optimizing some other features.

Does that sound like it would work?

mephisto41 · 2017-11-27T03:59:53Z

Sounds like a very good solution. I'll start to implement it! Thanks.

bors-servo · 2017-11-29T07:50:04Z

☔ The latest upstream changes (presumably #2126) made this pull request unmergeable. Please resolve the merge conflicts.

mephisto41 · 2017-12-01T02:51:38Z

gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=569e33fdca16263ec09031dfc06ded23afc0cfe0&selectedJob=148673156

The long-chain.html failure looks like a known issue. Even if I remove the drop-shadow effect in long-chain, the result isn't correct. Looks like the result is wrong if filters contain opacity and other effects.

mephisto41 · 2017-12-04T03:11:08Z

The long-chain.html should be fixed by #2150

mephisto41 · 2017-12-04T03:11:45Z

I think this is ready to review. r? @glennw

glennw

This generally looks great, the technique worked out quite well! Mostly just a few small style comments to address. I'd like @kvark to take a quick look over the render target pool changes too, since he has been working on that code recently.

glennw · 2017-12-04T04:56:06Z

webrender/src/internal_types.rs

@@ -45,6 +45,8 @@ pub enum SourceTexture {
    External(ExternalImageData),
    CacheA8,
    CacheRGBA8,
+    RenderTaskCacheA8(usize),


We could use a newtype here to give a bit of type safety, and more importantly, self-document what these fields are. For example:

struct RenderPassIndex(usize); ... RenderTaskCacheA8(RenderPassIndex), ...

Or something like that?

glennw · 2017-12-04T04:56:06Z

webrender/src/render_task.rs

@@ -283,6 +283,7 @@ pub struct RenderTask {
    pub children: Vec<RenderTaskId>,
    pub kind: RenderTaskKind,
    pub clear_mode: ClearMode,
+    pub pass_index: usize,


This could use the RenderPassIndex mentioned above. If we make it Option<RenderPassIndex> we can ensure that when the later code reads the value, it has been correctly assigned to a pass.

glennw · 2017-12-04T04:56:06Z

webrender/src/renderer.rs

@@ -564,6 +564,11 @@ struct SourceTextureResolver {
    /// The current cache textures.
    cache_rgba8_texture: Option<Texture>,
    cache_a8_texture: Option<Texture>,
+
+    pass_rgba8_textures: FastHashMap<usize, usize>,


We could use a stronger type here, or at least a comment to describe what this is mapping between.

glennw · 2017-12-04T04:56:06Z

webrender/src/renderer.rs

    ) {
        // If we have cache textures from previous pass, return them to the pool.
-        pool.extend(self.cache_rgba8_texture.take());
-        pool.extend(self.cache_a8_texture.take());
+        match self.cache_rgba8_texture {


This can be written as if self.cache_rgba8_texture.is_some()

glennw · 2017-12-04T04:56:06Z

webrender/src/renderer.rs

+            }
+            _ => {}
+        }
+        match self.cache_a8_texture {


glennw · 2017-12-04T04:58:09Z

Looks like this has a reftest failure on CI too, which will need investigation. It might just be a fuzziness issue that requires updating the PNG reference.

bors-servo · 2017-12-04T05:43:03Z

☔ The latest upstream changes (presumably #2146) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2017-12-04T07:44:32Z

☔ The latest upstream changes (presumably #2150) made this pull request unmergeable. Please resolve the merge conflicts.

mephisto41 · 2017-12-04T09:13:10Z

Review status: 4 of 16 files reviewed at latest revision, 5 unresolved discussions.

webrender/src/internal_types.rs, line 48 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

We could use a newtype here to give a bit of type safety, and more importantly, self-document what these fields are. For example:
struct RenderPassIndex(usize);

...
RenderTaskCacheA8(RenderPassIndex),
...
Or something like that?

Done.

webrender/src/render_task.rs, line 286 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

This could use the RenderPassIndex mentioned above. If we make it Option<RenderPassIndex> we can ensure that when the later code reads the value, it has been correctly assigned to a pass.

Done.

webrender/src/renderer.rs, line 568 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

We could use a stronger type here, or at least a comment to describe what this is mapping between.

Done.

webrender/src/renderer.rs, line 632 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

This can be written as if self.cache_rgba8_texture.is_some()

Done.

webrender/src/renderer.rs, line 639 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

Same here

Done.

Comments from Reviewable

kvark · 2017-12-04T20:11:26Z

Reviewed 4 of 13 files at r1, 3 of 11 files at r2, 8 of 9 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

webrender/src/frame_builder.rs, line 1753 at r4 (raw file):

                passes.push(RenderPass::new_off_screen(self.screen_rect.size.to_i32(), RenderPassIndex(idx)));
            }
            passes.push(RenderPass::new_main_framebuffer(self.screen_rect.size.to_i32(), RenderPassIndex(required_pass_count)));

Is this intentionally not required_pass_count -1 ? If so, needs a comment

webrender/src/render_task.rs, line 20 at r4 (raw file):

use tiling::{RenderPass, RenderTargetIndex};
use tiling::{RenderTargetKind};
use internal_types::RenderPassIndex;

nit: let's keep those imports alphabetically ordered

webrender/src/renderer.rs, line 643 at r4 (raw file):

    ) {
        // If we have cache textures from previous pass, return them to the pool.
        if self.cache_rgba8_texture.is_some() {

if let Some(texture) = self.cache_rgba8_texture.take() {...}

webrender/src/renderer.rs, line 645 at r4 (raw file):

        if self.cache_rgba8_texture.is_some() {
            self.pass_rgba8_textures.insert(
                RenderPassIndex(pass_index.0 - 1), RenderTargetPoolId(self.render_target_pool.len()));

why does this have -1?

webrender/src/renderer.rs, line 689 at r4 (raw file):

                let texture = match self.pass_rgba8_textures.get(&pass_index) {
                    Some(pool_index) => &self.render_target_pool[pool_index.0],
                    None => &self.dummy_cache_texture,

is this legal? I'd expect us to panic here

webrender/src/renderer.rs, line 4028 at r4 (raw file):

        let mut size = 512;
        let fb_width = framebuffer_size.width as i32;
        let num_layers: i32 = self.texture_resolver.render_target_pool

what is the reasoning behind moving the RT pool under TextureResolver?

webrender/src/tiling.rs, line 1806 at r4 (raw file):

    tasks: Vec<RenderTaskId>,
    dynamic_tasks: FastHashMap<RenderTaskKey, DynamicTaskInfo>,
    pass_index: RenderPassIndex,

would it be simpler to pass the index into build as opposed to storing here?

webrender/src/tiling.rs, line 1866 at r4 (raw file):

                    {
                        let task = &mut render_tasks[task_id];
                        task.pass_index = Some(self.pass_index);

render_tasks.get_mut(task_id).unwrap().pass_index = ...

wrench/reftests/filters/filter-drop-shadow.yaml, line 6 at r4 (raw file):

    - type: stacking-context
      bounds: [100, 100, 400, 400]
      filters: drop-shadow(73, 73, 20, 1, 0, 0, 1)

would be cleaner to keep the vector parts vectors, e.g. drop-shadow([73, 73], 20, [1, 0, 0, 1])
also, aren't we specifying the color in byte values everywhere else in YAML (except alpha component)?

Comments from Reviewable

mephisto41 · 2017-12-05T08:38:29Z

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

webrender/src/frame_builder.rs, line 1753 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Is this intentionally not required_pass_count -1 ? If so, needs a comment

I'll remove this line in next patch.

webrender/src/render_task.rs, line 20 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: let's keep those imports alphabetically ordered

Done.

webrender/src/renderer.rs, line 643 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if let Some(texture) = self.cache_rgba8_texture.take() {...}

Done.

webrender/src/renderer.rs, line 645 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why does this have -1?

I added a comment. This is because the cache textures that return to pool are last pass's result. So we need -1 for pass_index.

webrender/src/renderer.rs, line 689 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is this legal? I'd expect us to panic here

Not every pass generated cache_texture. For example, if a RenderTargetList didn't has any targets, we don't build a texture for them.

webrender/src/renderer.rs, line 4028 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what is the reasoning behind moving the RT pool under TextureResolver?

Because we don't need to pass RT poll when calling TexutreResolver.bind or TextureResolver.resolve.

webrender/src/tiling.rs, line 1806 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would it be simpler to pass the index into build as opposed to storing here?

Done.

webrender/src/tiling.rs, line 1866 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

render_tasks.get_mut(task_id).unwrap().pass_index = ...

Done.

wrench/reftests/filters/filter-drop-shadow.yaml, line 6 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would be cleaner to keep the vector parts vectors, e.g. drop-shadow([73, 73], 20, [1, 0, 0, 1])
also, aren't we specifying the color in byte values everywhere else in YAML (except alpha component)?

Done.

Comments from Reviewable

kvark · 2017-12-05T16:01:24Z

Reviewed 7 of 7 files at r5.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

webrender/src/render_task.rs, line 139 at r5 (raw file):

    }

    pub fn get_mut(&mut self, id: RenderTaskId) -> &mut RenderTask {

hmm, why is this needed? the caller should be able to use the array syntax

webrender/src/renderer.rs, line 689 at r4 (raw file):

Previously, mephisto41 (Morris Tseng) wrote…

Not every pass generated cache_texture. For example, if a RenderTargetList didn't has any targets, we don't build a texture for them.

Right, but we are also not calling bind() (this function) for every pass, we should only be calling it where it makes sense and guaranteed to have the texture

wrench/src/parse_function.rs, line 21 at r5 (raw file):

    }
    impl<'a> Parser<'a> {
        fn skip_whitespace_and_bracket(&mut self) {

that's not ideal to skip brackets with whitespace... perhaps we could parse the bracket-enclosed value separately as opposed to flattening the list?

Comments from Reviewable

mephisto41 · 2017-12-06T05:58:22Z

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

webrender/src/render_task.rs, line 139 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

hmm, why is this needed? the caller should be able to use the array syntax

Done.

webrender/src/renderer.rs, line 689 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Right, but we are also not calling bind() (this function) for every pass, we should only be calling it where it makes sense and guaranteed to have the texture

Ok, I got it. I added panic for it.

wrench/src/parse_function.rs, line 21 at r5 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

that's not ideal to skip brackets with whitespace... perhaps we could parse the bracket-enclosed value separately as opposed to flattening the list?

Done.

Comments from Reviewable

kvark · 2017-12-06T16:55:45Z

Thanks!
Please rebase, squash a bit, and answer my last few concerns, and we are good to go ;)

Reviewed 7 of 7 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

webrender/src/internal_types.rs, line 44 at r6 (raw file):

// native texture ID.

// XXX Remove this once RenderTaskCacheA8 is used.

can we move it to the specified variant only (as opposed to the whole enum)?

webrender/src/renderer.rs, line 689 at r4 (raw file):

Previously, mephisto41 (Morris Tseng) wrote…

Ok, I got it. I added panic for it.

Thanks! nit: the expect() is idiomatic for the case where you match Option or Result expecting it to be Some/Ok correspondingly.

webrender/src/tiling.rs, line 1862 at r6 (raw file):

                for &task_id in &self.tasks {
                    assert_eq!(render_tasks[task_id].target_kind(), RenderTargetKind::Color);
                    render_tasks.tasks.get_mut(task_id.0 as usize).unwrap().pass_index = Some(pass_index);

does array syntax work here?

wrench/src/yaml_helper.rs, line 571 at r6 (raw file):

                    let mut yaml_doc = YamlLoader::load_from_str(&str).expect("Failed to parse drop-shadow");
                    let yaml = yaml_doc.pop().unwrap();
                    Some(FilterOp::DropShadow(yaml["offset"].as_vector().unwrap(),

looks much better 👍

Comments from Reviewable

mephisto41 · 2017-12-07T02:49:24Z

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

webrender/src/internal_types.rs, line 44 at r6 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we move it to the specified variant only (as opposed to the whole enum)?

Done.

webrender/src/renderer.rs, line 689 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Thanks! nit: the expect() is idiomatic for the case where you match Option or Result expecting it to be Some/Ok correspondingly.

Thanks for suggestion. I use expect for this.

webrender/src/tiling.rs, line 1862 at r6 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

does array syntax work here?

Ah, array syntax works!!

Comments from Reviewable


        Add drop-shadow support.

kvark · 2017-12-07T03:20:59Z

Review status: 14 of 17 files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

kvark · 2017-12-07T03:21:35Z

@bors-servo r+

bors-servo · 2017-12-07T03:21:35Z

📌 Commit cf85e75 has been approved by kvark

bors-servo · 2017-12-07T07:58:01Z

⌛ Testing commit cf85e75 with merge 4ad764b...


        Auto merge of #2091 - mephisto41:drop-shadow, r=kvark

Add drop shadow support. The major changes here are using hash map to store render_task_id in PicturePrimitive. In order to support drop shadow, we need add a primitive to multiple pictures. So, if we only stored one render_task_id, the result is wrong. Hence, I use a hash map to store multiple render_task_ids and use parent PrimitiveIndex as key. Another change is brush_image now has third configuration which is take color target as source, but only use alpha of this target and multiply color as result. This is because drop shadow is similar to box shadow which take a alpha mask and a color, but drop shadow's alpha mask is the alpha channel of color target.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2091)

bors-servo · 2017-12-07T14:52:21Z

☀️ Test successful - status-appveyor, status-travis
Approved by: kvark
Pushing 4ad764b to master...

mephisto41 mentioned this pull request Nov 24, 2017

Implement drop-shadow filter #2044

Closed

mephisto41 force-pushed the mephisto41:drop-shadow branch from 6a7ea72 to 2a4783a Nov 29, 2017

mephisto41 force-pushed the mephisto41:drop-shadow branch from 2a4783a to f6983f7 Nov 29, 2017

glennw requested changes Dec 4, 2017

View changes

mephisto41 force-pushed the mephisto41:drop-shadow branch from f6983f7 to 2f6ad7a Dec 4, 2017

mephisto41 force-pushed the mephisto41:drop-shadow branch from 2f6ad7a to e5d1782 Dec 4, 2017

mephisto41 force-pushed the mephisto41:drop-shadow branch from e5d1782 to 1a11c5e Dec 5, 2017

mephisto41 force-pushed the mephisto41:drop-shadow branch 2 times, most recently from 18e8c13 to 1370760 Dec 6, 2017

Add drop-shadow support.

Loading status checks…

cf85e75

mephisto41 force-pushed the mephisto41:drop-shadow branch from 1370760 to cf85e75 Dec 7, 2017

bors-servo mentioned this pull request Dec 7, 2017

Add support for color animated properties #2149

Closed

3 of 6 tasks complete

servo / webrender

Add drop shadow support. #2091

Add drop shadow support. #2091

mephisto41 commented Nov 23, 2017 •

edited by larsbergstrom

bors-servo commented Nov 24, 2017

glennw commented Nov 24, 2017

glennw commented Nov 27, 2017

glennw commented Nov 27, 2017

mephisto41 commented Nov 27, 2017

bors-servo commented Nov 29, 2017

mephisto41 commented Dec 1, 2017

mephisto41 commented Dec 4, 2017

mephisto41 commented Dec 4, 2017

glennw left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 4, 2017

mephisto41 commented Dec 4, 2017

kvark commented Dec 4, 2017

mephisto41 commented Dec 5, 2017

kvark commented Dec 5, 2017

mephisto41 commented Dec 6, 2017

kvark commented Dec 6, 2017

mephisto41 commented Dec 7, 2017

kvark commented Dec 7, 2017

kvark commented Dec 7, 2017

bors-servo commented Dec 7, 2017

bors-servo commented Dec 7, 2017

bors-servo commented Dec 7, 2017

servo / webrender

Join GitHub today

Add drop shadow support. #2091

Add drop shadow support. #2091

Conversation

mephisto41 commented Nov 23, 2017 • edited by larsbergstrom

bors-servo commented Nov 24, 2017

glennw commented Nov 24, 2017

glennw commented Nov 27, 2017

glennw commented Nov 27, 2017

mephisto41 commented Nov 27, 2017

bors-servo commented Nov 29, 2017

mephisto41 commented Dec 1, 2017

mephisto41 commented Dec 4, 2017

mephisto41 commented Dec 4, 2017

glennw left a comment

This comment has been minimized.

glennw Dec 4, 2017

This comment has been minimized.

glennw Dec 4, 2017

This comment has been minimized.

glennw Dec 4, 2017

This comment has been minimized.

glennw Dec 4, 2017

This comment has been minimized.

glennw Dec 4, 2017

glennw commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 4, 2017

mephisto41 commented Dec 4, 2017

kvark commented Dec 4, 2017

mephisto41 commented Dec 5, 2017

kvark commented Dec 5, 2017

mephisto41 commented Dec 6, 2017

kvark commented Dec 6, 2017

mephisto41 commented Dec 7, 2017

kvark commented Dec 7, 2017

kvark commented Dec 7, 2017

bors-servo commented Dec 7, 2017

bors-servo commented Dec 7, 2017

bors-servo commented Dec 7, 2017

mephisto41 commented Nov 23, 2017 •

edited by larsbergstrom