Refactor vello::util

[nicoburns]

• A lot of general cleanup, adding comments, etc.
• Split out a find_existing_device method
• Store a copy of DeviceHandle inside RenderSurface
• Move methods from RenderContext to RenderSurface
• Allow users to customise Features and Limits (possibly these should be less global)

(the latter was the initial motivation for me interacting with this code - I need to be able to configure these for the override_image demo (both vello and blitz))

Some questions:

• Should RenderContext be global? (actually static? suggested to be Arc>?)
• Should RenderContext also hold VelloRenderers? Why is with_winit reusing VelloRenderer instances?
• Should util / RenderContext provide utility methods to help with pipeline caching?

[DJMcNab]

A few thoughts, without having specifically carefully reviewed the code:

1. vello::util should never have been our job to expose, and should be removed entirely (maybe inlined as appropriate into with_winit, and the other examples should just manage this themselves¹). If you want to make a crate for something like this, the best place for that would probably be in the wgpu repository itself. I think we could land this first, if you wanted. However, I don't want to make another release with breaking changes here before removing it, so we need a plan for someone to do the requisite work. That might involve just slapping a docs(hidden) on the whole module; I wouldn't like that solution, but it does address our needs here.
2. vello::util's is something which should be pretty independent of the rest of Vello - it's pretty much a generic wgpu utility. The addition of the blitter was explicitly as an intermediate state.
3. We have another copy of this code in sparse_strips, which probably should be updated similarly.
4. The updates generally look like a reasonable improvement to the code quality here
5. This needs a changelog entry.

It's clear to me that Vello owning all of this stuff is not fit for purpose, and will require more and more special cased APIs.

Footnotes

1. This is a question about what the role of the "simple" example is; I think it's reasonable for that example to not care about multiple windows. See the discussion in https://github.com/linebender/vello/pull/227, where it seems like this multi-device support was never actually necessary, at least for single Surface cases. For headless and the tests, there is no surface, so it definitely doesn't need all this complexity. ↩

[DJMcNab]

We probably should pass these in using the new mechanism, now

[nicoburns]

@DJMcNab It seems that this infrastructure is mostly for caching/reusing resources? Otherwise we could just recreate everything on every "resume"? Do you have a read on which operations are expensive?

• Creating a wgpu::Instance (wgpu docs say this doesn't necessarily need to be persisted)
• Creating a wgpu::Adapter (wgpu docs say this doesn't necessarily need to be persisted)
• Creating a wgpu::Device (should be relatively cheap?)
• Creating a vello::Renderer (I'm guessing is where the cost is and why we're caching devices?)
• How does pipeline caching interact with all of this?

---

maybe inlined as appropriate into with_winit, and the other examples should just manage this themselves1

I would really like to make with_winit simpler if at all possible. It feels very spaghetti and under-abstracted to me. Although this could be an abstraction that lives within the example if need be.

---

vello::util's is something which should be pretty independent of the rest of Vello - it's pretty much a generic wgpu utility.

I think Vello also has quite specific requirements around things like features, limits, texture format? I guess we might be able to pass those in as parameters.

---

We have another copy of this code in sparse_strips, which probably should be updated similarly.

It would be nice to unify these into one if possible. Although I guess the intermediate texture isn't great in that regard.

[DJMcNab]

It seems that this infrastructure is mostly for caching/reusing resources? Otherwise we could just recreate everything on every "resume"? Do you have a read on which operations are expensive?

I don't think that's the case? It just some utilities around creating wgpu utilities, because there's a lot of boilerplate there. In particular, this is boilerplate around supporting multiple devices, which is only "necessary" if you're going to have multiple surfaces (an unoptimal surface will keep chugging along with bad performance, which is fine - we don't even both recreating a more optimal surface)

I don't see how any part of my suggestion suggests to not maintain these resources, just not have them be managed by code in Vello - that's the wrong abstraction level.

I would really like to make with_winit simpler if at all possible. It feels very spaghetti and under-abstracted to me. Although this could be an abstraction that lives within the example if need be.

Yeah, this would live inside of the with_winit example in that case.

I think Vello also has quite specific requirements around things like features, limits, texture format? I guess we might be able to pass those in as parameters.

Ish? We basically just require baseline wgpu, but can make use of the clear texture ability. We really don't want to be in charge of creating your device.

It would be nice to unify these into one if possible. Although I guess the intermediate texture isn't great in that regard.

Writing this out yourself in each app is basically the idiomatic way to write wgpu code. My suggestion for where a utility for this should live is the wgpu repository - the maintainers there are better placed to determine how to write this type of code idiomatically. See for example the examples for vello_hybrid:

vello/sparse_strips/vello_hybrid/examples/wgpu_webgl/src/lib.rs

Lines 38 to 71 in 36c6659

	let instance = wgpu::Instance::new(&wgpu::InstanceDescriptor {
	backends: wgpu::Backends::GL,
	..Default::default()
	});
	let surface = instance
	.create_surface(wgpu::SurfaceTarget::Canvas(canvas.clone()))
	.expect("Canvas surface to be valid");
	let adapter = instance
	.request_adapter(&wgpu::RequestAdapterOptions {
	compatible_surface: Some(&surface),
	..Default::default()
	})
	.await
	.expect("Adapter to be valid");
	let (device, queue) = adapter
	.request_device(
	&wgpu::DeviceDescriptor {
	label: None,
	required_features: wgpu::Features::empty(),
	required_limits: wgpu::Limits {
	// WGPU's downlevel defaults use a generous number of color attachments
	// (8). Some devices (including CI) support only up to 4.
	max_color_attachments: 4,
	max_texture_dimension_2d: adapter.limits().max_texture_dimension_2d,
	..wgpu::Limits::downlevel_webgl2_defaults()
	},
	..Default::default()
	},
	None,
	)
	.await
	.expect("Device to be valid");

[DJMcNab]

See also in #879, where we recreate the device etc in resume if and only if the old device was not compatible:
https://github.com/linebender/vello/pull/879/files#diff-566ea9ba31e45a5ffe1a87c559ccd0e12eb967c5160d4ce65ef0651e3384999cR303-R314

[nicoburns]

I don't think that's the case? It just some utilities around creating wgpu utilities, because there's a lot of boilerplate there. In particular, this is boilerplate around supporting multiple devices, which is only "necessary" if you're going to have multiple surfaces

If it's just boilerplate, then I don't understand why we persist the devices and the instance (i.e. why the code is stateful). It seems like you could just have a function that goes through the instance -> adapter -> device -> surface dance from scratch every time which would be much simpler (never bother with find_existing_device just always create_new_device).

My assumption from the structure of the code was that there was some reason why devices must be reused, and I assumed performance (I'm still confused).

[DJMcNab]

I really don't follow what you're asking; your suggests seem completely unrelated to what I'm suggesting.

But as I say, if you want to just move on from this, we can land this PR, with adding doc(hidden) to the util module (so it's clear that this isn't to be treated as a public API) and have removing this utility as follow-up.