1624493 - Remove brush_opacity shader and replace it with brush_image

Status: NEW

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Glenn Watson [:gw]]

I think that the functionality of the brush_opacity shader can be completely handled by the brush_image shader. Removing brush_opacity would remove another source of batch breaks and shader changes.

If it's determined that it can't be simply replaced with brush_image we should document why, and consider what changes we'd need to make so it can be replaced.

Comment 1

[Bert Peers [:bpeers]]

I looked a bit at the two shaders and got them to draw the same thing using filters/blend-clipped.yaml as a test case, using the following hacks:

• WR_FEATURE_TEXTURE_RECT and WR_FEATURE_REPITITION should not be defined;
• WR_FEATURE_ANTIALIASING should be defined, if we want to preserve the call to init_transform_fs; I couldn't see it make much of a difference though;
• WR_FEATURE_ALPHA_PASS must be defined, else the image brush shader will just pass-through the source texture without opacity; it seems to be #defined and I sort of assume it always would be?
• V_MASK_SWIZZLE should be (1,0) and V_COLOR should be the opacity repeated 4 times; we can achieve this with ShaderColorMode::Image and AlphaType::PremultipliedAlpha in the ImageBrushData, no problems there;
• the VS is missing a call to get_image_quad_uv and this one's not optional. So I imagine perhaps a new flag a la BRUSH_FLAG_IMAGE_QUAD_UV which then somehow gets set through batch.rs;
• in the PS, we probably need to sample the texture with pre-clamped UVs (use repeated_uv instead of uv) and/or we probably need to call point_inside_rect and adjust alpha; again I can't see much difference either way but exactly replicating what brush_opacity does is probably safer. If we don't want to break batching then that's another branch, perhaps a more general BRUSH_FLAG_IMAGE_OPACITY_MODE then?

With that, it seems to work? :)

// Filter::Opacity case:
// 1. leave amount as is (0.0 ... 1.0)
// 2. 
let key = BatchKey::new(
    BatchKind::Brush( // *** new brush ***
        BrushBatchKind::Image(ImageBufferKind::Texture2DArray)),
    BlendMode::PremultipliedAlpha,
    textures,
);

let prim_user_data = ImageBrushData { // *** new user data ***
    color_mode: ShaderColorMode::Image,
    alpha_type: AlphaType::PremultipliedAlpha,
    raster_space: RasterizationSpace::Local,
    opacity: *amount,
}.encode();

// *** different header ***
let prim_header_index = prim_headers.push(&prim_header, z_id, prim_user_data);

self.add_brush_instance_to_batches(
    key,
    batch_features,
    bounding_rect,
    z_id,
    INVALID_SEGMENT_INDEX,
    EdgeAaSegmentMask::empty(),
    clip_task_address.unwrap(),
    brush_flags,
    prim_header_index,
    uv_rect_address.as_int(), // *** pass rect ***
    prim_vis_mask,
);

So I guess tomorrow I can continue trying to do this properly (= add flags and branches).

Comment 2

[Glenn Watson [:gw]]

Thanks for taking a look!

I'm hoping we can do this without needing to add extra logic to the brush_image shader (such as the point_inside_rect you mentioned).

If that's not possible, we might want to put this on hold until we establish raster roots for all opacity filters (which I think would simplify any complexity in the shader related to having non-axis-aligned images we're sampling from?)?

Comment 3

[Glenn Watson [:gw]]

(to clarify, if we can make it so that opacity filter pictures are always drawn in local-space, with raster roots, then any complexity in brush_opacity should collapse down to drawing the simple texture images currently handled by brush_image).

Comment 4

[Bert Peers [:bpeers]]

Attachment: brush_opacity.patch

point_inside_rect seems cheaper than init_transform_fs so the extra cost shouldn't be too high. But it would still be a new flag+branch, as well as a new value to be passed to the pixel shader. So waiting to see what happens with more caching sounds good. I've attached the patch to pick it up again later.