Introduce color matrix filter. #2323

eeejay · 2018-01-18T17:10:24Z

This is a patch I came up with for issue #2297. It's not very good! This is my first Rust patch, and I know nothing of graphics or shaders. But it works? And it passes all the ref tests.

For one, I increased the FilterOp enum type to be over 80 bytes.

Second, I badly abused the GPU cache. I'm not sure how to do this. Primitives are too small for storing a whole matrix.

This change is

glennw · 2018-01-19T03:02:13Z

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

webrender/res/ps_blend.glsl, line 24 at r1 (raw file):

  ColorTransforms ctransform;

  ctransform.matrix = mat4(data[3], data[4], data[5], data[6]);

It wasn't really clear to me why this worked, starting to index from [3]. But I guess it's because the code passes the GPU cache address of the primitive, which already writes [local rect, local clip, color]. You mentioned this in the original commit, but it took me a bit to work out what was going on here :)

webrender/res/ps_blend.glsl, line 161 at r1 (raw file):

            break;
        case 10:
            oFragColor = vColorTransforms.matrix * Cs + vColorTransforms.multiplier;

It seems like the multiplier is really an offset? Perhaps it should be renamed?

webrender_api/src/display_item.rs, line 505 at r1 (raw file):

    Sepia(f32),
    DropShadow(LayoutVector2D, f32, ColorF),
    ColorMatrix([f32; 20]),

As you mentioned in your commit, increasing the size here isn't ideal. It's probably not that big of a deal - since there aren't that many filter ops on most pages, but it might show up in some of the benchmarks we run that do a lot of opacity ops. I think the "proper" solution is probably to store these in the display list in a separate place, and reference them. Thoughts @gankro?

