-
Notifications
You must be signed in to change notification settings - Fork 142
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
Let's add images #293
Let's add images #293
Changes from all commits
165b3a0
a858578
d12b711
37f05b8
923c949
a6307a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
use std::collections::HashMap; | ||
use std::path::{Path, PathBuf}; | ||
use std::sync::Arc; | ||
|
||
use vello::peniko::{Blob, Format, Image}; | ||
|
||
/// Simple hack to support loading images for examples. | ||
#[derive(Default)] | ||
pub struct ImageCache { | ||
files: HashMap<PathBuf, Image>, | ||
bytes: HashMap<usize, Image>, | ||
} | ||
|
||
impl ImageCache { | ||
pub fn new() -> Self { | ||
Self::default() | ||
} | ||
|
||
pub fn from_file(&mut self, path: impl AsRef<Path>) -> anyhow::Result<Image> { | ||
let path = path.as_ref(); | ||
if let Some(image) = self.files.get(path) { | ||
Ok(image.clone()) | ||
} else { | ||
let data = std::fs::read(path)?; | ||
let image = decode_image(&data)?; | ||
self.files.insert(path.to_owned(), image.clone()); | ||
Ok(image) | ||
} | ||
} | ||
|
||
pub fn from_bytes(&mut self, key: usize, bytes: &[u8]) -> anyhow::Result<Image> { | ||
if let Some(image) = self.bytes.get(&key) { | ||
Ok(image.clone()) | ||
} else { | ||
let image = decode_image(bytes)?; | ||
self.bytes.insert(key, image.clone()); | ||
Ok(image) | ||
} | ||
} | ||
} | ||
|
||
fn decode_image(data: &[u8]) -> anyhow::Result<Image> { | ||
let image = image::io::Reader::new(std::io::Cursor::new(data)) | ||
.with_guessed_format()? | ||
.decode()?; | ||
let width = image.width(); | ||
let height = image.height(); | ||
let data = Arc::new(image.into_rgba8().into_vec()); | ||
let blob = Blob::new(data); | ||
Ok(Image::new(blob, Format::Rgba8, width, height)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ use vello::kurbo::{Affine, BezPath, Ellipse, PathEl, Point, Rect}; | |
use vello::peniko::*; | ||
use vello::*; | ||
|
||
const PIET_LOGO_IMAGE: &[u8] = include_bytes!("../../assets/piet-logo.png"); | ||
|
||
macro_rules! scene { | ||
($name: ident) => { | ||
scene!($name: false) | ||
|
@@ -97,6 +99,13 @@ fn cardioid_and_friends(sb: &mut SceneBuilder, _: &mut SceneParams) { | |
} | ||
|
||
fn animated_text(sb: &mut SceneBuilder, params: &mut SceneParams) { | ||
// Uses the static array address as a cache key for expedience. Real code | ||
// should use a better strategy. | ||
let piet_logo = params | ||
.images | ||
.from_bytes(PIET_LOGO_IMAGE.as_ptr() as usize, PIET_LOGO_IMAGE) | ||
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. From a strict provenance perspective, this should use pointer::addr (whereas 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. This is just my expedient hacky solution to support image loading for examples. Being an example though, it probably does deserve a “don’t do this in real code!” comment. Since the address is never converted back into a pointer, I don’t think provenance is an issue. Size likely would be on CHERI architectures though. 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. Yeah, to be clear, there's no soundness issue here. This is not at all important, I just recognised this as an opportunity to brush up my understanding of strict aliasing 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. Fair enough! Pointer to int casts are always worth calling out in code review just to have some discussion logged for future reference. |
||
.unwrap(); | ||
|
||
use PathEl::*; | ||
let rect = Rect::from_origin_size(Point::new(0.0, 0.0), (1000.0, 1000.0)); | ||
let star = [ | ||
|
@@ -184,6 +193,10 @@ fn animated_text(sb: &mut SceneBuilder, params: &mut SceneParams) { | |
None, | ||
&star, | ||
); | ||
sb.draw_image( | ||
&piet_logo, | ||
Affine::translate((550.0, 250.0)) * Affine::skew(-20f64.to_radians().tan(), 0.0), | ||
); | ||
} | ||
|
||
fn brush_transform(sb: &mut SceneBuilder, params: &mut SceneParams) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,6 +126,13 @@ fn write_grad(ty: u32, index: u32, info_offset: u32) { | |
cmd_offset += 3u; | ||
} | ||
|
||
fn write_image(info_offset: u32) { | ||
alloc_cmd(2u); | ||
ptcl[cmd_offset] = CMD_IMAGE; | ||
ptcl[cmd_offset + 1u] = info_offset; | ||
cmd_offset += 2u; | ||
} | ||
|
||
fn write_begin_clip() { | ||
alloc_cmd(1u); | ||
ptcl[cmd_offset] = CMD_BEGIN_CLIP; | ||
|
@@ -377,6 +384,13 @@ fn main( | |
write_grad(CMD_RAD_GRAD, index, info_offset); | ||
} | ||
} | ||
// DRAWTAG_FILL_IMAGE | ||
case 0x248u: { | ||
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. It's unfortunate that naga still doesn't seem to support non-literal const-expressions as a case selector (which is supported by the spec). 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. I had to change those values several times while playing with the encoding and missing one of these actually led to a TDR. So yeah, this is supremely annoying. 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. On the naga side this seems to tracked by gfx-rs/naga#2106. I'm putting a mention here and I'll ping the naga issue as well as this would be a huge quality of life improvement (I've wanted this in other projects as well). 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. Thanks for the link. This will hopefully make the cut for the next release. |
||
let linewidth = bitcast<f32>(info_bin_data[di]); | ||
if write_path(tile, linewidth) { | ||
write_image(di + 1u); | ||
} | ||
} | ||
// DRAWTAG_BEGIN_CLIP | ||
case 0x9u: { | ||
if tile.segments == 0u && tile.backdrop == 0 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,7 +113,9 @@ fn main( | |
var matrx: vec4<f32>; | ||
var translate: vec2<f32>; | ||
var linewidth = bbox.linewidth; | ||
if linewidth >= 0.0 || tag_word == DRAWTAG_FILL_LIN_GRADIENT || tag_word == DRAWTAG_FILL_RAD_GRADIENT { | ||
if linewidth >= 0.0 || tag_word == DRAWTAG_FILL_LIN_GRADIENT || tag_word == DRAWTAG_FILL_RAD_GRADIENT || | ||
tag_word == DRAWTAG_FILL_IMAGE | ||
{ | ||
let transform = read_transform(config.transform_base, bbox.trans_ix); | ||
matrx = transform.matrx; | ||
translate = transform.translate; | ||
|
@@ -123,8 +125,8 @@ fn main( | |
linewidth *= sqrt(abs(matrx.x * matrx.w - matrx.y * matrx.z)); | ||
} | ||
switch tag_word { | ||
// DRAWTAG_FILL_COLOR, DRAWTAG_FILL_IMAGE | ||
case 0x44u, 0x48u: { | ||
// DRAWTAG_FILL_COLOR | ||
case 0x44u: { | ||
info[di] = bitcast<u32>(linewidth); | ||
} | ||
// DRAWTAG_FILL_LIN_GRADIENT | ||
|
@@ -169,6 +171,21 @@ fn main( | |
info[di + 9u] = bitcast<u32>(ra); | ||
info[di + 10u] = bitcast<u32>(roff); | ||
} | ||
// DRAWTAG_FILL_IMAGE | ||
case 0x248u: { | ||
info[di] = bitcast<u32>(linewidth); | ||
let inv_det = 1.0 / (matrx.x * matrx.w - matrx.y * matrx.z); | ||
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. This is perfectly fine as is but if we were to use the built-in matrix types this could possible use the 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. It would be great to do a pass through the code to make use of appropriate matrix types/ops where appropriate. For this PR, I'd like to avoid the extra churn though. 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. For sure, I didn't mean for that to happen in this CL. I'm happy to do a fixup pass later for matrices seeing as I've brought it up a couple times already and you seem to like the idea :) 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. sgtm! |
||
let inv_mat = inv_det * vec4(matrx.w, -matrx.y, -matrx.z, matrx.x); | ||
let inv_tr = mat2x2(inv_mat.xy, inv_mat.zw) * -translate; | ||
info[di + 1u] = bitcast<u32>(inv_mat.x); | ||
info[di + 2u] = bitcast<u32>(inv_mat.y); | ||
info[di + 3u] = bitcast<u32>(inv_mat.z); | ||
info[di + 4u] = bitcast<u32>(inv_mat.w); | ||
info[di + 5u] = bitcast<u32>(inv_tr.x); | ||
info[di + 6u] = bitcast<u32>(inv_tr.y); | ||
info[di + 7u] = scene[dd]; | ||
info[di + 8u] = scene[dd + 1u]; | ||
} | ||
default: {} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,9 @@ var gradients: texture_2d<f32>; | |
@group(0) @binding(6) | ||
var<storage> info: array<u32>; | ||
|
||
@group(0) @binding(7) | ||
var image_atlas: texture_2d<f32>; | ||
|
||
fn read_fill(cmd_ix: u32) -> CmdFill { | ||
let tile = ptcl[cmd_ix + 1u]; | ||
let backdrop = i32(ptcl[cmd_ix + 2u]); | ||
|
@@ -81,6 +84,24 @@ fn read_rad_grad(cmd_ix: u32) -> CmdRadGrad { | |
return CmdRadGrad(index, matrx, xlat, c1, ra, roff); | ||
} | ||
|
||
fn read_image(cmd_ix: u32) -> CmdImage { | ||
let info_offset = ptcl[cmd_ix + 1u]; | ||
let m0 = bitcast<f32>(info[info_offset]); | ||
let m1 = bitcast<f32>(info[info_offset + 1u]); | ||
let m2 = bitcast<f32>(info[info_offset + 2u]); | ||
let m3 = bitcast<f32>(info[info_offset + 3u]); | ||
let matrx = vec4(m0, m1, m2, m3); | ||
let xlat = vec2(bitcast<f32>(info[info_offset + 4u]), bitcast<f32>(info[info_offset + 5u])); | ||
let xy = info[info_offset + 6u]; | ||
let width_height = info[info_offset + 7u]; | ||
// The following are not intended to be bitcasts | ||
let x = f32(xy >> 16u); | ||
let y = f32(xy & 0xffffu); | ||
let width = f32(width_height >> 16u); | ||
let height = f32(width_height & 0xffffu); | ||
return CmdImage(matrx, xlat, vec2(x, y), vec2(width, height)); | ||
} | ||
|
||
fn read_end_clip(cmd_ix: u32) -> CmdEndClip { | ||
let blend = ptcl[cmd_ix + 1u]; | ||
let alpha = bitcast<f32>(ptcl[cmd_ix + 2u]); | ||
|
@@ -265,6 +286,28 @@ fn main( | |
} | ||
cmd_ix += 3u; | ||
} | ||
// CMD_IMAGE | ||
case 8u: { | ||
let image = read_image(cmd_ix); | ||
let atlas_extents = image.atlas_offset + image.extents; | ||
for (var i = 0u; i < PIXELS_PER_THREAD; i += 1u) { | ||
let my_xy = vec2(xy.x + f32(i), xy.y); | ||
let atlas_uv = image.matrx.xy * my_xy.x + image.matrx.zw * my_xy.y + image.xlat + image.atlas_offset; | ||
// This currently clips to the image bounds. TODO: extend modes | ||
if all(atlas_uv < atlas_extents) && area[i] != 0.0 { | ||
let uv_quad = vec4(max(floor(atlas_uv), image.atlas_offset), min(ceil(atlas_uv), atlas_extents)); | ||
let uv_frac = fract(atlas_uv); | ||
let a = premul_alpha(textureLoad(image_atlas, vec2<i32>(uv_quad.xy), 0)); | ||
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. I'm curious why the by-hand bilinear interpolation here rather than using hardware. Going over the WGSL intrinsics, I think That could perhaps be followup work to this patch. 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. I hadn’t intended to do filtering at all in this PR so it was sort of tacked on as an afterthought. I’ll work on hardware sampling as a followup. |
||
let b = premul_alpha(textureLoad(image_atlas, vec2<i32>(uv_quad.xw), 0)); | ||
let c = premul_alpha(textureLoad(image_atlas, vec2<i32>(uv_quad.zy), 0)); | ||
let d = premul_alpha(textureLoad(image_atlas, vec2<i32>(uv_quad.zw), 0)); | ||
let fg_rgba = mix(mix(a, b, uv_frac.y), mix(c, d, uv_frac.y), uv_frac.x); | ||
let fg_i = fg_rgba * area[i]; | ||
rgba[i] = rgba[i] * (1.0 - fg_i.a) + fg_i; | ||
} | ||
} | ||
cmd_ix += 2u; | ||
} | ||
// CMD_BEGIN_CLIP | ||
case 9u: { | ||
if clip_depth < BLEND_STACK_SPLIT { | ||
|
@@ -326,3 +369,7 @@ fn main( | |
} | ||
#endif | ||
} | ||
|
||
fn premul_alpha(rgba: vec4<f32>) -> vec4<f32> { | ||
return vec4(rgba.rgb * rgba.a, rgba.a); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ impl DrawTag { | |
pub const RADIAL_GRADIENT: Self = Self(0x2dc); | ||
|
||
/// Image fill. | ||
pub const IMAGE: Self = Self(0x48); | ||
pub const IMAGE: Self = Self(0x248); | ||
|
||
/// Begin layer/clip. | ||
pub const BEGIN_CLIP: Self = Self(0x9); | ||
|
@@ -104,10 +104,10 @@ pub struct DrawRadialGradient { | |
#[derive(Clone, Copy, Debug, Default, Zeroable, Pod)] | ||
#[repr(C)] | ||
pub struct DrawImage { | ||
/// Image index. | ||
pub index: u32, | ||
/// Packed image offset. | ||
pub offset: u32, | ||
/// Packed atlas coordinates. | ||
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. Would [u16; 2] be a better choice here? It feels more semantic, but perhaps just using u32 leaves less slop around stuff like endianness. 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. Yeah, it was simply a desire to keep the shape of the data the same for both CPU and GPU. Also, when we do add descriptor indexing, the xy field (maybe renamed location) can be used as the array index in that mode. |
||
pub xy: u32, | ||
/// Packed image dimensions. | ||
pub width_height: u32, | ||
} | ||
|
||
/// Draw data for a clip or layer. | ||
|
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.
Not so sure about using the Piet logo here. Ideally, we'd have a Vello logo, or a Linebender logo.
I don't have a better suggestion for what we can use.
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.
Linebender or vello logos would be great! This choice allowed me to sidestep licensing concerns and, as an additional benefit, has transparency which exposed a filtering bug.
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.
What's the licensing situation for the druid logo from the screenshot you posted on xulip chat? I'm guessing Druid is a bit more relevant than Piet.
I'm supportive of a weekend art project for a vello logo :)
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 assume we're free to use the druid logo as well but I thought piet might be slightly more closely related to vello given the lineage. For now, the priority is just to have some example that displays an image.
I can provide colorful... commentary on a logo project :) I don't think my artistic talents are up to the task!