vello_hybrid: support blurred rounded rects fast path#1610
Conversation
|
This was done with the assistance of Codex (5.5, high). I've reviewed it and think it is right, but I don't know all of the hybrid internals to be sure I didn't miss something. |
|
Does this address #1373? |
aa87886 to
8932902
Compare
LaurenzV
left a comment
There was a problem hiding this comment.
LGTM overall, from some testing on my tablet I wasn't able to determine any slowdown from adding this additional shader code.
| final_color = alpha * gradient_color; | ||
| } else if paint_type == PAINT_TYPE_BLURRED_ROUNDED_RECT { | ||
| let paint_tex_idx = in.paint_and_rect_flag & PAINT_TEXTURE_INDEX_MASK; | ||
| let blurred_rect = unpack_blurred_rounded_rect(paint_tex_idx); |
There was a problem hiding this comment.
I can probably just do it myself depending on which PR gets merged first, but just as a note the way we process paint types is likely to change here: https://github.com/linebender/vello/pull/1619/changes
| // Approximation for the convolution of a gaussian filter with a rounded rectangle. | ||
| fn calculate_blurred_rounded_rect(fragment_pos: vec2<f32>, rect: BlurredRoundedRect) -> vec4<f32> { | ||
| let local_xy = rect.transform * fragment_pos + rect.translate; | ||
| let y = local_xy.y - 0.5 * rect.height; |
There was a problem hiding this comment.
Can you maybe add a note that 0.5 and 0.0 correspond to v0 and v1 in the CPU code? And in general, a small comment that this shader is modelled after the vello_cpu variant (instead of vello itself)
| } | ||
|
|
||
| fn try_fast_rect(&mut self, rect: &Rect) -> bool { | ||
| if self.fast_rect_bounds(rect).is_none() { |
There was a problem hiding this comment.
Now we are doing the computation twice, once here and then again in try_fast_rect_with_paint. Could we not change it so that
fast_rect_boundsjust returns a normal Optionkurbo::Rect (since the coordinates are f64 anyway) and then- Turn
try_fast_rect_with_paintinto a method that takes the calculated bounds directly and just pushes it to the fast strips buffer (and rename into something likepush_fast_rect)?
There was a problem hiding this comment.
Improved now, I think.
| return; | ||
| } | ||
|
|
||
| if is_axis_aligned(&ctx.render_state.transform) && ctx.aliasing_threshold.is_none() { |
There was a problem hiding this comment.
I wonder whether all of this can be deduplicated somehow, but maybe better for a follow-up.
8932902 to
bfdf9a0
Compare
grebmeg
left a comment
There was a problem hiding this comment.
I checked on a low-tier device, a Samsung Galaxy A05s, to make sure the PR doesn’t degrade performance, and it looks like it doesn’t. I ran fairly intensive tests with 10 warm-up runs and 100 iterations for each benchmark.
From the results, there doesn’t seem to be any noticeable degradation, although benchmarking GPU performance properly is always tricky... In interactive mode, it feels fine as well.
Ideally, we’d introduce feature gating for such features, but we currently don’t have the infrastructure for that. The long-term plan is to move to more specialized shaders, but until then, I still think it’s better to have the feature gating in place since it doesn’t take much effort to implement. Otherwise, users might not even use some of these features, but still end up paying the performance cost for them.
You'll have to sort out the CI cost / impact for this due to how CI checks features. That makes additional features more painful (and it is already bad). |
This wires Hybrid through the existing shared
vello_commonblurred rounded rect encoding, adds GPU/WebGL encoded paint upload support, and teaches the sparse strip shader to evaluate the analytic blurred rounded rect paint. Root-level unclipped cases use the existing direct rectangle fast path; other supported cases fall back through the normal strip/coarse path.Also adds a small Hybrid
winitdemo scene, adapted from the Classic test scene, so the feature can be inspected interactively.