Skip to content

Add ImageAlphaType#121

Merged
sagudev merged 3 commits into
linebender:mainfrom
sagudev:alpha_type
Aug 29, 2025
Merged

Add ImageAlphaType#121
sagudev merged 3 commits into
linebender:mainfrom
sagudev:alpha_type

Conversation

@sagudev

@sagudev sagudev commented Aug 23, 2025

Copy link
Copy Markdown
Contributor

Similar as in #120, although I think this one is even more important as vello_cpu currently accepts only premultiplied images while vello classic only unpremultiplied. This will hopefully allow vello_cpu to use peniko's image type natively.

@DJMcNab

DJMcNab commented Aug 25, 2025

Copy link
Copy Markdown
Member

The obvious question that comes to mind is whether this should be part of ImageFormat (e.g. in wgpu, you have R8Unorm and R8Snorm). Concretely, for formats other than Rgba8/Bgra8, the pre-multiplication algorithm is not necessarily the same (so you can't write a generic premultiply/unpremultiply method without also having awareness of the format).

@sagudev

sagudev commented Aug 25, 2025

Copy link
Copy Markdown
Contributor Author

The obvious question that comes to mind is whether this should be part of ImageFormat (e.g. in wgpu, you have R8Unorm and R8Snorm). Concretely, for formats other than Rgba8/Bgra8, the pre-multiplication algorithm is not necessarily the same (so you can't write a generic premultiply/unpremultiply method without also having awareness of the format).

Webrender has separate AlphaType and ImageFormat and that's what I've been following too. Users will likely use them together, but I think still it's worth to have them separated, because they represent different stuff (one can use ImageFormat without knowing AlphaType, but inverse is not true).

@DJMcNab

DJMcNab commented Aug 25, 2025

Copy link
Copy Markdown
Member

one can use ImageFormat without knowing AlphaType, but inverse is not true

Oh, interesting! What operations can you perform when you have the format of the image but not whether the image's alpha is premultiplied? I guess you can do things like a hue shift if you know you have a HSL-like color?

@sagudev

sagudev commented Aug 25, 2025

Copy link
Copy Markdown
Contributor Author

Getting buffer size, resizing (crop), making it opaque (although here one might want to unpremultiply before), interpolation (debatable if it's "correct"), flipY, basically any op where you are only interested in pixels not specific pixel content.

@DJMcNab

DJMcNab commented Aug 26, 2025

Copy link
Copy Markdown
Member

It seems to me that all of those methods (except again for the operations which you already note are likely to be incorrect if done blindly to the premultiplication state) only care about the byte size of each texel. And so the only advantage from this proposed scheme for that way around is your match format { ImageFormat::Rgba8 | ImageFormat::PremulRgba8 => 4, ... } would have half as many arms.

But if we have that byte_size method ourselves on TextureFormat (which I think we probably should), this small advantage also goes away.

I would like to understand why WebRender stores this separately before making the decision here.

But for me personally, I would be more likely to just accept the addition of PremulRgba8/PremulBgra8. This is something else we'll want to discuss in office hours tomorrow.

@sagudev

sagudev commented Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Maybe we could keep at least some orthogonality of types by having something like:

enum ImageFormat {
    Rgba8(AlphaType),
    Bgra8(AlphaType),
}

@DJMcNab DJMcNab left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We discussed this in #office hours > 2025-08-28, and decided that we can support this. The deciding factors were:

  1. We're never going to support cylindrical colour spaces here, so the algorithm for premultiply/unpremultiply is always "take every fourth component, and multiply the other three by it".
  2. We'll have to revisit so much stuff when we get to non-byte based component thing that we can talk about this again then.

Fwiw, my preference is still to fold this into the ImageFormat enum, but the level of badness of having it seperate is (currently) sufficiently lower that I'm happy to approve this with the current implementation.
Obviously, this version is also an ergonomics hit when creating images, as you'll now need to specify the alpha type and format separately.

There are reasonable arguments for providing "premultiply"/"unpremultiply" methods here, but those shouldn't be in this PR.

This does also need a changelog entry.
Thanks for adding this; sorry it's taken so long to get this approved; it's hard to find consensus around this stuff, as Peniko is sufficiently in the background.

Comment thread src/image.rs Outdated
Comment thread src/image.rs Outdated
Comment thread src/image.rs Outdated
sagudev and others added 3 commits August 29, 2025 17:53
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>

@DJMcNab DJMcNab left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@sagudev sagudev added this pull request to the merge queue Aug 29, 2025
Merged via the queue into linebender:main with commit 370b0ae Aug 29, 2025
15 checks passed
@sagudev sagudev deleted the alpha_type branch August 29, 2025 16:55
DJMcNab added a commit to DJMcNab/peniko that referenced this pull request Oct 1, 2025
github-merge-queue Bot pushed a commit that referenced this pull request Oct 1, 2025
This pointed to `#121` instead of #115

Part of final checks for #140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants