[WIP] Simplify texture names and support more than two textures in the shaders. #534

nical · 2016-11-07T11:03:05Z

WebRender currently mostly assumes two reusable texture slots: the color and mask textures, which are sometimes used as color and color, and will soon be also used for other purposes like the Y, U and V planes (requiring an extra slot).
The way some shaders refer to the mask texture (sMask) as an extra color texture is confusing, and while it is most often used as a mask, the fact that it is sometimes not used as such means one can never know for sure until careful review of the corresponding shader code.
We also need more samplers for some effects such as YUV images.

I propose that the reusable samplers have generic names (sTexture0, sTexture1, etc.) which are used in the generic places (for example the device mostly does not know whether a texture is used as a color, a mask or a YUV plane), with a collection of specific name aliases (COLOR_TEXTURE_0, MASK_TEXTURE, Y_TEXTURE, etc.) for the code manipulating the samplers for specific purposes. This applies to both the shaders and the various data structures generated in the render backend.

This change is

nical · 2016-11-07T11:08:17Z

This is a work in progress (do not land), I haven't yet figured out all of the places where to use generic names versus specific (for example I have a hunch that mask_texture_id in prim_store.rs may used as a non-mask texture but I am not sure).
I'd also like to use this refactoring to add extra samplers where they'll be needed for yuv images and rebase my video work on top of this if the approach is good.

Feedback welcome!

kvark · 2016-11-07T17:07:02Z

This redirection would allow us to use less texture slots, so can be useful if we see the problem with the texture slots on the horizon. Alternatively, If we don't anticipate the problem, we can continue using pre-defined slots for stuff, just make it more clean (i.e. add sColorExtra instead of re-using sMask), which has a benefit of simpler code.

nical · 2016-11-07T17:43:01Z

I'm all for not aliasing the sampler names if we have the budget to do so. As long as I can trust the variable names when I read the code I am happy.

we'd have:

sColor
sSecondaryColor (for blending)
sMask
sYTexture
sUTexture
sVTexture

It would not be too bad to reuse "color" for the yuv planes so we could have instead:

sColor0 (most things, including Y plane)
sColor1 (blending, U plane)
sColor2 (V plane)
sMask

That's for the shaders. The rust side of things has the same naming issue (even harder to understand in my opinion) which my patch doesn't properly address yet.

glennw · 2016-11-07T19:18:12Z

I'd prefer not to alias the names right now - we can deal with that if/when we hit it. Having sColor0-2 and sMask seems good to me.

bors-servo · 2016-11-07T20:50:33Z

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

nical · 2016-11-08T07:30:52Z

Sounds good to me, I'll update the PR.

nical · 2016-11-08T14:44:58Z

The latest version is still a work in progress. Shaders can receive up to 3 color textures plus 1 mask. AlphaBatchKey specifies the four textures (instead of a color and something that is either a mask or a color but is called mask), I haven't gotten to the primitive store yet, but something similar needs to happen there.

Note that these four SourceTexture enums end up taking a lot more space that two texture ids. If that's an issue we can optimize the SourceTexture data structure (for example encode the external key in the TextureId using some of the spare bits in the target member to differentiate between the two).

nical · 2016-11-08T15:54:06Z

Things are starting to come together. I have removed the SourceTexture/ExternalImage part, which I'll come back with in a separate pull request.

This PR now does the following things:

Stop reusing texture names for other things (for instance using mask_texture to store a secondary color), both in the shaders and the rust code.
Support up to three color textures, in preparation for the YUV image work.

kvark

Looks good! Got a few non-critical nits to address.

kvark · 2016-11-08T16:08:46Z

webrender/res/clip_shared.glsl

@@ -58,7 +58,7 @@ float do_clip(vec2 pos) {
    vec2 clamped_mask_uv = repeat_mask ? fract(vMaskUv) :
        clamp(vMaskUv, vec2(0.0, 0.0), vec2(1.0, 1.0));
    vec2 source_uv = clamped_mask_uv * vClipMaskUvRect.zw + vClipMaskUvRect.xy;
-    float mask_alpha = texture(sMask, source_uv).r; //careful: texture has type A8
+    float mask_alpha = texture(sMask,  source_uv).r; //careful: texture has type A8


accidental spacing?

kvark · 2016-11-08T16:12:39Z

webrender/src/device.rs

@@ -341,6 +341,8 @@ impl TextureId {
            target: gl::TEXTURE_2D,
        }
    }
+
+    pub fn is_invalid(&self) -> bool { *self == TextureId::invalid() }


while certainly useful, I think it would be better to avoid double-negative, and have is_valid() method instead

I originally had is_valid, but all call sites were negating it if !texture.is_valid() { ... so I ended up bundling the negation in the function. I don't mind either way, whichever you prefer works for me.

Perf-wise, marking it #[inline] makes both equally efficient, and !texture.is_valid() takes even 1 less char than texture.is_invalid(), so it's not like we are sacrificing compactness for clarity here :)

kvark · 2016-11-08T16:14:04Z

webrender/src/internal_types.rs

+}
+
+// In some places we need to temporarily bind a texture to any slot.
+pub const DEFAULT_SAMPLER: TextureSampler = TextureSampler::Color0;


