Support setting AA method dynamically #400

armansito · 2023-10-31T06:14:32Z

This replaces the static anti-aliasing setting with a dynamic option in the form of two new settings (one used during pipeline creation and one used during a render):

The FullShaders collection now maintains up to 3 fine stage pipeline variants. This can be driven using a new optional RendererOptions field called preferred_antialiasing_method, which determines which fine stage pipeline variants should get instantiated at start up.
RenderParams now requires an AaConfig which selects which fine stage variant to use.
Added a new key binding (M) to the with_winit example to dynamically cycle through all 3 AA methods.

DJMcNab

Mostly some style considerations

examples/with_winit/src/lib.rs

DJMcNab · 2023-10-31T13:54:34Z

src/lib.rs

+
+    /// The anti-aliasing specialization that should be used when creating the pipelines. `None`
+    /// initializes all variants.
+    pub preferred_antialiasing_method: Option<AaConfig>,


Would a bitflags be better here?

I guess I'd need to consider what the use cases are for the different modes? Why would you choose one over another. Should it be user configurable?

A partial answer to this is that we think we'll probably want to end up rendering glyphs with area AA and general paths with MSAA. But that speaks to a setting that's even more dynamic than per-render.

Would a bitflags be better here?

A bitfield is probably a good idea. I changed this to use a struct-of-bools instead of bitflags for now.

I mainly wanted to provide a way for existing clients to avoid compiling the variants they don't need. Currently each AaConfig enum maps to exactly one fine stage permutation but this may evolve over time with more configs, such as a single pipeline that is capable of multiple AA modes. Even in that world it may be desirable to turn off AA logic in that pipeline if the client doesn't need it.

I guess I'd need to consider what the use cases are for the different modes? Why would you choose one over another. Should it be user configurable?

This is a great question. Each AA method has a certain trade-off: analytic area AA provides very high quality anti-aliasing but results in conflation artifacts with certain paths (#49) (see also the labyrinth and conflation_artifacts scenes). As @raphlinus said, this is likely the preferred way to render text where quality really matters and glyphs are unlikely to be crafted in a way that can result in conflation artifacts.

The MSAA methods OTOH do not suffer from conflation artifacts but the AA quality is noticeably poorer compared to analytic area. This may be a reasonable trade-off for an application that doesn't have control over all content (for example an SVG viewer or the Canvas API implementation in web a browser). In fact, Skia has had to disable area AA algorithms in the past following user complaints (see crbug.com/913223 and skbug.com/40038110) if you're interested in some of the history of this in Chromium).

Between MSAAx16 and MSAAx8 the trade-off is between performance and quality but maybe that gap will narrow as we invest more time into optimizations.

For now, I chose to provide the option but we can always revisit this.

DJMcNab · 2023-10-31T13:55:11Z

src/lib.rs

+    /// TODO: Consider evolving this so that the CPU stages can be configured dynamically via
+    /// `RenderParams`.


Should this be a doc comment rather than just a comment?

Done. To clarify, you wanted this to be a regular comment instead of a doc comment, correct?

src/render.rs

src/shaders.rs

raphlinus

Looks good. I agree with most of Daniel's comments, and just had some golfing to make the logic hopefully a little simpler - it feels just a bit clunky and I'm slightly worried about what will happen as the number of permutations grows. But this is definitely useful to have!

Of course, we might reduce permutations by having multiple aa methods present in the same shader, but one nice thing about this setup is that we can experiment with the effect of shared memory size (with effect on occupancy but also likely a nontrivial effect from just clearing the memory, especially as this isn't always optimized well).

raphlinus · 2023-10-31T19:05:00Z

src/lib.rs

+
+    /// The anti-aliasing specialization that should be used when creating the pipelines. `None`
+    /// initializes all variants.
+    pub preferred_antialiasing_method: Option<AaConfig>,


A partial answer to this is that we think we'll probably want to end up rendering glyphs with area AA and general paths with MSAA. But that speaks to a setting that's even more dynamic than per-render.

src/render.rs

raphlinus · 2023-10-31T19:06:20Z

src/render.rs

                        AaConfig::Msaa16 => crate::mask::make_mask_lut_16(),
                        AaConfig::Msaa8 => crate::mask::make_mask_lut(),
                        _ => unreachable!(),
                    };
                    let buf = recording.upload("mask lut", mask_lut);
                    self.mask_buf = Some(buf.into());
                }
