Sync with integration

core/dr-pipeline/ops/README.md

@@ -209,11 +209,12 @@ They still belong in this directory, because the pipeline's **order** is the
 one thing a reader comes here to learn, and an order written half in YAML and
 half in Rust would be worse than either alone.
 
-Currently hand-written: `tone_curve` (a curve widget over five interpolated
-points), `colour_mixer` (thirty-six faceted parameters from twelve computed hue
-bands), `capture_sharpen` (a separable convolution, which is the other reason a
-node is Rust — see the next section). `vignetting` is hand-written too but is
-not in the develop chain — it
+Currently hand-written: `tone_curve` (one widget over four curves of five
+interpolated points — master, red, green, blue — each reaching the shader only
+when it has been moved), `colour_mixer` (thirty-six faceted parameters from
+twelve computed hue bands), `capture_sharpen` (a separable convolution, which
+is the other reason a node is Rust — see the next section). `vignetting` is
+hand-written too but is not in the develop chain — it
 carries lens-profile coefficients that are not parameters. `distortion` and
 `aberration` are `Warp`s rather than operations: they rewrite coordinates
 before sampling rather than transforming a colour after it.

core/dr-pipeline/ops/tone_curve.yaml

@@ -16,13 +16,26 @@ attributes: [tone, colour]
 rust: ToneCurve
 
 why_rust: |
-  Five control points presented as one curve widget, with an interpolator and
-  a monotonicity guarantee behind it. Its neutral is a *relationship* between
-  parameters rather than a set of values — the identity diagonal — which is
-  not something the declarative `active:` rule can express, and its
-  `presentation()` spans parameters rather than describing one.
+  Four curves — master, red, green, blue — of five control points each,
+  presented as one widget, with an interpolator and a monotonicity guarantee
+  behind them. Its neutral is a *relationship* between parameters rather than
+  a set of values — the identity diagonal, on every channel — which is not
+  something the declarative `active:` rule can express; its `presentation()`
+  spans forty parameters rather than describing one; and its fragment is
+  *assembled* rather than written, because each curve reaches the shader only
+  when it has been moved off the diagonal. A declared node's `wgsl:` is one
+  fixed block of text, which is the right shape for nearly everything here and
+  the wrong one for a node whose cost has to follow what the photographer
+  actually touched.
 
 placement: |
   After the fixed-weight region controls, so the curve is the final word on
   tone: a photographer reaches for it to fix what those controls could not
   place exactly.
+
+  Within the node, the master curve runs before the per-channel ones. Both
+  orders are visibly different images and the reasons for this one are written
+  out in `src/ops/curve.rs`: the master is tonal and hue-preserving, the
+  per-channel curves are the chromatic grade over the tones it produced, and a
+  point placed on a channel curve should act on the tone the photographer can
+  see rather than on the one the master is about to move.

core/dr-pipeline/src/sidecar.rs

