Skip to content
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

Merged
merged 6 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ bytemuck = { version = "1.12.1", features = ["derive"] }
smallvec = "1.8.0"
moscato = { git = "https://github.com/dfrg/pinot", rev = "59db153" }
peniko = { git = "https://github.com/linebender/peniko", rev = "cafdac9a211a0fb2fec5656bd663d1ac770bcc81" }
guillotiere = "0.6.2"

[workspace.dependencies]
wgpu = "0.15"
Expand Down
Binary file added examples/assets/piet-logo.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 3 additions & 1 deletion examples/headless/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::{

use anyhow::{anyhow, bail, Context, Result};
use clap::{CommandFactory, Parser};
use scenes::{SceneParams, SceneSet, SimpleText};
use scenes::{ImageCache, SceneParams, SceneSet, SimpleText};
use vello::{
block_on_wgpu,
kurbo::{Affine, Vec2},
Expand Down Expand Up @@ -97,9 +97,11 @@ async fn render(mut scenes: SceneSet, index: usize, args: &Args) -> Result<()> {
let mut builder = SceneBuilder::for_fragment(&mut fragment);
let example_scene = &mut scenes.scenes[index];
let mut text = SimpleText::new();
let mut images = ImageCache::new();
let mut scene_params = SceneParams {
time: args.time.unwrap_or(0.),
text: &mut text,
images: &mut images,
resolution: None,
base_color: None,
};
Expand Down
1 change: 1 addition & 0 deletions examples/scenes/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ vello = { path = "../../" }
vello_svg = { path = "../../integrations/vello_svg" }
anyhow = { workspace = true }
clap = { workspace = true, features = ["derive"] }
image = "0.24.5"

# Used for the `download` command
byte-unit = "4.0"
Expand Down
51 changes: 51 additions & 0 deletions examples/scenes/src/images.rs
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))
}
3 changes: 3 additions & 0 deletions examples/scenes/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pub mod download;
mod images;
mod simple_text;
mod svg;
mod test_scenes;
Expand All @@ -7,6 +8,7 @@ use std::path::PathBuf;
use anyhow::{anyhow, Result};
use clap::{Args, Subcommand};
use download::Download;
pub use images::ImageCache;
pub use simple_text::SimpleText;
pub use svg::{default_scene, scene_from_files};
pub use test_scenes::test_scenes;
Expand All @@ -16,6 +18,7 @@ use vello::{kurbo::Vec2, peniko::Color, SceneBuilder};
pub struct SceneParams<'a> {
pub time: f64,
pub text: &'a mut SimpleText,
pub images: &'a mut ImageCache,
pub resolution: Option<Vec2>,
pub base_color: Option<vello::peniko::Color>,
}
Expand Down
13 changes: 13 additions & 0 deletions examples/scenes/src/test_scenes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

@armansito armansito Mar 10, 2023

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 :)

Copy link
Collaborator Author

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!


macro_rules! scene {
($name: ident) => {
scene!($name: false)
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

From a strict provenance perspective, this should use pointer::addr (whereas as usize exposes the address, AIUI). That however is unstable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, to be clear, there's no soundness issue here.
This is just an observation that the semantics of pointer to integer casts are messy. The way I think this works is that the use of as usize "tells" the compiler that you might do a corresponding as *const _, even though there isn't. .addr doesn't have that "side effect", but is unstable.

This is not at all important, I just recognised this as an opportunity to brush up my understanding of strict aliasing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 = [
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 3 additions & 1 deletion examples/with_winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use std::time::Instant;

use anyhow::Result;
use clap::{CommandFactory, Parser};
use scenes::{SceneParams, SceneSet, SimpleText};
use scenes::{ImageCache, SceneParams, SceneSet, SimpleText};
use vello::peniko::Color;
use vello::util::RenderSurface;
use vello::{
Expand Down Expand Up @@ -96,6 +96,7 @@ fn run(
let mut scene = Scene::new();
let mut fragment = SceneFragment::new();
let mut simple_text = SimpleText::new();
let mut images = ImageCache::new();
let start = Instant::now();

let mut touch_state = multi_touch::TouchState::new();
Expand Down Expand Up @@ -264,6 +265,7 @@ fn run(
let mut scene_params = SceneParams {
time: start.elapsed().as_secs_f64(),
text: &mut simple_text,
images: &mut images,
resolution: None,
base_color: None,
};
Expand Down
14 changes: 14 additions & 0 deletions shader/coarse.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -377,6 +384,13 @@ fn main(
write_grad(CMD_RAD_GRAD, index, info_offset);
}
}
// DRAWTAG_FILL_IMAGE
case 0x248u: {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 {
Expand Down
23 changes: 20 additions & 3 deletions shader/draw_leaf.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 determinant intrinsic. Maybe worth exploring in the future (I also don't know if there are any potential compatibility issues with using the intrinsics over hand-rolling calculation yourself).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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: {}
}
}
Expand Down
47 changes: 47 additions & 0 deletions shader/fine.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The 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 textureSampleLevel is the most appropriate version - unlike textureSample it doesn't rely on derivatives, and I think it's likely to be more performant than textureSampleGrad (which based on past piet-gpu experimentation was slow).

That could perhaps be followup work to this patch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 {
Expand Down Expand Up @@ -326,3 +369,7 @@ fn main(
}
#endif
}

fn premul_alpha(rgba: vec4<f32>) -> vec4<f32> {
return vec4(rgba.rgb * rgba.a, rgba.a);
}
2 changes: 1 addition & 1 deletion shader/shared/drawtag.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ let DRAWTAG_NOP = 0u;
let DRAWTAG_FILL_COLOR = 0x44u;
let DRAWTAG_FILL_LIN_GRADIENT = 0x114u;
let DRAWTAG_FILL_RAD_GRADIENT = 0x2dcu;
let DRAWTAG_FILL_IMAGE = 0x48u;
let DRAWTAG_FILL_IMAGE = 0x248u;
let DRAWTAG_BEGIN_CLIP = 0x9u;
let DRAWTAG_END_CLIP = 0x21u;

Expand Down
8 changes: 8 additions & 0 deletions shader/shared/ptcl.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ let CMD_SOLID = 3u;
let CMD_COLOR = 5u;
let CMD_LIN_GRAD = 6u;
let CMD_RAD_GRAD = 7u;
let CMD_IMAGE = 8u;
let CMD_BEGIN_CLIP = 9u;
let CMD_END_CLIP = 10u;
let CMD_JUMP = 11u;
Expand Down Expand Up @@ -57,6 +58,13 @@ struct CmdRadGrad {
roff: f32,
}

struct CmdImage {
matrx: vec4<f32>,
xlat: vec2<f32>,
atlas_offset: vec2<f32>,
extents: vec2<f32>,
}

struct CmdEndClip {
blend: u32,
alpha: f32,
Expand Down
1 change: 1 addition & 0 deletions src/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ mod draw;
mod encoding;
mod glyph;
mod glyph_cache;
mod image_cache;
mod math;
mod monoid;
mod path;
Expand Down
10 changes: 5 additions & 5 deletions src/encoding/draw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.
Expand Down
Loading