From 7cb22c292c4d55c470351fc135632974da12c1e5 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 2 Sep 2024 15:17:48 +0100 Subject: [PATCH 1/5] Add a "failing" test for #680 --- examples/scenes/src/test_scenes.rs | 16 +++++++ vello_tests/src/snapshot.rs | 12 +++++- vello_tests/tests/known_issues.rs | 67 ++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 vello_tests/tests/known_issues.rs diff --git a/examples/scenes/src/test_scenes.rs b/examples/scenes/src/test_scenes.rs index 92f513d72..f4f5d5b81 100644 --- a/examples/scenes/src/test_scenes.rs +++ b/examples/scenes/src/test_scenes.rs @@ -79,6 +79,7 @@ export_scenes!( mmark(crate::mmark::MMark::new(80_000), "mmark", false), many_draw_objects(many_draw_objects), blurred_rounded_rect(blurred_rounded_rect), + many_bins(many_bins) ); /// Implementations for the test scenes. @@ -1746,4 +1747,19 @@ mod impls { params.time.sin() * 50.0 + 50.0, ); } + + /// A reproduction for + pub(super) fn many_bins(scene: &mut Scene, params: &mut SceneParams) { + params.resolution = Some(Vec2 { + x: 256. * 17., + y: 256. * 16., + }); + scene.fill( + Fill::NonZero, + Affine::IDENTITY, + Color::AQUA, + None, + &Rect::new(3000., 5., 4200., 4200.), + ); + } } diff --git a/vello_tests/src/snapshot.rs b/vello_tests/src/snapshot.rs index fe32aa667..4b03369be 100644 --- a/vello_tests/src/snapshot.rs +++ b/vello_tests/src/snapshot.rs @@ -147,14 +147,24 @@ pub fn smoke_snapshot_test_sync(scene: Scene, params: &TestParams) -> Result Result { let raw_rendered = render_then_debug(&scene, params).await?; + snapshot_test_image(raw_rendered, params, directory) +} +/// Evaluate a snapshot test on the given image. +/// +/// This is useful if a post-processing step needs to happen +/// in-between running Vello and the image. +pub fn snapshot_test_image( + raw_rendered: Image, + params: &TestParams, + directory: SnapshotDirectory, +) -> Result { let reference_path = snapshot_dir(directory) .join(¶ms.name) .with_extension("png"); diff --git a/vello_tests/tests/known_issues.rs b/vello_tests/tests/known_issues.rs new file mode 100644 index 000000000..2bc16e101 --- /dev/null +++ b/vello_tests/tests/known_issues.rs @@ -0,0 +1,67 @@ +// Copyright 2024 the Vello Authors +// SPDX-License-Identifier: Apache-2.0 OR MIT + +//! Reproductions for known bugs, to allow test driven development + +use vello::{ + kurbo::{Affine, Rect}, + peniko::{Color, Format}, + Scene, +}; +use vello_tests::TestParams; + +/// A reproduction of +fn many_bins(use_cpu: bool) { + let mut scene = Scene::new(); + scene.fill( + vello::peniko::Fill::NonZero, + Affine::IDENTITY, + Color::RED, + None, + &Rect::new(-5., -5., 256. * 20., 256. * 20.), + ); + let params = TestParams { + use_cpu, + ..TestParams::new("many_bins", 256 * 17, 256 * 17) + }; + // To view, use VELLO_DEBUG_TEST=many_bins + let image = vello_tests::render_then_debug_sync(&scene, ¶ms).unwrap(); + assert_eq!(image.format, Format::Rgba8); + let mut red_count = 0; + let mut black_count = 0; + for pixel in image.data.data().chunks_exact(4) { + let &[r, g, b, a] = pixel else { unreachable!() }; + let is_red = r == 255 && g == 0 && b == 0 && a == 255; + let is_black = r == 0 && g == 0 && b == 0 && a == 255; + if !is_red && !is_black { + panic!("{pixel:?}"); + } + match (is_red, is_black) { + (true, true) => unreachable!(), + (true, false) => red_count += 1, + (false, true) => black_count += 1, + (false, false) => panic!("Got unexpected pixel {pixel:?}"), + } + } + // When #680 is fixed, this will become: + // let drawn_bins = 17 /* x bins */ * 17 /* y bins*/; + + // The current maximum number of bins. + let drawn_bins = 256; + let expected_red_count = drawn_bins * 256 /* tiles per bin */ * 256 /* Pixels per tile */; + assert_eq!(red_count, expected_red_count); + assert!(black_count > 0); +} + +#[test] +#[cfg_attr(skip_gpu_tests, ignore)] +fn many_bins_gpu() { + many_bins(false); +} + +#[test] +#[cfg_attr(skip_gpu_tests, ignore)] +#[should_panic] +fn many_bins_cpu() { + many_bins(true); +} From e2575459bf63a5ec3fbf15c731e03404a0f43ce3 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 2 Sep 2024 15:22:06 +0100 Subject: [PATCH 2/5] Remove the many_bins test scene --- examples/scenes/src/test_scenes.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/examples/scenes/src/test_scenes.rs b/examples/scenes/src/test_scenes.rs index f4f5d5b81..92f513d72 100644 --- a/examples/scenes/src/test_scenes.rs +++ b/examples/scenes/src/test_scenes.rs @@ -79,7 +79,6 @@ export_scenes!( mmark(crate::mmark::MMark::new(80_000), "mmark", false), many_draw_objects(many_draw_objects), blurred_rounded_rect(blurred_rounded_rect), - many_bins(many_bins) ); /// Implementations for the test scenes. @@ -1747,19 +1746,4 @@ mod impls { params.time.sin() * 50.0 + 50.0, ); } - - /// A reproduction for - pub(super) fn many_bins(scene: &mut Scene, params: &mut SceneParams) { - params.resolution = Some(Vec2 { - x: 256. * 17., - y: 256. * 16., - }); - scene.fill( - Fill::NonZero, - Affine::IDENTITY, - Color::AQUA, - None, - &Rect::new(3000., 5., 4200., 4200.), - ); - } } From 0c5d4dd90ab245ce11d036912f8b3d6f27eb2094 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Wed, 18 Sep 2024 11:15:49 +0100 Subject: [PATCH 3/5] Add a warning when there are more than 256 bins --- vello/src/render.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/vello/src/render.rs b/vello/src/render.rs index 2cca8e6b2..f8038a608 100644 --- a/vello/src/render.rs +++ b/vello/src/render.rs @@ -168,6 +168,19 @@ impl Render { } let cpu_config = RenderConfig::new(&layout, params.width, params.height, ¶ms.base_color); + // The coarse workgroup counts is the number of bins active. + if (cpu_config.workgroup_counts.coarse.0 + * cpu_config.workgroup_counts.coarse.1 + * cpu_config.workgroup_counts.coarse.2) + > 256 + { + log::warn!( + "Trying to paint too large image. {}x{}.\n\ + See https://github.com/linebender/vello/issues/680 for details", + params.width, + params.height + ); + } let buffer_sizes = &cpu_config.buffer_sizes; let wg_counts = &cpu_config.workgroup_counts; From 6ddb727777e08e76afdfd55bfc99f83cc2107ebf Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:29:50 +0100 Subject: [PATCH 4/5] Make it clear that the hack is a hack --- vello/src/render.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vello/src/render.rs b/vello/src/render.rs index f8038a608..eadb5c2a1 100644 --- a/vello/src/render.rs +++ b/vello/src/render.rs @@ -168,7 +168,7 @@ impl Render { } let cpu_config = RenderConfig::new(&layout, params.width, params.height, ¶ms.base_color); - // The coarse workgroup counts is the number of bins active. + // HACK: The coarse workgroup counts is the number of active bins. if (cpu_config.workgroup_counts.coarse.0 * cpu_config.workgroup_counts.coarse.1 * cpu_config.workgroup_counts.coarse.2) From 471acd120bdd17fabc4fb37c2543b1ca67947e5f Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Fri, 20 Sep 2024 16:56:41 +0100 Subject: [PATCH 5/5] Re-add docs to `snapshot_test` --- vello_tests/src/snapshot.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vello_tests/src/snapshot.rs b/vello_tests/src/snapshot.rs index 4b03369be..eaefd42fe 100644 --- a/vello_tests/src/snapshot.rs +++ b/vello_tests/src/snapshot.rs @@ -147,6 +147,9 @@ pub fn smoke_snapshot_test_sync(scene: Scene, params: &TestParams) -> Result