@@ -1088,7 +1088,7 @@ impl std::error::Error for ParseError {}
 mod tests {
     use super::*;
     use crate::framing;
-    use crate::ops::{exposure, saturation, white_balance};
+    use crate::ops::{curve, exposure, saturation, white_balance};
 
     fn edited() -> EditGraph {
         let mut g = EditGraph::default_chain();
@@ -1273,6 +1273,90 @@ mod tests {
         assert_eq!(once, twice);
     }
 
+    /// TRACES: FR-DEV-3 | FR-CAT-8
+    /// A file written before the tone curve had per-channel curves.
+    ///
+    /// Spelled out as literal text rather than produced by `to_text`, because
+    /// the claim is about *those bytes*: a sidecar generated by this build
+    /// would agree with this build by construction, and would go on agreeing
+    /// with it through a rename that broke every file on disk.
+    #[test]
+    fn a_sidecar_from_before_the_channel_curves_still_names_the_master() {
+        let text = "drsc 1\n\n[version u1]\nname = Default\nrevision = 4\nmodified = 9\n\
+                    tone_curve.p1_y = 0.15\ntone_curve.p3_y = 0.85\n";
+        let parsed = Sidecar::parse(text).expect("valid");
+        let mut g = EditGraph::default_chain();
+        parsed.default_version().expect("a version").apply(&mut g);
+
+        // The S-curve the file describes, on the master curve and nowhere
+        // else.
+        assert_eq!(g.param(curve::ID, curve::P1_Y), Some(0.15));
+        assert_eq!(g.param(curve::ID, curve::P3_Y), Some(0.85));
+        for channel in [curve::Channel::Red, curve::Channel::Green, curve::Channel::Blue] {
+            for point in 0..curve::POINTS {
+                for axis in [curve::Axis::X, curve::Axis::Y] {
+                    let id = curve::coordinate(channel, point, axis);
+                    let expected = g
+                        .capabilities()
+                        .iter()
+                        .find(|c| c.id == curve::ID)
+                        .and_then(|c| c.params.iter().find(|p| p.id == id))
+                        .map(|p| p.default);
+                    assert_eq!(
+                        g.param(curve::ID, id),
+                        expected,
+                        "{id} moved, and no line in the file mentions it"
+                    );
+                }
+            }
+        }
+
+        // And writing it back produces the same two lines: the curves the file
+        // never mentioned are still at their defaults, so they are still
+        // absent (`only_non_default_values_are_written`).
+        let written = Sidecar::parse(&Sidecar::parse(text).expect("valid").to_text())
+            .expect("valid")
+            .to_text();
+        assert!(written.contains("tone_curve.p1_y = 0.15"), "{written}");
+        assert!(written.contains("tone_curve.p3_y = 0.85"), "{written}");
+        assert!(
+            !written.contains("tone_curve.r_"),
+            "an untouched channel curve was written out:\n{written}"
+        );
+    }
+
+    /// TRACES: FR-DEV-3
+    /// The other direction: the new curves persist like any other parameter.
+    #[test]
+    fn a_per_channel_curve_survives_the_round_trip() {
+        let mut g = EditGraph::default_chain();
+        // A faded shadow: blue lifted at the black point, red pulled down.
+        let blue = curve::coordinate(curve::Channel::Blue, 0, curve::Axis::Y);
+        let red = curve::coordinate(curve::Channel::Red, 4, curve::Axis::Y);
+        g.set_param(curve::ID, blue, 0.08);
+        g.set_param(curve::ID, red, 0.92);
+        g.set_param(curve::ID, curve::P2_Y, 0.55);
+
+        let mut sidecar = Sidecar::new();
+        sidecar.put(version_of(&g));
+        let text = sidecar.to_text();
+        // Keyed by the channel-prefixed id, which is what makes the master's
+        // unprefixed ones safe to leave alone.
+        assert!(text.contains("tone_curve.b_p0_y = 0.08"), "{text}");
+
+        let parsed = Sidecar::parse(&text).expect("valid");
+        let mut restored = EditGraph::default_chain();
+        parsed
+            .default_version()
+            .expect("a version")
+            .apply(&mut restored);
+
+        assert_eq!(restored.param(curve::ID, blue), Some(0.08));
+        assert_eq!(restored.param(curve::ID, red), Some(0.92));
+        assert_eq!(restored.param(curve::ID, curve::P2_Y), Some(0.55));
+        assert!(!restored.is_neutral());
+    }
+
     #[test]
     fn an_unknown_operation_survives_a_round_trip() {
         // The data-loss case that matters: a device running an older build

core/dr-pipeline/tests/tone_curve.rs

@@ -0,0 +1,202 @@
+//! TRACES: FR-DEV-3
+//! The tone curve's four curves, seen from outside the crate.
+//!
+//! The unit tests beside the operation assert what one `ToneCurve` does. These
+//! assert the two properties that only show up once it is a node in a chain
+//! with a sidecar under it: that a file written before the per-channel curves
+//! existed still describes the edit it described, and that the three new
+//! curves cost a photograph that does not use them precisely nothing.
+
+use dr_pipeline::ops::curve::{self, Axis, Channel};
+use dr_pipeline::{EditGraph, Sidecar};
+
+/// A version block carrying `params`, in the on-disk spelling.
+fn sidecar_with(params: &[(&str, f32)]) -> String {
+    let mut text = String::from("drsc 1\n\n[version u1]\nname = Default\nrevision = 2\nmodified = 0\n");
+    for (key, value) in params {
+        text.push_str(&format!("{key} = {value}\n"));
+    }
+    text
+}
+
+fn apply(text: &str) -> EditGraph {
+    let sidecar = Sidecar::parse(text).expect("a valid sidecar");
+    let mut graph = EditGraph::default_chain();
+    sidecar
+        .default_version()
+        .expect("a default version")
+        .apply(&mut graph);
+    graph
+}
+
+/// TRACES: FR-CAT-8
+/// **The compatibility guarantee, end to end.**
+///
+/// An edit made before this build existed has to produce the same image now.
+/// Asserted on the generated shader rather than on the parameter values,
+/// because that is what the photograph is actually made of: same source, same
+/// uniforms, same picture.
+#[test]
+fn an_edit_written_before_the_channel_curves_renders_as_it_did() {
+    let from_file = apply(&sidecar_with(&[
+        ("tone_curve.p1_y", 0.15),
+        ("tone_curve.p3_y", 0.85),
+    ]));
+
+    let mut by_hand = EditGraph::default_chain();
+    by_hand.set_param(curve::ID, curve::P1_Y, 0.15);
+    by_hand.set_param(curve::ID, curve::P3_Y, 0.85);
+
+    let restored = from_file.compose();
+    let expected = by_hand.compose();
+    assert_eq!(restored.source, expected.source);
+    assert_eq!(restored.uniforms, expected.uniforms);
+    assert_eq!(restored.structure_hash, expected.structure_hash);
+}
+
+/// **Neutral means absent, and stays absent with four times as much to be
+/// neutral about.**
+///
+/// The curve carries forty parameters now. An image edited with an S-curve and
+/// nothing else must generate the shader it generated when it carried ten:
+/// three untouched curves are not three identity evaluations, they are nothing
+/// at all.
+#[test]
+fn three_untouched_curves_cost_nothing() {
+    let mut graph = EditGraph::default_chain();
+    graph.set_param(curve::ID, curve::P2_Y, 0.62);
+    let shader = graph.compose();
+
+    assert!(
+        shader.source.contains("---- tone_curve ----"),
+        "the master curve must reach the shader"
+    );
+    assert!(
+        !shader.source.contains("channel_curve"),
+        "an untouched channel curve reached the shader:\n{}",
+        shader.source
+    );
+    for prefix in ["r_", "g_", "b_"] {
+        assert!(
+            !shader.source.contains(&format!("tone_curve_{prefix}")),
+            "an untouched channel curve declared uniforms:\n{}",
+            shader.source
+        );
+    }
+}
+
+/// And with none of them touched, the operation is not there at all — the
+/// property `a_fresh_graph_is_neutral` asserts for the chain, restated for the
+/// node that just quadrupled in size.
+#[test]
+fn a_curve_at_its_defaults_is_absent_from_the_shader() {
+    let graph = EditGraph::default_chain();
+    assert!(graph.is_neutral());
+    assert!(!graph.compose().source.contains("tone_curve"));
+
+    // Including when every one of the forty parameters has been explicitly
+    // written to its own default, which is what a sidecar round trip through
+    // a build with a different idea of "default" would produce.
+    let mut written = EditGraph::default_chain();
+    for cap in written.capabilities() {
+        if cap.id != curve::ID {
+            continue;
+        }
+        for p in &cap.params {
+            written.set_param(curve::ID, p.id, p.default);
+        }
+    }
+    assert!(written.is_neutral());
+}
+
+/// A grade with no tonal work is a real edit, and it must not drag the
+/// luminance path in behind it.
+#[test]
+fn a_channel_curve_reaches_the_shader_on_its_own() {
+    let mut graph = EditGraph::default_chain();
+    graph.set_param(
+        curve::ID,
+        curve::coordinate(Channel::Blue, 0, Axis::Y),
+        0.08,
+    );
+
+    let shader = graph.compose();
+    assert!(shader.source.contains("c.b = channel_curve(c.b,"));
+    assert!(
+        !shader.source.contains("apply_tone_gain"),
+        "the identity master curve reached the shader:\n{}",
+        shader.source
+    );
+    // Blue's ten points, and nothing else: the other two curves are at the
+    // identity and contribute no slot to the uniform block.
+    for prefix in ["r_", "g_"] {
+        assert!(
+            !shader.source.contains(&format!("tone_curve_{prefix}")),
+            "an untouched channel curve declared uniforms:\n{}",
+            shader.source
+        );
+    }
+    for point in 0..curve::POINTS {
+        assert!(
+            shader.source.contains(&format!("tone_curve_b_x{point}")),
+            "blue's point {point} is missing from the uniform block"
+        );
+    }
+}
+
+/// Each curve is its own shader, so the pipeline cache cannot hand the red
+/// curve's compiled program to an edit that moved the green one.
+#[test]
+fn every_curve_generates_a_distinct_shader() {
+    let mut hashes: Vec<u64> = Vec::new();
+    for channel in Channel::ALL {
+        let mut graph = EditGraph::default_chain();
+        graph.set_param(curve::ID, curve::coordinate(channel, 2, Axis::Y), 0.62);
+        hashes.push(graph.compose().structure_hash);
+    }
+    let before = hashes.len();
+    hashes.sort_unstable();
+    hashes.dedup();
+    assert_eq!(before, hashes.len(), "two curves share a compiled shader");
+}
+
+/// The forty parameters all persist, and the file names each one once.
+#[test]
+fn every_point_of_every_curve_round_trips_through_a_sidecar() {
+    let mut graph = EditGraph::default_chain();
+    // A different y per coordinate, so a point wired to the wrong channel
+    // cannot pass by holding the value it was supposed to hold anyway.
+    // Thirty-secondths because they survive both the decimal the file is
+    // written in and the binary32 it is read back into exactly — this test is
+    // about which parameter a value lands in, and a rounding difference here
+    // would fail it for an unrelated reason.
+    let mut step = 1;
+    for channel in Channel::ALL {
+        for point in 0..curve::POINTS {
+            graph.set_param(
+                curve::ID,
+                curve::coordinate(channel, point, Axis::Y),
+                step as f32 / 32.0,
+            );
+            step += 1;
+        }
+    }
+
+    let mut sidecar = Sidecar::new();
+    sidecar.put(dr_pipeline::sidecar::Version::from_graph(
+        "u1", "Default", &graph,
+    ));
+    let restored = apply(&sidecar.to_text());
+
+    for channel in Channel::ALL {
+        for point in 0..curve::POINTS {
+            let id = curve::coordinate(channel, point, Axis::Y);
+            assert_eq!(
+                restored.param(curve::ID, id),
+                graph.param(curve::ID, id),
+                "{id} did not survive the sidecar"
+            );
+        }
+    }
+}
+