1564973 - Handle world-space clip rectangles better in WR

Status: RESOLVED WONTFIX

(Core :: Graphics: WebRender, enhancement)

Description

[Dzmitry Malyshau [:kvark]]

The approach WR is currently taking is: if a clip is not just a clip-in rectangle, or it's in a different space from the primitive, it is baked into a mask. This means both the clip and the primitives then "meet" in raster space, where the mask is sampled, and we clip out pixels. For clips that are placed in a scale/offset relation to the primitive, we take their bounding rectangles into account for the primitive's local rect, applied in the vertex shader.

There is a particular case that we can handle much better: when the clip is just a clip-in rectangle that is placed in a scale/offset relation to the raster space. The shaders already keep track of the raster space bounds for the clip UVs (as vClipMaskUvBounds). What we could do is to make the raster bounds of a clip to be included in those bounds.

Costs:

• slightly increased implementation complexity
• need to provide the raster rect to the shader, which has some overhead

Benefits:

• by having better vClipMaskUvBounds we can have the pixel shaders to discard before actually sampling anything from the mask more often
• for most clip-in rectangles, we no longer need to bake them into clip tasks, which means less render passes and tasks, less wasted pixel writes, and less sampling
• primitives only affected by those clips would no longer need to render in the alpha pass, which means better depth buffer utilization and fill-rate savings

Comment 1

[Dzmitry Malyshau [:kvark]]

Documenting one aspect of the solution here. When we have a situation where the raster-space clip includes all the clipping we need, it's not trivial to apply it efficiently. Originally I thought that the segment/primitive would just move to the opaque pass, but we aren't using any of the clipping logic there. If we force the switch to the alpha shader variant in the opaque pass, we start requiring the discard() in the shaders since we can't blend in the opaque pass... Discard makes some drivers implementation of the depth testing less efficient, so we face a trade-off: we fill out the Z buffer, but we do it slower than we could otherwise be rejecting the pixels.

I can think of one solution that gets us the best of both worlds, but it's more invasive. The problematic case only happens if we have raster-space rect-only clips, and the primitive is not transformed (doesn't need AA or blending in general). So we have clip -> raster space to be scale/offset, and primitive -> raster space to be scale/offset, which means the conversion between clip and primitive is also pure scale/offset (which we already detect). We should therefore just force the local pic_clip_rect to include our clip instead of relying on the raster-space clip rectangle. This would have the benefit of working in the opaque pass and not wasting pixel fill-rate (since the clip is applied in the vertex shader).

The caveat here is to review the policy we have of including clips in the local_clip_rect. We need to indicate properly that this rect has to be re-computed every frame (whenever spatial positions change between the clip and the primitive), if it's not already the case. Side note: if it is the case, I wonder why we aren't doing this already (i.e. applying the ScaleOffset clips to the pic_clip_rect). cc :gw

Comment 2

[Dzmitry Malyshau [:kvark]]

Just as I got close to figuring out the source of difference and closing it, I'm hitting a problem at a conceptual level that may need some good talk with Glenn. It's the confusion we have about World space versus Raster space.

The shader code sees (and operates on) "world" names when it really is only concerned about the "raster" space. The clips sampling and UV bounds clipping is technically done by the shader in "raster" space, therefore. But the Rust code considers the clips in non-local space of a primitive to always apply in the "world" space. So I guess an important aspect I'm missing here is that where in the code we decide that, when we are rendering into a particular picture, it's time or not yet time to apply the world space clips?

So far, the patch I'm working on is written in an assumption that clips are applied in the "raster" space of a primitive. It's hitting an assertion in "perspective-clip.yaml" that the node space (1) is not a child of the current raster space (3).

Comment 3

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:kvark, maybe it's time to close this bug?

Comment 4

[Dzmitry Malyshau [:kvark]]

Nope, there is WIP, I just switched to a different task.

Comment 5

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:kvark, maybe it's time to close this bug?

Comment 6

[Dzmitry Malyshau [:kvark]]

More patches are needed here, still

Comment 7

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 8

[Glenn Watson [:gw]]

Every surface is a raster root now.