Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BlendMode::Src overwrites unrelated pixels in affected rows #193

Open
wchargin opened this issue Jul 31, 2023 · 0 comments
Open

BlendMode::Src overwrites unrelated pixels in affected rows #193

wchargin opened this issue Jul 31, 2023 · 0 comments

Comments

@wchargin
Copy link

wchargin commented Jul 31, 2023

I have a program that uses raqote to render an image in "chunks" on
different threads, and then composite them all into a final image. By
"chunks" I mean disjoint rectangular regions that cover the canvas:
e.g., a 2×2 grid of quadrants.

This works fine with the default SrcOver blend mode, but I figured
that it would be faster to use Src since it's is a no-op
instead of some bit operations that compile down to 15 or so
instructions
and probably prevent autovectorizing of the
copy loop.

I thought that blending with Src would produce the same result because
no two pixels in a chunk overlap. But in fact it seems that when drawing
an image into a DrawTarget with Src, all other pixels in the row
are cleared, set to transparency. Here is an example:

use raqote::{BlendMode, DrawOptions, DrawTarget, Image, PathBuilder, SolidSource, Source};

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let rgb_source = |r, g, b| Source::Solid(SolidSource { r, g, b, a: 255 });
    let blue = rgb_source(34, 127, 190);
    let white = rgb_source(255, 255, 255);

    // Create a white circle on a blue field.
    let mut circle = DrawTarget::new(50, 50);
    circle.fill_rect(0.0, 0.0, 50.0, 50.0, &blue, &DrawOptions::new());
    let mut pb = PathBuilder::new();
    pb.arc(25.0, 25.0, 20.0, 0.0, std::f32::consts::TAU);
    circle.fill(&pb.finish(), &white, &DrawOptions::new());
    let circle_image = Image {
        width: circle.width(),
        height: circle.height(),
        data: circle.get_data(),
    };

    // Now, try to draw a 2x2 grid of those.
    let mut canvas = DrawTarget::new(100, 100);

    // Top row: two blue circles next to each other, default (SrcOver) compositing.
    canvas.draw_image_at(0.0, 0.0, &circle_image, &DrawOptions::new());
    canvas.draw_image_at(50.0, 0.0, &circle_image, &DrawOptions::new());

    // Bottom row: using Src compositing, since the pixels are disjoint and should cover the row
    // exactly. Should be the same, right?
    let mut opts_src = DrawOptions::new();
    opts_src.blend_mode = BlendMode::Src;
    canvas.draw_image_at(0.0, 50.0, &circle_image, &opts_src);
    canvas.draw_image_at(50.0, 50.0, &circle_image, &opts_src);

    canvas.write_png("canvas.png")?;
    Ok(())
}

At raqote v0.8.2, this produces the following image:

An image with four quadrants. The top-left, top-right, and bottom-right quadrants have a white circle on a blue background, but the bottom-left quadrant is transparent.

As you can see, drawing the bottom-right quadrant has caused the
bottom-left quadrant to be clobbered with transparency, even though it
shouldn't have been affected at all because it is not within the bounds
of the image that I asked to draw.

In addition to being a correctness issue, this suggests that raqote is
doing a lot more work compositing than it needs to! e.g., we would
expect drawing an M×N grid of fixed-size images onto a canvas to take
O(M·N) time, but it seems like it would actually take O(M·N²) time,
because each image that's drawn needs to rewrite the whole row.

I tried to track down what's happening, and this is my understanding:

  • The data gets clobbered in ShaderBlendBlitter::blit_span, where
    blend_fn points to blend_row::<Src>;
  • blend_row::<Src>(src, dst) effectively picks how much to blend
    based on the length of src, since dst is set to a whole suffix
    of the image data buffer;
  • the instantiated value of src is &self.tmp[..], which is created
    in DrawTarget::choose_blitter as a vector with length width;
  • and choose_blitter's unique call site, in DrawTarget::composite,
    unconditionally sets width to self.width, which does not at all
    incorporate the rect to which we're drawing.

Correspondingly, this patch seems to fix the problem for me:

diff --git a/src/draw_target.rs b/src/draw_target.rs
index 8206a5d..cc65771 100644
--- a/src/draw_target.rs
+++ b/src/draw_target.rs
@@ -979,7 +979,7 @@ impl<Backing : AsRef<[u32]> + AsMut<[u32]>> DrawTarget<Backing> {
         let shader = choose_shader(&ti, src, alpha, &mut shader_storage);
 
         let mut blitter_storage = ShaderBlitterStorage::None;
-        let blitter = DrawTarget::choose_blitter(mask, &self.clip_stack, &mut blitter_storage, shader, blend, dest, dest_bounds, self.width);
+        let blitter = DrawTarget::choose_blitter(mask, &self.clip_stack, &mut blitter_storage, shader, blend, dest, dest_bounds, self.width.min(rect.width()));
 
         match mask {
             Some(mask) => {

The raqote test suite continues to pass with this patch. I'm not
familiar enough to understand whether this is the right patch and good
in all cases, but I did try it out on the sweep-gradient example and
on images like this that my program generates; each one was
bitwise-identical before and after this patch.

What do you think—is this patch appropriate and sufficient, or is
something trickier needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant