mirror of
https://github.com/storytold/photocraft.git
synced 2026-11-10 12:22:22 +00:00
Fix-up for #9182: build jobs carry their vertex entry point, and the test rules out a shared vs
The build now takes each job's vertex entry name from build_jobs, the vertex module is generated from the same jobs, and build_jobs_share_no_shader also checks that compose.wgsl has no vertex entry point. Before, the test checked the job plan on its own and still passed if the build went back to one shared vs (verified: re-adding vs to compose.wgsl now fails the test). Co-Authored-By: FinnNk <FinnNk@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
f805540d0a
commit
8701903f9b
3 files changed
+34
-29
No files matched your search
+16
-14
@@ -127,41 +127,43 @@ pub(crate) fn uniform_entry(binding: u32, size: u64) -> wgpu::BindGroupLayoutEnt
|
||||
/// The compositor's WGSL source (exposed for validation in tests).
|
||||
pub const SHADER: &str = include_str!("compose.wgsl");
|
||||
|
||||
/// One thread's share of the compositor's pipelines: the vertex entry point (in
|
||||
/// [`vertex_shader`]) and the (kernel, target format) pairs it builds.
|
||||
pub(crate) struct BuildJob {
|
||||
pub(crate) vertex: String,
|
||||
pub(crate) keys: Vec<(Kernel, wgpu::TextureFormat)>,
|
||||
}
|
||||
|
||||
/// The compositor's pipelines, grouped by the thread that builds them: one job per kernel, and
|
||||
/// an effect-map kernel's R32F and R16F pipelines in the same job. With [`vertex_entry`], no two
|
||||
/// jobs share a shader (see `Compositor::build`).
|
||||
pub(crate) fn build_jobs(acc_format: wgpu::TextureFormat) -> Vec<Vec<(Kernel, wgpu::TextureFormat)>> {
|
||||
/// an effect-map kernel's R32F and R16F pipelines in the same job. Each job has its own vertex
|
||||
/// entry point, so no two jobs share a shader (see `Compositor::build`).
|
||||
pub(crate) fn build_jobs(acc_format: wgpu::TextureFormat) -> Vec<BuildJob> {
|
||||
let one = |k: Kernel| if k.is_map() { vec![(k, MAP32), (k, MAP16)] } else { vec![(k, acc_format)] };
|
||||
let (plain, maps): (Vec<Kernel>, Vec<Kernel>) = Kernel::DRAWN.iter().partition(|k| !k.is_map());
|
||||
plain.into_iter().chain(maps).map(one).collect()
|
||||
plain.into_iter().chain(maps).enumerate().map(|(job, k)| BuildJob { vertex: format!("vs_{job}"), keys: one(k) }).collect()
|
||||
}
|
||||
|
||||
/// The vertex entry point of build job `job` in [`vertex_shader`].
|
||||
pub(crate) fn vertex_entry(job: usize) -> String {
|
||||
format!("vs_{job}")
|
||||
}
|
||||
|
||||
/// The full-chunk triangle's vertex stage, once per build job (`vs_0`, `vs_1`, ...): the same
|
||||
/// code under distinct names, so each job compiles its own vertex shader.
|
||||
/// The full-chunk triangle's vertex stage, once per build job under that job's name (`vs_0`,
|
||||
/// `vs_1`, ...): the same code under distinct names, so each job compiles its own vertex shader.
|
||||
///
|
||||
/// A workaround, not a design: wgpu-hal's D3D12 shader cache shares a compiled FXC blob between
|
||||
/// threads, and FXC's blobs don't count references atomically, so a shared one can be freed
|
||||
/// while still cached (gfx-rs/wgpu#10547, #9173). Each copy costs one compile of a three-line
|
||||
/// shader. Once a wgpu release fixes the cache, go back to one `vs` in `compose.wgsl` and let
|
||||
/// `build_jobs` split map kernels by format again.
|
||||
pub(crate) fn vertex_shader(jobs: usize) -> String {
|
||||
pub(crate) fn vertex_shader(jobs: &[BuildJob]) -> String {
|
||||
let mut s = String::from(
|
||||
"struct VOut { @builtin(position) pos: vec4<f32> };
|
||||
",
|
||||
);
|
||||
for job in 0..jobs {
|
||||
for job in jobs {
|
||||
s.push_str(&format!(
|
||||
"@vertex fn {}(@builtin(vertex_index) vi: u32) -> VOut {{
|
||||
let uv = vec2(f32((vi << 1u) & 2u), f32(vi & 2u));
|
||||
return VOut(vec4(uv.x * 2.0 - 1.0, 1.0 - uv.y * 2.0, 0.0, 1.0));
|
||||
}}
|
||||
",
|
||||
vertex_entry(job)
|
||||
job.vertex
|
||||
));
|
||||
}
|
||||
s
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
use super::batch::tile_grid;
|
||||
use super::consts::{ACC_FORMAT, CHUNK, CHUNK_UNIFORM, FX_BUDGET, MAX_PATTERN_TEXTURE_BYTES, OP_UNIFORM, PAGE, PATTERN_BUDGET, RESIDENT_BUDGET};
|
||||
use super::kit::{Kit, SHADER, build_jobs, tex_entry, uniform_entry, vertex_entry, vertex_shader};
|
||||
use super::kit::{Kit, SHADER, build_jobs, tex_entry, uniform_entry, vertex_shader};
|
||||
use super::paging::{cell_rect, field_reaches, first_error, grid_rects};
|
||||
use super::textures::{FxEntry, Resident, Tex, TexKind};
|
||||
use super::{
|
||||
@@ -61,7 +61,7 @@ impl Compositor {
|
||||
let jobs = build_jobs(acc_format);
|
||||
let vertex = device.create_shader_module(wgpu::ShaderModuleDescriptor {
|
||||
label: Some("pc_compose_vertex"),
|
||||
source: wgpu::ShaderSource::Wgsl(vertex_shader(jobs.len()).into()),
|
||||
source: wgpu::ShaderSource::Wgsl(vertex_shader(&jobs).into()),
|
||||
});
|
||||
let bgl0 = device.create_bind_group_layout(&wgpu::BindGroupLayoutDescriptor {
|
||||
label: Some("pc_compose_uniforms"),
|
||||
@@ -98,11 +98,11 @@ impl Compositor {
|
||||
};
|
||||
// Effect maps render to R32Float / R16Float, which some adapters (e.g. GL without float
|
||||
// render targets) can't: then layer effects use the CPU compositor and the rest stays here.
|
||||
let built = fx::par_map(jobs.into_iter().enumerate().collect(), |(job, keys)| {
|
||||
let vs = vertex_entry(job);
|
||||
keys.into_iter()
|
||||
let built = fx::par_map(jobs, |job| {
|
||||
job.keys
|
||||
.into_iter()
|
||||
.map(|key| {
|
||||
let (built, error) = first_error(device, || pipeline(key.0, key.1, &vs));
|
||||
let (built, error) = first_error(device, || pipeline(key.0, key.1, &job.vertex));
|
||||
(key, built, error)
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
|
||||
+12
-9
@@ -136,18 +136,18 @@ fn half_roundtrip() {
|
||||
#[test]
|
||||
fn build_jobs_share_no_shader() {
|
||||
use super::consts::{MAP16, MAP32};
|
||||
use super::kit::{build_jobs, vertex_entry, vertex_shader};
|
||||
use super::kit::{build_jobs, vertex_shader};
|
||||
let jobs = build_jobs(wgpu::TextureFormat::Rgba32Float);
|
||||
let mut seen = std::collections::HashSet::new();
|
||||
for (job, keys) in jobs.iter().enumerate() {
|
||||
let fragments: std::collections::HashSet<_> = keys.iter().filter_map(|k| k.0.entry()).collect();
|
||||
for job in &jobs {
|
||||
let fragments: std::collections::HashSet<_> = job.keys.iter().filter_map(|k| k.0.entry()).collect();
|
||||
for f in fragments {
|
||||
assert!(seen.insert(f.to_string()), "fragment shader {f} is built by two jobs");
|
||||
}
|
||||
assert!(seen.insert(vertex_entry(job)), "vertex entry of job {job} is shared");
|
||||
assert!(seen.insert(job.vertex.clone()), "vertex entry {} is shared", job.vertex);
|
||||
}
|
||||
// Every drawn pipeline is still built, in the same formats.
|
||||
let keys: Vec<_> = jobs.concat();
|
||||
let keys: Vec<_> = jobs.iter().flat_map(|job| job.keys.iter().copied()).collect();
|
||||
let plain = Kernel::DRAWN.iter().filter(|k| !k.is_map()).count();
|
||||
assert_eq!(keys.len(), plain + 2 * (Kernel::DRAWN.len() - plain));
|
||||
for k in Kernel::DRAWN {
|
||||
@@ -156,15 +156,18 @@ fn build_jobs_share_no_shader() {
|
||||
assert_eq!(formats, want, "{k:?}");
|
||||
}
|
||||
// The vertex module validates and has every job's entry point.
|
||||
let src = vertex_shader(jobs.len());
|
||||
let src = vertex_shader(&jobs);
|
||||
let module = wgpu::naga::front::wgsl::parse_str(&src).unwrap_or_else(|e| panic!("{}", e.emit_to_string(&src)));
|
||||
wgpu::naga::valid::Validator::new(wgpu::naga::valid::ValidationFlags::all(), wgpu::naga::valid::Capabilities::empty())
|
||||
.validate(&module)
|
||||
.unwrap_or_else(|e| panic!("{e:?}"));
|
||||
for job in 0..jobs.len() {
|
||||
let e = vertex_entry(job);
|
||||
assert!(module.entry_points.iter().any(|ep| ep.name == e && ep.stage == wgpu::naga::ShaderStage::Vertex), "missing {e}");
|
||||
for job in &jobs {
|
||||
assert!(module.entry_points.iter().any(|ep| ep.name == job.vertex && ep.stage == wgpu::naga::ShaderStage::Vertex), "missing {}", job.vertex);
|
||||
}
|
||||
// The fragment module has no vertex stage, so the build can't fall back to one shared `vs`.
|
||||
let compose = wgpu::naga::front::wgsl::parse_str(SHADER).unwrap_or_else(|e| panic!("{}", e.emit_to_string(SHADER)));
|
||||
let shared: Vec<_> = compose.entry_points.iter().filter(|ep| ep.stage == wgpu::naga::ShaderStage::Vertex).map(|ep| ep.name.as_str()).collect();
|
||||
assert!(shared.is_empty(), "compose.wgsl has a vertex entry point every job would share: {shared:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in new issue
Block a user