-
Notifications
You must be signed in to change notification settings - Fork 295
vello_sparse_tests: Add support for multi-frame tests #1501
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
23a0de9
c4798c2
2632353
90b6a44
eb301c1
5aca476
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -44,6 +44,11 @@ struct Arguments { | |||||
| no_ref: bool, | ||||||
| /// A reason for ignoring a test. | ||||||
| ignore_reason: Option<String>, | ||||||
| /// The number of frames to render. When greater than 1, the test function receives an | ||||||
| /// additional `u32` argument indicating the current frame index. The generated test will | ||||||
| /// loop `frame_count` times, resetting the context before each frame. Only the final | ||||||
| /// frame is checked against the reference image. | ||||||
| frame_count: u32, | ||||||
| } | ||||||
|
|
||||||
| impl Default for Arguments { | ||||||
|
|
@@ -61,6 +66,7 @@ impl Default for Arguments { | |||||
| no_ref: false, | ||||||
| diff_pixels: 0, | ||||||
| ignore_reason: None, | ||||||
| frame_count: 1, | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -158,8 +164,28 @@ pub(crate) fn vello_test_inner(attr: TokenStream, item: TokenStream) -> TokenStr | |||||
| ignore_reason, | ||||||
| no_ref, | ||||||
| diff_pixels, | ||||||
| frame_count, | ||||||
| } = parse_args(&attrs); | ||||||
|
|
||||||
| let has_frame_count = frame_count > 1; | ||||||
|
|
||||||
| let test_fn_call = if has_frame_count { | ||||||
| quote! { #input_fn_name(&mut ctx, frame); } | ||||||
| } else { | ||||||
| quote! { #input_fn_name(&mut ctx); } | ||||||
| }; | ||||||
|
|
||||||
| let width_f64 = width as f64; | ||||||
| let height_f64 = height as f64; | ||||||
| let draw_bg_snippet = if !transparent { | ||||||
| quote! { | ||||||
| ctx.set_paint(vello_common::color::palette::css::WHITE); | ||||||
| ctx.fill_rect(&vello_common::kurbo::Rect::new(0.0, 0.0, #width_f64, #height_f64)); | ||||||
| } | ||||||
| } else { | ||||||
| quote! {} | ||||||
| }; | ||||||
|
|
||||||
| // Wasm doesn't have access to the filesystem. For wasm, inline the snapshot bytes into the | ||||||
| // binary. | ||||||
| let reference_image_name = Ident::new( | ||||||
|
|
@@ -276,9 +302,15 @@ pub(crate) fn vello_test_inner(attr: TokenStream, item: TokenStream) -> TokenStr | |||||
| }; | ||||||
| use vello_cpu::{RenderContext, RenderMode}; | ||||||
|
|
||||||
| let mut ctx = get_ctx::<RenderContext>(#width, #height, #transparent, #num_threads, #level, #render_mode, false); | ||||||
| #input_fn_name(&mut ctx); | ||||||
| ctx.flush(); | ||||||
| let mut ctx = get_ctx::<RenderContext>(#width, #height, #num_threads, #level, #render_mode, false); | ||||||
| for frame in 0..#frame_count { | ||||||
| if frame > 0 { | ||||||
| ctx.reset() | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand the reasoning here?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be semicolons after resetting? Sorry for removing the spaces (now fixed). |
||||||
| } | ||||||
| #draw_bg_snippet | ||||||
| #test_fn_call | ||||||
| ctx.flush(); | ||||||
| } | ||||||
| if !#no_ref { | ||||||
| check_ref(&ctx, #input_fn_name_str, #fn_name_str, #tolerance, #diff_pixels, #is_reference, #reference_image_name); | ||||||
| } | ||||||
|
|
@@ -462,9 +494,15 @@ pub(crate) fn vello_test_inner(attr: TokenStream, item: TokenStream) -> TokenStr | |||||
| use crate::renderer::HybridRenderer; | ||||||
| use vello_cpu::RenderMode; | ||||||
|
|
||||||
| let mut ctx = get_ctx::<HybridRenderer>(#width, #height, #transparent, 0, "fallback", RenderMode::OptimizeSpeed, false); | ||||||
| #input_fn_name(&mut ctx); | ||||||
| ctx.flush(); | ||||||
| let mut ctx = get_ctx::<HybridRenderer>(#width, #height, 0, "fallback", RenderMode::OptimizeSpeed, false); | ||||||
| for frame in 0..#frame_count { | ||||||
| if frame > 0 { | ||||||
| ctx.reset() | ||||||
| } | ||||||
| #draw_bg_snippet | ||||||
| #test_fn_call | ||||||
| ctx.flush(); | ||||||
| } | ||||||
| if !#no_ref { | ||||||
| check_ref(&ctx, #input_fn_name_str, #hybrid_fn_name_str, #hybrid_tolerance, #diff_pixels, false, #reference_image_name); | ||||||
| } | ||||||
|
|
@@ -480,9 +518,15 @@ pub(crate) fn vello_test_inner(attr: TokenStream, item: TokenStream) -> TokenStr | |||||
| use crate::renderer::HybridRenderer; | ||||||
| use vello_cpu::RenderMode; | ||||||
|
|
||||||
| let mut ctx = get_ctx::<HybridRenderer>(#width, #height, #transparent, 0, "fallback", RenderMode::OptimizeSpeed, true); | ||||||
| #input_fn_name(&mut ctx); | ||||||
| ctx.flush(); | ||||||
| let mut ctx = get_ctx::<HybridRenderer>(#width, #height, 0, "fallback", RenderMode::OptimizeSpeed, true); | ||||||
| for frame in 0..#frame_count { | ||||||
| if frame > 0 { | ||||||
| ctx.reset() | ||||||
| } | ||||||
| #draw_bg_snippet | ||||||
| #test_fn_call | ||||||
| ctx.flush(); | ||||||
| } | ||||||
| if !#no_ref { | ||||||
| check_ref(&ctx, #input_fn_name_str, #hybrid_constrained_fn_name_str, #hybrid_tolerance, #diff_pixels, false, #reference_image_name); | ||||||
| } | ||||||
|
|
@@ -498,9 +542,15 @@ pub(crate) fn vello_test_inner(attr: TokenStream, item: TokenStream) -> TokenStr | |||||
| use crate::renderer::HybridRenderer; | ||||||
| use vello_cpu::RenderMode; | ||||||
|
|
||||||
| let mut ctx = get_ctx::<HybridRenderer>(#width, #height, #transparent, 0, "fallback", RenderMode::OptimizeSpeed, false); | ||||||
| #input_fn_name(&mut ctx); | ||||||
| ctx.flush(); | ||||||
| let mut ctx = get_ctx::<HybridRenderer>(#width, #height, 0, "fallback", RenderMode::OptimizeSpeed, false); | ||||||
| for frame in 0..#frame_count { | ||||||
| if frame > 0 { | ||||||
| ctx.reset() | ||||||
| } | ||||||
| #draw_bg_snippet | ||||||
| #test_fn_call | ||||||
| ctx.flush(); | ||||||
| } | ||||||
| if !#no_ref { | ||||||
| check_ref(&ctx, #input_fn_name_str, #webgl_fn_name_str, #hybrid_tolerance, #diff_pixels, false, #reference_image_name); | ||||||
| } | ||||||
|
|
@@ -533,6 +583,9 @@ fn parse_args(attribute_input: &AttributeInput) -> Arguments { | |||||
| "hybrid_tolerance" => { | ||||||
| args.hybrid_tolerance = parse_int_lit::<u8>(expr, "hybrid_tolerance"); | ||||||
| } | ||||||
| "frame_count" => { | ||||||
| args.frame_count = parse_int_lit::<u32>(expr, "frame_count"); | ||||||
| } | ||||||
| _ => panic!("unknown pair attribute {key_str}"), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -547,3 +547,34 @@ fn composite_to_pixmap_at_offset() { | |
| "composite_to_pixmap_at_offset result should match direct rendering" | ||
| ); | ||
| } | ||
|
|
||
| /// Ensure that fast path strips from the last frame don't leak into the next one. | ||
| #[vello_test(transparent, frame_count = 2)] | ||
| fn multi_frame_basic(ctx: &mut impl Renderer, frame: u32) { | ||
| let rect = Rect::new(15.0, 15.0, 85.0, 85.0); | ||
|
|
||
| if frame == 0 { | ||
| ctx.set_paint(RED.with_alpha(0.5)); | ||
| } else { | ||
| ctx.set_paint(BLUE.with_alpha(0.5)); | ||
| } | ||
|
|
||
| ctx.fill_rect(&rect); | ||
| } | ||
|
|
||
| /// Ensure that wide tiles are properly reset between two frames. | ||
| #[vello_test(transparent, frame_count = 2)] | ||
| fn multi_frame_with_clip_layer(ctx: &mut impl Renderer, frame: u32) { | ||
| let rect = Rect::new(15.0, 15.0, 85.0, 85.0); | ||
|
|
||
| ctx.push_clip_layer(&rect.to_path(0.1)); | ||
|
|
||
| if frame == 0 { | ||
| ctx.set_paint(RED.with_alpha(0.5)); | ||
| } else { | ||
| ctx.set_paint(BLUE.with_alpha(0.5)); | ||
| } | ||
|
|
||
| ctx.fill_rect(&rect); | ||
| ctx.pop_layer(); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about adding more tests to check for state leaks, or even better a single test (or unify above tests) where you'd have clip, opacity layer, blends, etc.? |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about snapshotting all frames into a single long strip and then comparing the whole thing? I think this way we could verify that all frames in the sequence have not changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess this would be not ideal if we have some tests that rely on many frames to trigger a bug, but I wouldn't mind changing it. @taj-p What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snapshotting across multiple frames increases our testing coverage in that we are also asserting on intermediate states. Perhaps it can be an additional flag to specify? Depending on what tests you want to add at this stage, we can decide on whether to default on or default off.
The existing test suite seems like it doesn't need it but I can imagine it might being wanted in the future (if we're unsure, we could leave it till then).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t mind adding an extra flag for this. As for test coverage, I don’t think we’ll have many tests that check using multiple frames, but I’d definitely prefer to keep snapshotting all frames behind a flag. My thinking is that the final output might be correct even if intermediate frames are not. Anyway, that’s just an assumption.