-
Notifications
You must be signed in to change notification settings - Fork 181
sparse_strips: Use peniko::Brush #1253
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
use crate::pixmap::Pixmap; | ||
use alloc::sync::Arc; | ||
use peniko::{ | ||
Gradient, ImageQuality, ImageSampler, | ||
Gradient, | ||
color::{AlphaColor, PremulRgba8, Srgb}, | ||
}; | ||
|
||
|
@@ -77,84 +77,68 @@ pub enum ImageSource { | |
OpaqueId(ImageId), | ||
} | ||
|
||
/// An image. | ||
#[derive(Debug, Clone)] | ||
pub struct Image { | ||
/// The underlying pixmap of the image. | ||
pub source: ImageSource, | ||
/// Extend mode in the horizontal direction. | ||
pub x_extend: peniko::Extend, | ||
/// Extend mode in the vertical direction. | ||
pub y_extend: peniko::Extend, | ||
/// Hint for desired rendering quality. | ||
pub quality: ImageQuality, | ||
} | ||
|
||
impl Image { | ||
/// Convert a [`peniko::ImageBrush`] to an [`Image`]. | ||
impl ImageSource { | ||
/// Convert a [`peniko::ImageData`] to an [`ImageSource`]. | ||
/// | ||
/// This is a somewhat lossy conversion, as the image data data is transformed to | ||
/// [premultiplied RGBA8](`PremulRgba8`). | ||
/// | ||
/// # Panics | ||
/// | ||
/// This panics if `image` has a `width` or `height` greater than `u16::MAX`. | ||
pub fn from_peniko_image(brush: &peniko::ImageBrush) -> Self { | ||
pub fn from_peniko_image_data(image: &peniko::ImageData) -> Self { | ||
// TODO: how do we deal with `peniko::ImageFormat` growing? See also | ||
// <https://github.com/linebender/vello/pull/996#discussion_r2080510863>. | ||
if brush.image.format != peniko::ImageFormat::Rgba8 { | ||
unimplemented!("Unsupported image format: {:?}", brush.image.format); | ||
} | ||
if brush.image.alpha_type != peniko::ImageAlphaType::Alpha { | ||
unimplemented!("Unsupported image alpha type: {:?}", brush.image.alpha_type); | ||
} | ||
let do_alpha_multiply = image.alpha_type != peniko::ImageAlphaType::AlphaPremultiplied; | ||
|
||
assert!( | ||
brush.image.width <= u16::MAX as u32 && brush.image.height <= u16::MAX as u32, | ||
image.width <= u16::MAX as u32 && image.height <= u16::MAX as u32, | ||
"The image is too big. Its width and height can be no larger than {} pixels.", | ||
u16::MAX, | ||
); | ||
let width = brush.image.width.try_into().unwrap(); | ||
let height = brush.image.height.try_into().unwrap(); | ||
let ImageSampler { | ||
x_extend, | ||
y_extend, | ||
quality, | ||
alpha: global_alpha, | ||
} = brush.sampler; | ||
|
||
#[expect(clippy::cast_possible_truncation, reason = "deliberate quantization")] | ||
let global_alpha = u16::from((global_alpha * 255. + 0.5) as u8); | ||
let width = image.width.try_into().unwrap(); | ||
let height = image.height.try_into().unwrap(); | ||
|
||
// TODO: SIMD | ||
#[expect(clippy::cast_possible_truncation, reason = "This cannot overflow.")] | ||
let pixels = brush | ||
.image | ||
let pixels = image | ||
.data | ||
.data() | ||
.chunks_exact(4) | ||
.map(|rgba| { | ||
let alpha = ((u16::from(rgba[3]) * global_alpha) / 255) as u8; | ||
let multiply = |component| ((u16::from(alpha) * u16::from(component)) / 255) as u8; | ||
PremulRgba8 { | ||
r: multiply(rgba[0]), | ||
g: multiply(rgba[1]), | ||
b: multiply(rgba[2]), | ||
a: alpha, | ||
.map(|pixel| { | ||
let rgba: [u8; 4] = match image.format { | ||
peniko::ImageFormat::Rgba8 => pixel.try_into().unwrap(), | ||
peniko::ImageFormat::Bgra8 => [pixel[2], pixel[1], pixel[0], pixel[3]], | ||
sagudev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
format => unimplemented!("Unsupported image format: {format:?}"), | ||
}; | ||
let alpha = u16::from(rgba[3]); | ||
let multiply = |component| ((alpha * u16::from(component)) / 255) as u8; | ||
if do_alpha_multiply { | ||
PremulRgba8 { | ||
r: multiply(rgba[0]), | ||
g: multiply(rgba[1]), | ||
b: multiply(rgba[2]), | ||
a: rgba[3], | ||
} | ||
} else { | ||
PremulRgba8 { | ||
r: rgba[0], | ||
g: rgba[1], | ||
b: rgba[2], | ||
a: rgba[3], | ||
} | ||
Comment on lines
+108
to
+129
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. cc @ajakubowicz-canva re: image premultiply/unpremultiply |
||
} | ||
}) | ||
.collect(); | ||
let pixmap = Pixmap::from_parts(pixels, width, height); | ||
|
||
Self { | ||
source: ImageSource::Pixmap(Arc::new(pixmap)), | ||
x_extend, | ||
y_extend, | ||
quality, | ||
} | ||
Self::Pixmap(Arc::new(pixmap)) | ||
} | ||
} | ||
|
||
/// An image. | ||
pub type Image = peniko::ImageBrush<ImageSource>; | ||
|
||
/// A premultiplied color. | ||
#[derive(Debug, Clone, PartialEq, Copy)] | ||
pub struct PremulColor { | ||
|
@@ -193,30 +177,4 @@ impl PremulColor { | |
} | ||
|
||
/// A kind of paint that can be used for filling and stroking shapes. | ||
#[derive(Debug, Clone)] | ||
pub enum PaintType { | ||
/// A solid color. | ||
Solid(AlphaColor<Srgb>), | ||
/// A gradient. | ||
Gradient(Gradient), | ||
/// An image. | ||
Image(Image), | ||
} | ||
|
||
impl From<AlphaColor<Srgb>> for PaintType { | ||
fn from(value: AlphaColor<Srgb>) -> Self { | ||
Self::Solid(value) | ||
} | ||
} | ||
|
||
impl From<Gradient> for PaintType { | ||
fn from(value: Gradient) -> Self { | ||
Self::Gradient(value) | ||
} | ||
} | ||
|
||
impl From<Image> for PaintType { | ||
fn from(value: Image) -> Self { | ||
Self::Image(value) | ||
} | ||
} | ||
pub type PaintType = peniko::Brush<Image, Gradient>; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Doesn't have to be addressed in this PR, but I think it would be smarter to apply the opacity during encoding time instead of at sampling time (using
Arc::make_mut(pixmap)
)? I'm concerned that adding an if branch for that for sampling will be pretty bad for performance, especially when doing bilinear/bicubic filtering.I also thought we already support image alphas, but seems like I remember wrong...
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.
And I think using
unimplented
is fine for now, but before the first minor release we should probably replace all panics with a warning instead.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 am not sure this would work, for cases where one wants to put same image with different alpha (this is currently possible in vello classic and is expressed as alpha in ImageSampler).
Yes, that's how we already have other stuff, so I just continued the pattern.
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.
Well, in this case can just do the encoding twice? I think that applying an opacity to an image is already pretty rare, placing the same image twice with different alphas is even more rare. I don't think it's worth sacrificing rendering performance for all images just to account for those special cases.
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 wonder if we can do this in creation of image sampler (
ImagePainterData
) using generics rather than doing in sampling (which is hot). This way only do "dispatch" once.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.
In theory yes, but the main problem is that we already have lots of generics in
vello-cpu
which are already impacting compile times (and probably build size) pretty badly. :/ So I would be careful about adding even more, especially since each additional generic serves as a multiplying factor.