Image mask support #445

kvark · 2016-10-17T17:20:33Z

Closes #405

I'm not completely sure if this is the right approach, hoping to get some review/feedback to proceed. The API and logic works for me with wr_sample, although the implementation lacks a lot of features to support, like image repeating and layout transformations.

This change is

glennw · 2016-10-17T22:51:39Z

@kvark Thanks! The general idea here seems good - the public API looks fine to me.

In terms of implementation, I try to avoid using uniform variables whenever possible, to simplify how the batching code works and reduce batch breaks. Instead, what I'd suggest is adding fields to the Clip struct in tiling.rs, and modifying the code in prim_shared to fetch the data from there. That way, there shouldn't be any need to change the batching code at all.

Let me know if that doesn't make sense or you foresee any issues with that :)

kvark · 2016-10-18T00:36:53Z

@glennw thanks for taking a look!
I've actually considered that (passing via Clip) but then thought that since the image itself is being passed as a uniform sampler, it doesn't make sense to pass the associated data per batch.
Now that I think of it though, the image we pass is an atlas, so it makes perfect sense to have different source/dest rectangles per batch given that these refer to different sub-images within the atlas.
I'll implement the proposed change and get back to you ;)

glennw · 2016-10-18T01:02:46Z

@kvark Yep, that's right - we put (almost) everything in an atlas, so it should batch together nicely :)

bors-servo · 2016-10-18T23:06:46Z

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

kvark · 2016-10-21T14:13:35Z

Rebased now, the implementation follows the route you laid out - passing the clip rectangles via the batch data instead of uniforms.

bors-servo · 2016-10-24T04:45:20Z

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

glennw

Looks pretty good, just a few minor issues / questions. I rebased it locally on my machine to test with, there was only a couple of minor conflicts to fix up.

glennw · 2016-10-24T06:27:22Z

webrender/res/clip_shared.glsl

@@ -5,6 +5,8 @@

 flat varying vec4 vClipRect;
 flat varying vec4 vClipRadius;
+flat varying vec4 vClipMaskUvRect;
+flat varying vec4 vClipMaskScreenRect;


We should probably rename this to be something like vClipMaskLocalRect, since (I think) it's in the local space of the primitive rectangle?

glennw · 2016-10-24T06:28:24Z

webrender/res/prim_shared.glsl

+    return rect;
+}
+
+struct MaskRect {


Could we rename these fields to be a bit more descriptive? Perhaps uv_rect and local_mask_rect or something like that?

glennw · 2016-10-24T06:28:54Z

webrender/src/prim_store.rs

@@ -206,12 +209,19 @@ impl ClipCorner {
 }

 #[derive(Debug, Clone)]
+pub struct MaskRect {


Perhaps rename this to ImageMaskInfo or something like that?

glennw · 2016-10-24T06:30:10Z

webrender/src/renderer.rs

@@ -552,6 +553,11 @@ impl Renderer {
        let data64_texture = VertexDataTexture::new(&mut device);
        let data128_texture = VertexDataTexture::new(&mut device);

+        let dummy_mask_texture = device.create_texture_ids(1)[0];


I wonder if this will cause unnecessary batch breaks when there are clips that don't have masks. We could probably insert a dummy mask into the atlas instead of creating a separate texture. But that doesn't matter for now, we can do it as a follow up if it's an issue.

I had a hard time figuring out how to properly assign a default masking texture for this case. Ended up introducing DummyResources struct to store the white texture as well the opaque one (which were previously forgotten but still created). Let me know if there is a better solution please.

glennw · 2016-10-24T06:31:14Z

webrender/src/tiling.rs

@@ -702,6 +704,23 @@ impl RenderTask {
 pub const SCREEN_TILE_SIZE: i32 = 64;
 pub const RENDERABLE_CACHE_SIZE: DevicePixel = DevicePixel(2048);

+/// Per-batch clipping info merged with the mask image.
+#[derive(Clone, Debug)]
+pub struct MaskedClip {


Instead of introducing a new structure here, could we just add the fields/struct to the Clip structure? The Clip structure already handles simple / complex clips, it seems like it's probably reasonable to make the Clip structure able to handle any number of clip types. If we do that, we won't need to modify the API of all the FrameBuilder methods.

MaskedClip is a part of the Clip, so moving the fields in there directly would break the API as much regardless. The reason I even introduced this was to pack both rectangles into a single 32 byte gpu block, instead of having a 16 byte pad after each.

glennw

Nearly there! :)

glennw · 2016-10-24T22:06:25Z

webrender/res/ps_gradient.fs.glsl

+    vec2 local_pos = vPos;
+#endif
+
+    alpha = min(alpha, do_clip(local_pos));


This contains a do_clip() call which fails to compile.

oh, nicely spotted!

glennw · 2016-10-24T22:09:29Z

webrender/src/renderer.rs

@@ -676,7 +693,7 @@ impl Renderer {
    fn enable_msaa(&self, _: bool) {
    }

-    #[cfg(any(target_os = "windows", unix))]
+    #[cfg(any(target_os = "linux", target_os = "windows", target_os = "macos"))]


Mistake during rebase to change this?

weird indeed, fixed

glennw · 2016-10-24T22:12:00Z

webrender/src/tiling.rs

+
+/// Per-batch clipping info merged with the mask image.
+#[derive(Clone, Debug)]
+pub struct MaskedClip {


I'm still not sure about having this struct. Could we have something like:

struct Clip { // existing clip rect and corners pub mask: Option<MaskImageSource>, }

And then use that to construct the data to be put in the gpu blocks during prim prepare / cache?

I might be missing something as I haven't actually tried this - but it sounds like it might be a simpler API if it works.

I'm following the existing style in here. Clip structure (contrary to ClipRegion) is supposed to map semi-directly to the GPU. At least it seems it should, given that the shader side has the same structs (Clip, ClipRect, ClipCorner).
If I add Option<MaskImageSource>, that would make it inconsistent, since:

pub enum MaskImageSource { User(ImageKey), Renderer(TextureId), }

I can see your point about the consistency aspect of the structure internals, but I think having the public API type for FrameBuilder be called Clip is important. How about as a compromise we keep the structure layout as is, but rename Clip -> ClipGpu and make it private, and rename MaskedClip -> Clip?

agreed, fixed now

kvark · 2016-10-25T14:33:15Z

webrender/res/ps_gradient.fs.glsl

    oFragColor = vColor * vec4(1, 1, 1, alpha);
-
-#ifdef WR_FEATURE_TRANSFORM


@glennw
Looks like we were multiplying the alpha twice. I assume this was a mistake.

glennw · 2016-10-25T20:54:17Z

@kvark Looks good! Let's squash and get this merged :)

kvark · 2016-10-25T21:54:46Z

Do you want a single commit? Or 2-3?

On Oct 25, 2016, at 16:54, Glenn Watson notifications@github.com wrote:

@kvark Looks good! Let's squash and get this merged :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

glennw · 2016-10-25T22:20:25Z

@kvark The smallest number of commits that make sense as individual commits. 2-3 is fine if that makes sense.

Includes the new DummyResource struct as well as a non-clipping gradient shader.

glennw · 2016-10-25T23:34:36Z

@bors-servo r+

bors-servo · 2016-10-25T23:34:37Z

📌 Commit 1e33028 has been approved by glennw

bors-servo · 2016-10-26T00:00:40Z

⌛ Testing commit 1e33028 with merge f13e95b...

Image mask support Closes #405 I'm not completely sure if this is the right approach, hoping to get some review/feedback to proceed. The API and logic works for me with `wr_sample`, although the implementation lacks a lot of features to support, like image repeating and layout transformations.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/445)

bors-servo · 2016-10-26T00:27:14Z

☀️ Test successful - status-travis

kvark force-pushed the image_mask branch from a264c17 to 483b23d Compare October 21, 2016 14:12

glennw requested changes Oct 24, 2016

View reviewed changes

kvark force-pushed the image_mask branch from 23062f5 to 88653ac Compare October 24, 2016 18:39

glennw requested changes Oct 24, 2016

View reviewed changes

kvark commented Oct 25, 2016

View reviewed changes

kvark added 2 commits October 25, 2016 19:11

Image mask support.

f4807a6

Includes the new DummyResource struct as well as a non-clipping gradient shader.

Hide the fields of some of the primitive_store module stucts in private

1e33028

kvark force-pushed the image_mask branch from 526eaaf to 1e33028 Compare October 25, 2016 23:13

glennw approved these changes Oct 25, 2016

View reviewed changes

bors-servo merged commit 1e33028 into servo:master Oct 26, 2016

This was referenced Oct 26, 2016

Replace ServoStackingContextId with ServoScrollRootId #467

Merged

Use texture layers for render target allocations, remove render phases. #478

Merged

kvark deleted the image_mask branch October 26, 2016 02:00

This was referenced Oct 26, 2016

Added mask image and border corner tests glennw/wr-sample#5

Merged

Support for transformed clip regions #498

Closed

		oFragColor = vColor * vec4(1, 1, 1, alpha);

		#ifdef WR_FEATURE_TRANSFORM

Image mask support #445

Image mask support #445

Uh oh!

Conversation

kvark commented Oct 17, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennw commented Oct 17, 2016

Uh oh!

kvark commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennw commented Oct 18, 2016

Uh oh!

bors-servo commented Oct 18, 2016

Uh oh!

kvark commented Oct 21, 2016

Uh oh!

bors-servo commented Oct 24, 2016

Uh oh!

glennw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennw Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennw commented Oct 25, 2016

Uh oh!

kvark commented Oct 25, 2016

Uh oh!

glennw commented Oct 25, 2016

Uh oh!

glennw commented Oct 25, 2016

Uh oh!

bors-servo commented Oct 25, 2016

Uh oh!

bors-servo commented Oct 26, 2016

Uh oh!

kvark commented Oct 17, 2016 •

edited by larsbergstrom

Loading

kvark commented Oct 18, 2016 •

edited

Loading

kvark Oct 24, 2016 •

edited

Loading

glennw Oct 24, 2016 •

edited

Loading