+                let shader = match fine.aa_config {


Personally I'd call this fine_shader.

raphlinus · 2023-10-31T19:10:06Z

src/shaders.rs

+        for (i, aa_mode) in AA_MODES.iter().enumerate() {
+            if options
+                .preferred_antialiasing_method
+                .as_ref()


Could the as_ref be eliminated by comparing m != *aa_mode? It's Copy for this reason.

armansito · 2023-11-01T00:55:56Z

Looks good. I agree with most of Daniel's comments, and just had some golfing to make the logic hopefully a little simpler - it feels just a bit clunky and I'm slightly worried about what will happen as the number of permutations grows. But this is definitely useful to have!

Of course, we might reduce permutations by having multiple aa methods present in the same shader, but one nice thing about this setup is that we can experiment with the effect of shared memory size (with effect on occupancy but also likely a nontrivial effect from just clearing the memory, especially as this isn't always optimized well).

I agree that this is a little clunky and it would be good to figure out a better way to specify the desired pipeline permutations. For now, this is scoped to 3 AA settings and I imagine it wouldn't be too hard to extend this to an additional "dynamic" mode if we were to explore that.

DJMcNab

Not approving as I haven't had a chance to run this yet, but the code looks good.

There are a couple of small style points, one of which isn't necessarily an improvement (especially if it makes rustfmt break up the aa_modes across 20 lines, which it might accidentally do)

DJMcNab · 2023-11-01T07:14:42Z

examples/with_winit/src/lib.rs

@@ -356,6 +374,7 @@ fn run(
                    .unwrap_or(Color::BLACK),
                width,
                height,
+                antialiasing_method: aa_config,


I'd probably inline this (so that the comment above about base_colour logic is in the right place)

aa_config gets reused below so I wanted to have a single binding for it. I moved things around so that the comment is positioned better.

DJMcNab · 2023-11-01T07:37:45Z

src/shaders.rs

+            }
+            let (range_end_offset, label, aa_config) = match *msaa_info {
+                Some((label, config)) => (0, label, Some(config)),
+                None => (1, "fine_area", None),


(I had a big comment written out here, which GitHub mobile helpfully deleted)

But broadly, I was wondering if all of this should be inlined into aa_mode, i.e. make it a (bool, usize, &str, Option<&str>).

If we were doing that, I'd then replace the usize offset with the pipeline resources, so fine_area would be:
(support.area, &fine_resources[..7], "fine_area", None)

(Where 7 might not be the right number, but trying to check it lead to the deletion of the previous version of this comment)

Done. I just folded the offset into the tuple to keep the column width manageable.

This replaces the static anti-aliasing setting with a dynamic option in the form of two new settings (one used during pipeline creation and one used during a render): - The `FullShaders` collection now maintains up to 3 `fine` stage pipeline variants. This can be driven using a new optional `RendererOptions` field called `preferred_antialiasing_method`, which determines which fine stage pipeline variants should get instantiated at start up. - `RenderParams` now requires an `AaConfig` which selects which `fine` stage variant to use. - Added a new key binding (`M`) to the `with_winit` example to dynamically cycle through all 3 AA methods.

Instead of passing in a single optional mode to enable, the caller can now specify which set of AA methods to compile.

raphlinus

This needs some merge conflict fixing, but hopefully that should be easy. I haven't run this version but have run a previous version, so I'm ok as long as things didn't get broken. And if they did, that can be a followup PR :)

raphlinus · 2023-11-01T15:28:29Z

examples/with_winit/src/lib.rs

@@ -180,7 +182,11 @@ fn run(
                                stats.clear_min_and_max();
                            }
                            Some(VirtualKeyCode::M) => {
-                                aa_config_ix = (aa_config_ix + 1) % AA_CONFIGS.len();
+                                aa_config_ix = if modifiers.shift() {


Personally I would keep the mod n behavior (as the reverse is less discoverable), but that's a style preference. Note though that we do have +/- and mod n for cycling through the test scenes.

That's fair, the scene cycling logic is actually structured this way (though bound to arrow keys instead of using a modifier) so I just followed that here for consistency.

armansito requested review from raphlinus and DJMcNab October 31, 2023 06:14

armansito force-pushed the dynamic-aa-setting branch 2 times, most recently from 66bdcb1 to 951cacc Compare October 31, 2023 08:26

DJMcNab reviewed Oct 31, 2023

View reviewed changes

raphlinus approved these changes Oct 31, 2023

View reviewed changes

armansito force-pushed the dynamic-aa-setting branch from 951cacc to 2962959 Compare November 1, 2023 00:35

armansito requested a review from DJMcNab November 1, 2023 01:57

DJMcNab reviewed Nov 1, 2023

View reviewed changes

armansito added 3 commits November 1, 2023 08:24

MSAA config: address review comments

fba8317

Change preferred_antialiasing_method setting to a struct

ea29d30

Instead of passing in a single optional mode to enable, the caller can now specify which set of AA methods to compile.

raphlinus approved these changes Nov 1, 2023

View reviewed changes

MSAA config: address last round of comments

1efb3a5

armansito force-pushed the dynamic-aa-setting branch from c8dcbb5 to 1efb3a5 Compare November 1, 2023 15:39

Fix unused warning with the headless example

876a934

armansito merged commit 1e7f850 into main Nov 1, 2023
4 checks passed

armansito deleted the dynamic-aa-setting branch November 1, 2023 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting AA method dynamically #400

Support setting AA method dynamically #400

armansito commented Oct 31, 2023

DJMcNab left a comment

DJMcNab Oct 31, 2023

raphlinus Oct 31, 2023

armansito Oct 31, 2023 •

edited

Loading

DJMcNab Oct 31, 2023

armansito Oct 31, 2023

raphlinus left a comment

raphlinus Oct 31, 2023

raphlinus Oct 31, 2023

armansito Oct 31, 2023

raphlinus Oct 31, 2023

armansito Oct 31, 2023

armansito commented Nov 1, 2023

DJMcNab left a comment

DJMcNab Nov 1, 2023

armansito Nov 1, 2023

DJMcNab Nov 1, 2023

armansito Nov 1, 2023

raphlinus left a comment

raphlinus Nov 1, 2023

armansito Nov 1, 2023

		/// TODO: Consider evolving this so that the CPU stages can be configured dynamically via
		/// `RenderParams`.

Support setting AA method dynamically #400

Support setting AA method dynamically #400

Conversation

armansito commented Oct 31, 2023

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito commented Nov 1, 2023

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito Oct 31, 2023 •

edited

Loading