wrench/src/yaml_helper.rs, line 565 at r1 (raw file):

                ("color-matrix", ref args, _) if args.len() == 1 => {
                    let mut matrix: [f32; 20] = [0.0; 20];
                    let str = format!("---\nmatrix: {}\n", args[0]);

It seems a little unfortunate to create a new yaml doc here. Could we modify the format slightly so that we can just read it as an array of floats, perhaps?

Comments from Reviewable

glennw · 2018-01-19T03:07:37Z

Thanks for the PR!

I made a few comments, but they are mostly just related to the stuff you already mentioned in your commit, or very minor details.

It's not clear to me right now where the best place would be to store that matrix data (both in the display list, and also how we get it to the GPU). I'll have a think over the next day or so and comment here. Some possibilities are:

Expanding the size of the Composite instance struct (might not be ideal due to batch storage issues).
Something similar to what you have now (it doesn't quite fit in with how the rest of the filter system works).
Having it in the GPU cache but stored as a separate entry with a different GPU address (need to think about this more).
Expanding the size of the render task data (might not be as bad as it sounds).
Storing it as a uniform array and breaking batches (doesn't really fit in with how the rest of the code works).

Any thoughts on the above or other ideas @kvark ? Any thoughts on the display list storage @gankro ?

Thanks!

Gankra · 2018-01-19T03:52:47Z

That's pretty harsh bloat, yeah. We could certainly pull it out as a fixed-size aux-list in the same way we do gradient stops. Otherwise we could add a junk None-like variant to this enum so that my deserialize_in_place optimization is making the overall size of the variant less relevant.

glennw · 2018-01-22T20:11:01Z

Having thought about this over the weekend - I think storing it in the GPU cache as part of the Picture data (as you've done here) is basically correct. It does actually fit in well with how brush primitives work, which the blend shader will eventually be ported to.

I'll add a couple of extra comments related to that. Then, we'll need to work out the display list stuff mentioned above by @gankro and the other minor comments.

glennw · 2018-01-22T20:12:48Z

webrender/res/ps_blend.glsl

+  vec4 data[8] = fetch_from_resource_cache_8(address);
+  ColorTransforms ctransform;
+
+  ctransform.matrix = mat4(data[3], data[4], data[5], data[6]);


I think it'd be clearer what's happening here if we pass in the modified address (offset), and then fetch the data from there.

glennw · 2018-01-22T20:12:48Z

webrender/res/ps_blend.glsl

@@ -4,13 +4,29 @@

 #include shared,prim_shared

+struct ColorTransforms {
+  mat4 matrix;


nit: the rest of the file uses 4-space indentation.

glennw · 2018-01-22T20:12:48Z

webrender/src/prim_store.rs

@@ -1276,6 +1276,20 @@ impl PrimitiveStore {
                    let pic = &self.cpu_pictures[metadata.cpu_prim_index.0];
                    pic.write_gpu_blocks(&mut request);

+                    match pic.kind {


Can we move this inside the write_gpu_blocks method of the Picture type?

bors-servo · 2018-01-22T23:35:58Z

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

eeejay · 2018-01-23T22:04:02Z

Review status: 5 of 10 files reviewed at latest revision, 7 unresolved discussions.

webrender/res/ps_blend.glsl, line 24 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

I think it'd be clearer what's happening here if we pass in the modified address (offset), and then fetch the data from there.

So have a constant "offset = 3", or pass offset as an argument to fetch_color_transforms?

webrender/res/ps_blend.glsl, line 24 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

It wasn't really clear to me why this worked, starting to index from [3]. But I guess it's because the code passes the GPU cache address of the primitive, which already writes [local rect, local clip, color]. You mentioned this in the original commit, but it took me a bit to work out what was going on here :)

Interesting! I actually figured out the offset with trial and error.

webrender/res/ps_blend.glsl, line 161 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

It seems like the multiplier is really an offset? Perhaps it should be renamed?

That looks like a bad name to me too. I misread an article about feColorMatrix and thought that was the name of that column. I'll rename it to offset.

wrench/src/yaml_helper.rs, line 565 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

It seems a little unfortunate to create a new yaml doc here. Could we modify the format slightly so that we can just read it as an array of floats, perhaps?

Yeah. Made it a function that takes 20 arguments :)

Comments from Reviewable

eeejay · 2018-01-23T22:07:30Z

Review status: 5 of 10 files reviewed at latest revision, 7 unresolved discussions.

webrender/res/ps_blend.glsl, line 8 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

nit: the rest of the file uses 4-space indentation.

Done.

webrender/src/prim_store.rs, line 1279 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Can we move this inside the write_gpu_blocks method of the Picture type?

Done.

Comments from Reviewable

eeejay · 2018-01-23T22:09:47Z

Review status: 5 of 10 files reviewed at latest revision, 7 unresolved discussions.

webrender_api/src/display_item.rs, line 505 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

As you mentioned in your commit, increasing the size here isn't ideal. It's probably not that big of a deal - since there aren't that many filter ops on most pages, but it might show up in some of the benchmarks we run that do a lot of opacity ops. I think the "proper" solution is probably to store these in the display list in a separate place, and reference them. Thoughts @gankro?

I'm lost in the display list serialization code, and I'm not sure how to do this...

Comments from Reviewable

glennw · 2018-01-24T20:03:53Z

@gankro Can you offer any tips for implementing the display list changes above?

Gankra · 2018-01-29T15:30:19Z

Sorry I was out with flu the last week. I want to spend a bit of today thinking over aux list stuff again since kvark has also been having trouble with this. Hopefully will get back to you tonight.

Gankra · 2018-01-30T17:46:41Z

Ok so having looked at this, I'd like to proceed with the changes to FilterOp as-is, and I'll do a followup PR to optimize our aux-list processing (or not if we measure no impact).

The change I'm picturing is fairly straight-forward for me to implement.

eeejay · 2018-01-30T21:17:04Z

Sounds good! I'm not sure how to use reviewable.io. But I think this pull request is ready for another round of reviews. I am PTO starting tomorrow till February 12th, so I'll be slow on followup.

Review status: 5 of 10 files reviewed at latest revision, 7 unresolved discussions.

Comments from Reviewable

glennw · 2018-01-30T21:19:30Z

@eeejay Sounds good, thanks! I'll do another review pass over this today.

glennw

Looks good! Just a couple of minor changes, then this should be ready to go.

glennw · 2018-01-31T02:57:28Z

webrender/res/ps_blend.glsl

+  vec4 offset;
+};
+
+varying ColorTransforms vColorTransform;


We've had issues in the past with driver bugs when a struct is passed as a varying, unfortunately. See https://github.com/servo/webrender/wiki/Driver-issues#1157---varying-structs for details. The workaround is to only pass GLSL primitive types as varyings.

glennw · 2018-01-31T02:57:28Z

webrender/src/picture.rs

-            PictureKind::TextShadow { .. } |
-            PictureKind::Image { .. } => {
-                request.push([0.0; 4]);
+            PictureKind::TextShadow { .. } => {


There are some code paths that rely on knowing the number of GPU blocks that a primitive writes (the brush / segment code). In this case, it's actually fine, since PictureKind::Image never passes through that path. However, it may do in the future. Could you add a comment here noting that this code will need to change if/when we start pushing Image pictures through the brush path?


        Introduce color matrix filter.

eeejay · 2018-01-31T17:59:30Z

Followed up with the nits you mentioned. Hopefully this is mergable, if not I'll be back on the 12th!

Review status: 3 of 10 files reviewed at latest revision, 9 unresolved discussions.

webrender/res/ps_blend.glsl, line 12 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

We've had issues in the past with driver bugs when a struct is passed as a varying, unfortunately. See https://github.com/servo/webrender/wiki/Driver-issues#1157---varying-structs for details. The workaround is to only pass GLSL primitive types as varyings.

Removing the structure and function.

webrender/src/picture.rs, line 599 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

There are some code paths that rely on knowing the number of GPU blocks that a primitive writes (the brush / segment code). In this case, it's actually fine, since PictureKind::Image never passes through that path. However, it may do in the future. Could you add a comment here noting that this code will need to change if/when we start pushing Image pictures through the brush path?

Cool. I'll add a comment.

Comments from Reviewable

glennw · 2018-01-31T23:02:07Z

Thank you!

@bors-servo r+

bors-servo · 2018-01-31T23:02:08Z

📌 Commit f537110 has been approved by glennw

bors-servo · 2018-02-01T00:04:12Z

⌛ Testing commit f537110 with merge 47f5afa...


        Auto merge of #2323 - eeejay:colormatrix, r=glennw

Introduce color matrix filter. This is a patch I came up with for issue #2297. It's not very good! This is my first Rust patch, and I know nothing of graphics or shaders. But it works? And it passes all the ref tests. For one, I increased the FilterOp enum type to be over 80 bytes. Second, I badly abused the GPU cache. I'm not sure how to do this. Primitives are too small for storing a whole matrix.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2323)

bors-servo · 2018-02-01T01:12:55Z

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: glennw
Pushing 47f5afa to master...

glennw reviewed Jan 22, 2018

View changes

eeejay force-pushed the eeejay:colormatrix branch from 7935168 to 6e02ea8 Jan 23, 2018

glennw reviewed Jan 31, 2018

View changes

Introduce color matrix filter.

Loading status checks…

f537110

eeejay force-pushed the eeejay:colormatrix branch from 6e02ea8 to f537110 Jan 31, 2018

servo / webrender

Introduce color matrix filter. #2323

Introduce color matrix filter. #2323

eeejay commented Jan 18, 2018 •

edited by larsbergstrom

glennw commented Jan 19, 2018

glennw commented Jan 19, 2018

Gankra commented Jan 19, 2018

glennw commented Jan 22, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jan 22, 2018

eeejay commented Jan 23, 2018

eeejay commented Jan 23, 2018

eeejay commented Jan 23, 2018

glennw commented Jan 24, 2018

Gankra commented Jan 29, 2018

Gankra commented Jan 30, 2018

eeejay commented Jan 30, 2018

glennw commented Jan 30, 2018

glennw left a comment

This comment has been minimized.

This comment has been minimized.

eeejay commented Jan 31, 2018

glennw commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Feb 1, 2018

bors-servo commented Feb 1, 2018

servo / webrender

Join GitHub today

Introduce color matrix filter. #2323

Introduce color matrix filter. #2323

Conversation

eeejay commented Jan 18, 2018 • edited by larsbergstrom

glennw commented Jan 19, 2018

glennw commented Jan 19, 2018

Gankra commented Jan 19, 2018

glennw commented Jan 22, 2018

This comment has been minimized.

glennw Jan 22, 2018

This comment has been minimized.

glennw Jan 22, 2018

This comment has been minimized.

glennw Jan 22, 2018

bors-servo commented Jan 22, 2018

eeejay commented Jan 23, 2018

eeejay commented Jan 23, 2018

eeejay commented Jan 23, 2018

glennw commented Jan 24, 2018

Gankra commented Jan 29, 2018

Gankra commented Jan 30, 2018

eeejay commented Jan 30, 2018

glennw commented Jan 30, 2018

glennw left a comment

This comment has been minimized.

glennw Jan 31, 2018

This comment has been minimized.

glennw Jan 31, 2018

eeejay commented Jan 31, 2018

glennw commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Feb 1, 2018

bors-servo commented Feb 1, 2018

eeejay commented Jan 18, 2018 •

edited by larsbergstrom