Skip to content

Add support for non-isolated blending in vello_cpu#1159

Merged
LaurenzV merged 8 commits intomainfrom
non_isolated_blend
Oct 30, 2025
Merged

Add support for non-isolated blending in vello_cpu#1159
LaurenzV merged 8 commits intomainfrom
non_isolated_blend

Conversation

@LaurenzV
Copy link
Copy Markdown
Collaborator

@LaurenzV LaurenzV commented Aug 13, 2025

I also removed the blend optimization since IMO it causes more troubles than it brings benefits.

Reference images match the ones in tiny-skia.

@LaurenzV LaurenzV requested a review from sagudev August 13, 2025 10:36
sagudev
sagudev reviewed Aug 13, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@sagudev sagudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am testing this on servo WPT and I did a quick look. I think this should be reviewed by someone with deeper understanding of sparse strips.

Comment thread sparse_strips/vello_cpu/src/render.rs
@sagudev

This comment was marked as outdated.

Sign in to view
sagudev
sagudev reviewed Aug 13, 2025
View reviewed changes
Comment thread sparse_strips/vello_sparse_tests/snapshots/compose_non_isolated_copy.png
sagudev
sagudev reviewed Aug 13, 2025
View reviewed changes
Comment thread sparse_strips/vello_sparse_tests/snapshots/compose_non_isolated_dest_in.png
@sagudev
Copy link
Copy Markdown
Contributor

sagudev commented Aug 14, 2025 •
edited
Loading

New proper results: https://github.com/sagudev/servo/actions/runs/16947826831/attempts/1#summary-48036644654.
So while canvas has actually isolated blending, this PR allows us to use less layers (more perf) and properly respect clipping.

We should still fix #1119 to match Vello classic.

@nicoburns nicoburns added the C-cpu Applies to the vello_cpu crate label Aug 15, 2025
@sagudev sagudev mentioned this pull request Aug 22, 2025
Open
@sagudev sagudev mentioned this pull request Oct 29, 2025
Open
@LaurenzV
Copy link
Copy Markdown
Collaborator Author

LaurenzV commented Oct 30, 2025

@sagudev Do you think you would have time to do a high-level review and approve if it looks okay? Otherwise I feel like this PR will remain in limbo forever. 😄 It's not that big of a change so I don't think landing this should cause any trouble, especially since existing blending tests work just fine.

@sagudev sagudev self-requested a review October 30, 2025 10:43
sagudev
sagudev approved these changes Oct 30, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@sagudev sagudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright from higher perspective.

@LaurenzV LaurenzV enabled auto-merge October 30, 2025 21:54
@LaurenzV LaurenzV added this pull request to the merge queue Oct 30, 2025
Merged via the queue into main with commit 186106e Oct 30, 2025
17 checks passed
@LaurenzV LaurenzV deleted the non_isolated_blend branch October 30, 2025 22:09
T-256
T-256 reviewed Oct 31, 2025
View reviewed changes
Comment thread sparse_strips/vello_cpu/src/render.rs
self.reset_paint_transform();
#[cfg(feature = "text")]
self.glyph_caches.as_mut().unwrap().maintain();
self.blend_mode = BlendMode::new(Mix::Normal, Compose::SrcOver);
Copy link
Copy Markdown
Contributor

@T-256 T-256 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't use BlendMode::default() consistently in here and other places?

Copy link
Copy Markdown
Collaborator Author

@LaurenzV LaurenzV Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, because right now the default is Mix::Clip which I want to avoid using.

Copy link
Copy Markdown
Contributor

@sagudev sagudev Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mix::Clip is not default anymore: https://docs.rs/peniko/latest/src/peniko/blend.rs.html#184-191, see linebender/peniko#144.

Copy link
Copy Markdown
Collaborator Author

@LaurenzV LaurenzV Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I guess we can change it then!

@grebmeg
Copy link
Copy Markdown
Collaborator

grebmeg commented Nov 12, 2025

Hi @LaurenzV, would you mind updating the CHANGELOG.md files in both vello_common and vello_cpu to reflect the recent updates? It’ll ensure everything is documented properly before release. Really appreciate it! 🙏

@LaurenzV
Copy link
Copy Markdown
Collaborator Author

LaurenzV commented Nov 13, 2025

Oh yeah sure, will try to do that soon!

@LaurenzV
Copy link
Copy Markdown
Collaborator Author

LaurenzV commented Nov 13, 2025

I opened a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sagudev sagudev sagudev approved these changes

+1 more reviewer

@T-256 T-256 T-256 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C-cpu Applies to the vello_cpu crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@LaurenzV @sagudev @grebmeg @T-256 @nicoburns