Perhaps, we can lean more to the "texture" part of the concept, since eventually we'll have separate samplers from textures, and this term (DEFAULT_SAMPLER) will be misleading. Maybe DEFAULT_TEXTURE?

kvark · 2016-11-08T16:15:28Z

webrender/src/internal_types.rs

+/// Optional textures that can be used as a source in the shaders.
+/// Textures that are not used by the batch are equal to TextureId::invalid().
+#[derive(Copy, Clone, Debug)]
+pub struct PrimitiveBatchTextures {


that's nice! Although, FYI the mask one may be gone soon.

kvark · 2016-11-08T16:16:03Z

webrender/src/prim_store.rs

@@ -93,7 +93,7 @@ pub enum ImagePrimitiveKind {
 #[derive(Debug)]
 pub struct ImagePrimitiveCpu {
    pub kind: ImagePrimitiveKind,
-    pub color_texture_id: TextureId,
+    pub color_texture: TextureId,


any reason you didn't like the _id at the end?

that's an artifact from when I had replaced some of the TextureId by the SourceTexture enum (and thus removed the _id suffix) before splitting that out for a future PR. I'll correct that.

kvark · 2016-11-08T16:17:46Z

webrender/src/renderer.rs

-        self.device.bind_texture(TextureSampler::Mask, mask_texture_id);
+
+        // TODO(nical): This will force all unused slots to be bound to the invalid
+        // TextureId. Should we just leave them out?


yeah, no need to touch what is not used
if a texture is used and not provided (thus forcing us to bind invalid()), that's a bug we should be chasing and fixing

FWIW we already explicitly bind to TextureId::invalid in some places such as

webrender/webrender/src/renderer.rs

Line 1157 in 74a21e9

self.device.bind_texture(TextureSampler::Mask, TextureId::invalid());

The renderer currently also does that for the color and mask textures every time.

In practice it unbinds the texture since the "invalid" gl handle is 0 under the hood.

I don't know if unbinding an unused texture slot after a pass that used it has any noticeable overhead so I added the comment bu it's probably not a big deal.

kvark · 2016-11-08T16:18:10Z

webrender/src/internal_types.rs

+}
+
+impl PrimitiveBatchTextures {
+    pub fn no_texture() -> Self {


perhaps just empty() ?

kvark · 2016-11-08T16:21:34Z

webrender/src/renderer.rs

-                        self.device.delete_buffer(ubo);
-                    }
+                    let max_prim_items = self.max_prim_blends;
+                    self.draw_ubo_batch(ubo_data, shader,


interesting. @glennw do you remember why draw_ubo_batch wasn't used here in the first place?

Hmmm, I'm not sure - possibly just an oversight.

kvark · 2016-11-08T16:24:40Z

webrender/src/tiling.rs

-             self.color_texture_id == other.color_texture_id) &&
-            (self.mask_texture_id == TextureId::invalid() || other.mask_texture_id == TextureId::invalid() ||
-             self.mask_texture_id == other.mask_texture_id)
+            (self.textures.colors[0].is_invalid() || other.textures.colors[0].is_invalid() ||


could use some iteration here for DRY

glennw

@nical This looks great, it tidies up the code a lot and adds support for the extra texture slots we need.

I was a little confused why you had come across usage of sMask as a color texture - then I realised there are a heap of old, unused shaders in the resource dir. I've removed them in #542.

So you'll need to rebase after that lands, which should be easy. Apart from that and the couple of minor comments, this should be good to merge if you're happy with it after that?

glennw · 2016-11-08T22:57:57Z

webrender/src/renderer.rs

-                        self.device.delete_buffer(ubo);
-                    }
+                    let max_prim_items = self.max_prim_blends;
+                    self.draw_ubo_batch(ubo_data, shader,


Hmmm, I'm not sure - possibly just an oversight.

glennw · 2016-11-08T22:59:09Z

webrender/src/tiling.rs

-             self.color_texture_id == other.color_texture_id) &&
-            (self.mask_texture_id == TextureId::invalid() || other.mask_texture_id == TextureId::invalid() ||
-             self.mask_texture_id == other.mask_texture_id)
+            (!self.textures.colors[0].is_valid() || !other.textures.colors[0].is_valid() ||


Can we add a (inline) function to check if two textures are compatible? That would make this function a lot easier to read.

bors-servo · 2016-11-09T02:31:57Z

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

glennw · 2016-11-09T05:45:55Z

@nical BTW, I've finished up the work I was doing for text-shadow to allow me to work on the texture cache. This is mostly related to improvements for handling the glyph cache, but is likely to overlap quite a bit with the SourceTexture / ExternalTexture stuff you mentioned - so we should discuss that at some point. In the meantime, let's get this PR finished up and merged as soon as we can :)

nical · 2016-11-09T14:28:20Z

I rebased everything on top of current master (which was epic, I should have squashed the commits before rather than after doing that), and squashed it down to one commit.

bors-servo · 2016-11-09T20:43:29Z

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

…ers.

glennw · 2016-11-10T01:04:51Z

@bors-servo r+

bors-servo · 2016-11-10T01:04:54Z

📌 Commit 5b8f55f has been approved by glennw

bors-servo · 2016-11-10T01:04:58Z

⌛ Testing commit 5b8f55f with merge 00a8818...

[WIP] Simplify texture names and support more than two textures in the shaders. WebRender currently mostly assumes two reusable texture slots: the color and mask textures, which are sometimes used as color and color, and will soon be also used for other purposes like the Y, U and V planes (requiring an extra slot). The way some shaders refer to the mask texture (sMask) as an extra color texture is confusing, and while it is most often used as a mask, the fact that it is sometimes not used as such means one can never know for sure until careful review of the corresponding shader code. We also need more samplers for some effects such as YUV images. I propose that the reusable samplers have generic names (sTexture0, sTexture1, etc.) which are used in the generic places (for example the device mostly does not know whether a texture is used as a color, a mask or a YUV plane), with a collection of specific name aliases (COLOR_TEXTURE_0, MASK_TEXTURE, Y_TEXTURE, etc.) for the code manipulating the samplers for specific purposes. This applies to both the shaders and the various data structures generated in the render backend.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/534)

bors-servo · 2016-11-10T01:37:25Z

☀️ Test successful - status-travis

Ms2ger changed the title ~~Use generic names for the reusable samplers and add specific aliases.~~ [WIP] Use generic names for the reusable samplers and add specific aliases. Nov 7, 2016

nical force-pushed the sampler-names branch from c096a94 to bd5569f Compare November 8, 2016 14:50

nical changed the title ~~[WIP] Use generic names for the reusable samplers and add specific aliases.~~ [WIP] Simplify texture names and support more than two textures in the shaders. Nov 8, 2016

kvark approved these changes Nov 8, 2016

View reviewed changes

glennw reviewed Nov 8, 2016

View reviewed changes

nical force-pushed the sampler-names branch 2 times, most recently from 6a379df to c2bf9c9 Compare November 9, 2016 14:25

nical force-pushed the sampler-names branch from c2bf9c9 to e0dd8fb Compare November 9, 2016 14:30

This was referenced Nov 9, 2016

Add a YUV image shader. #528

Merged

[WIP] Add support for externally allocated images in the render backend #548

Closed

nical and others added 2 commits November 10, 2016 06:52

Refactor texture names and support more than two textures in the shad…

c8b514b

…ers.

Rebase and update to handle new shaders.

5b8f55f

glennw force-pushed the sampler-names branch from e0dd8fb to 5b8f55f Compare November 9, 2016 21:49

bors-servo merged commit 5b8f55f into servo:master Nov 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Simplify texture names and support more than two textures in the shaders. #534

[WIP] Simplify texture names and support more than two textures in the shaders. #534

nical commented Nov 7, 2016 •

edited by larsbergstrom

Loading

nical commented Nov 7, 2016

kvark commented Nov 7, 2016

nical commented Nov 7, 2016 •

edited

Loading

glennw commented Nov 7, 2016

bors-servo commented Nov 7, 2016

nical commented Nov 8, 2016

nical commented Nov 8, 2016

nical commented Nov 8, 2016

kvark left a comment

kvark Nov 8, 2016

kvark Nov 8, 2016

nical Nov 8, 2016

kvark Nov 8, 2016

kvark Nov 8, 2016

kvark Nov 8, 2016

kvark Nov 8, 2016

nical Nov 8, 2016

kvark Nov 8, 2016

nical Nov 8, 2016

kvark Nov 8, 2016

kvark Nov 8, 2016

glennw Nov 8, 2016

kvark Nov 8, 2016

glennw left a comment

glennw Nov 8, 2016

glennw Nov 8, 2016

bors-servo commented Nov 9, 2016

glennw commented Nov 9, 2016

nical commented Nov 9, 2016

bors-servo commented Nov 9, 2016

glennw commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

[WIP] Simplify texture names and support more than two textures in the shaders. #534

[WIP] Simplify texture names and support more than two textures in the shaders. #534

Conversation

nical commented Nov 7, 2016 • edited by larsbergstrom Loading

nical commented Nov 7, 2016

kvark commented Nov 7, 2016

nical commented Nov 7, 2016 • edited Loading

glennw commented Nov 7, 2016

bors-servo commented Nov 7, 2016

nical commented Nov 8, 2016

nical commented Nov 8, 2016

nical commented Nov 8, 2016

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Nov 9, 2016

glennw commented Nov 9, 2016

nical commented Nov 9, 2016

bors-servo commented Nov 9, 2016

glennw commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

nical commented Nov 7, 2016 •

edited by larsbergstrom

Loading

nical commented Nov 7, 2016 •

edited

Loading