From ca833b6d2b3cf92ed38124c205817aa1a2838fc6 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Tue, 18 Aug 2026 15:23:04 +0200 Subject: [PATCH] Give every colour band its own compiled shader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The colour mixer emits a code block and a uniform only for the bands that are set, so which bands are adjusted is part of the shader's structure. The pipeline cache key was not: it hashed the set of *active operations*, which is "colour_mixer" whichever band that is. So a red adjustment and a blue one hashed alike. The second render was handed the first's compiled pipeline while its uniform was uploaded into a slot that shader had assigned to another band — whichever band compiled first kept acting on every subsequent move, and every other slider did nothing at all. Red is the first band declared, and the one reported as the only one working. The hash is now taken over the generated WGSL, because the source is what gets compiled and therefore is the structure. A summary of what went into it has to be kept in step with every operation's code generation by hand, and this one had fallen out of step. Values still do not enter it: no operation writes a parameter value into its source, so a slider drag regenerates identical text and reuses the pipeline, and one that did inline a value would have to recompile to be correct anyway. `each_colour_band_gets_its_own_pipeline` in dr-gpu renders a blue pixel through one pass with red set first and then blue, and fails on the old hash with the reported symptom — the blue slider returning the pixel unchanged to the byte. Co-Authored-By: Claude Opus 5 --- core/dr-pipeline/src/operation.rs | 56 +++++++++++++++-------- core/dr-pipeline/src/ops/colour_mixer.rs | 58 ++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 20 deletions(-) diff --git a/core/dr-pipeline/src/operation.rs b/core/dr-pipeline/src/operation.rs index 2684689..15dc2f4 100644 --- a/core/dr-pipeline/src/operation.rs +++ b/core/dr-pipeline/src/operation.rs @@ -397,16 +397,32 @@ fn main(@builtin(global_invocation_id) gid: vec3) {{ active.len() ); - // Framing enters the hash by structure only — which branches its prologue - // generated, never how far a slider moved. Dragging the crop handles must - // reuse the compiled pipeline and re-upload uniforms. + // Taken over the generated source, because the source *is* the structure: + // it is what gets compiled, and two compositions that produce different + // WGSL are two pipelines however alike their op-sets look. // - // The output space enters it too, and must: it changes the source, so two - // spaces sharing a hash would have the second silently rendered with the - // first's shader — a Display P3 export that came out sRGB and said - // otherwise. + // Hashing the list of active operation ids instead — which is what this + // did — assumed every operation emits the same code whatever its + // parameters say. The colour mixer does not: it emits a block and a + // uniform only for the bands that are set, so a red adjustment and a blue + // one are the same op-set and different shaders. They shared a cache + // entry, so the second was rendered with the first's compiled pipeline + // while its uniforms were uploaded in an order that pipeline never agreed + // to — whichever band was adjusted first kept acting, and every other + // band appeared dead. + // + // Values still do not enter it, since no operation writes a parameter + // value into its source; they arrive as uniforms, and dragging a slider + // regenerates identical text. One that did inline a value would have to + // recompile to be correct, and hashing the source says so rather than + // silently reusing the wrong pipeline. + // + // Framing and the output space are mixed in as well, though both already + // shape the source: the prologue's branches and the encode function are + // written into it. Belt and braces on the two inputs whose contribution to + // the source is indirect. let structure_hash = mix( - mix(hash_structure(&active), framing.structure_key()), + mix(hash_source(&source), framing.structure_key()), output as u64, ); @@ -575,22 +591,22 @@ fn mix(mut h: u64, value: u64) -> u64 { h } -/// Hash the op-set and order — the structure, not the values. +/// Hash the generated WGSL — the structure of the shader, not the values. /// -/// Two edits with the same operations at different slider positions produce -/// the same hash and reuse one compiled pipeline (ARCH §6.13: the hash is -/// over integer state only, so it is exactly deterministic). -fn hash_structure(active: &[&dyn Operation]) -> u64 { +/// Whole-source rather than a summary of what went into it: a summary has to +/// be kept in step with every operation's code generation by hand, and the +/// one that was here fell out of step with the colour mixer, which emits +/// different code for different bands. +/// +/// Still integer state hashed on the CPU, as ARCH §6.13 requires of a cache +/// key: the text is generated from parameters that are neutral or not, never +/// from a rendered float. +fn hash_source(source: &str) -> u64 { // FNV-1a: no dependency, stable across runs and platforms, which the // shader cache key requires. let mut h: u64 = 0xcbf2_9ce4_8422_2325; - for op in active { - for byte in op.descriptor().id.0.as_bytes() { - h ^= u64::from(*byte); - h = h.wrapping_mul(0x100_0000_01b3); - } - // A separator, so ["ab", "c"] and ["a", "bc"] differ. - h ^= 0xff; + for byte in source.as_bytes() { + h ^= u64::from(*byte); h = h.wrapping_mul(0x100_0000_01b3); } h diff --git a/core/dr-pipeline/src/ops/colour_mixer.rs b/core/dr-pipeline/src/ops/colour_mixer.rs index 3113f42..db94339 100644 --- a/core/dr-pipeline/src/ops/colour_mixer.rs +++ b/core/dr-pipeline/src/ops/colour_mixer.rs @@ -687,6 +687,64 @@ mod tests { assert!(body.contains("w_total > 0.0001")); } + #[test] + fn two_bands_are_two_shaders() { + // The bug this closes: the shader cache was keyed on the set of + // active operations, and this operation is active whichever band is + // set. A red adjustment and a blue one therefore shared a compiled + // pipeline — the first one to compile — and the second was rendered + // with the first's code while its uniform was uploaded into the + // first's slot. Every band but the one compiled first appeared to do + // nothing, and moving its slider moved the other band's colour. + // + // Asserted here rather than only in `operation.rs` because this is + // the operation that generates per-value code, and so the one whose + // shaders must not collide. + let compose_with = |id, v| { + let mut m = ColourMixer::new(); + m.set_param(ParamId(id), v); + let ops: Vec> = vec![Box::new(m)]; + crate::operation::compose(&ops) + }; + + let red = compose_with("red_sat", 60.0); + let blue = compose_with("blue_sat", 60.0); + assert_ne!( + red.source, blue.source, + "two bands emit different code, which is the premise" + ); + assert_ne!( + red.structure_hash, blue.structure_hash, + "two bands must not share a compiled pipeline" + ); + + // The same band at a different setting is the same shader, which is + // what keeps a slider drag from recompiling once per frame. + let red_harder = compose_with("red_sat", 90.0); + assert_eq!(red.structure_hash, red_harder.structure_hash); + assert_ne!(red.uniforms, red_harder.uniforms); + } + + #[test] + fn every_band_and_channel_is_its_own_shader() { + // All thirty-six, because the collision above was not special to red + // and blue: any two settings that generate different code and hash + // alike put one of them on the other's pipeline. + let mut seen = std::collections::HashMap::new(); + for band in BANDS.iter() { + for ch in Channel::ALL { + let id = format!("{}_{}", band.key, ch.suffix()); + let mut m = ColourMixer::new(); + m.set_param(ParamId(Box::leak(id.clone().into_boxed_str())), 50.0); + let ops: Vec> = vec![Box::new(m)]; + let hash = crate::operation::compose(&ops).structure_hash; + if let Some(other) = seen.insert(hash, id.clone()) { + panic!("{id} and {other} share a pipeline"); + } + } + } + } + #[test] fn band_weights_sum_to_one_at_every_hue() { // The property the shader relies on to add band deltas unscaled, and