feat(gallery): colour-managed decode/encode round-trip GUI, and the library colour fixes it forced - #67
Open
justin13888 wants to merge 7 commits into
Open
Conversation
`decode_standard_image` and `decode_standard_image_with` ran every result through `tag_srgb`, which overwrote the colour description with sRGB unconditionally. A Display P3 HEIC and an sRGB PNG came back indistinguishable, and `ImageProbe::color_space` was a hardcoded constant that never looked at the file at all. Callers had no way to learn a decoded image's actual colour space short of calling `read_standard_image_metadata` separately and parsing the ICC blob themselves. `rawshift-image-core::color_resolve` now maps what a container declares — a CICP code-point pair, an embedded ICC profile, or neither — onto a `ColorDescription`, and the five decoders that already hold that information (PNG, JPEG, WebP, AVIF, HEIC) tag with it. The remaining formats have no colour path in rawshift and keep the sRGB default, which `default_to_srgb` now applies by format rather than blanket. Resolution precedence is CICP, then the profile, then sRGB. A profile that is recognisably sRGB (or linear sRGB) is tagged as such; anything else resolves to `UNSPECIFIED`. That is not a shortfall — `ColorDescription` is a CICP pair and Adobe RGB / ProPhoto have no faithful CICP expression, so `UNSPECIFIED` is what that type already documents for them, with the profile preserved verbatim in `ImageMetadata::icc_profile`. Two details worth review attention: - The colorant tolerance is 4e-3, not something tighter. The sRGB profiles in circulation genuinely disagree: the ICC's own releases put the green colorant's Z at 0.09708, the HP/Microsoft "sRGB IEC61966-2.1" profile that rawshift itself embeds puts it at 0.09500. A tolerance that split them would make rawshift's own encode output round-trip as `UNSPECIFIED`; `rawshift_own_srgb_profile_is_recognised` pins this. Display P3, the nearest confusable space, is 0.079 away — twenty times the tolerance. - A single-entry `curveType` is matched by exponent range (2.1..=2.3) rather than by sampling against the sRGB EOTF. A 2.2 power law and the sRGB curve differ by more than the sampling tolerance in the toe, so sampling would reject the gamma-2.2 approximation that most v2 profiles — rawshift's included — use. JPEG's CMYK branch deliberately does not use the embedded profile: it describes the CMYK samples, not the RGB the Blinn approximation synthesises from them. `probe_standard_image` now reports the real colour space for JPEG, WebP, AVIF, and HEIC, all of which are a marker/chunk/box walk. PNG reports `UNSPECIFIED` — its `iCCP` chunk is DEFLATE-compressed and gamut-png exposes ancillary chunks only from a full decode, so reading it in a probe would decode every pixel. Marked `!` because decode output that was always `SRGB` can now be `DISPLAY_P3`, `REC2020`, `LINEAR_SRGB`, or `UNSPECIFIED`. Nothing inside rawshift reads `RgbImage::color()` on the decode path — `convert_to_srgb` has no internal callers — so this changes no behaviour within the crate. 644 workspace tests pass with `--features rawshift-image/full`, including the 32 fixture-backed standard decode tests. Claude-Session: https://claude.ai/code/session_019wHgpYYx1hJJ5NkMh6XiDB
Every `embed_icc` branch — PNG, JPEG, WebP, AVIF, JXL — wrote `IccProfile::srgb()`, so encoding discarded whatever profile the source carried and replaced it with a synthesised sRGB one. An Adobe RGB or ProPhoto RGB image survived decode with its pixels intact and its profile preserved in `ImageMetadata`, then lost the profile on the way out: the samples were still wide-gamut but the file now claimed to be sRGB. `profile_to_embed` picks the profile using the image's `ColorDescription` as the arbiter, which is exactly the distinction the previous commit made available: - `UNSPECIFIED` means the decoder found an ICC-authoritative profile with no faithful CICP expression, so the profile *is* the colour space and is embedded verbatim. - Any named space means the samples are in that space whatever `metadata` happens to carry, and the synthesised sRGB profile is used. That second arm is load-bearing rather than a fallback. `RawFile::process` tags its output `SRGB` while the source file's metadata may carry a *camera* profile describing the sensor, not the developed image; embedding it would mis-tag every RAW export. `named_colour_space_does_not_adopt_an_unrelated_profile` pins this. Known gap, documented on the helper: an image tagged `DISPLAY_P3` or `REC2020` from a container's CICP box still gets an sRGB profile, because rawshift can only synthesise sRGB and the source carried code points rather than a profile. The right fix is to write the code points through to the output container (a PNG `cICP` chunk, an AVIF `colr nclx` box), which is tracked separately. 646 workspace tests pass with `--features rawshift-image/full`. Claude-Session: https://claude.ai/code/session_019wHgpYYx1hJJ5NkMh6XiDB
rawshift had eight CLI examples but nothing showing the decode → encode → decode loop end to end, and nothing making hardware decode visible. The VideoToolbox backend's only proof was a device-gated test suite. `examples/gallery` is a cross-platform iced GUI: pick images, and each is decoded, re-encoded through every selected format with every parameter exposed, written to the system temp directory, decoded back, and shown side by side against the source with PSNR / max-delta / bit-exact verdicts. A stage that fails puts its error text in an empty placeholder rather than dropping the tile. `gamut-cmm` is a git dependency, and the gamut repository's aom/dav1d submodules make that a ~1.4 GB checkout. As a workspace member that cost would land on every CI job — including ones naming it in `--exclude`, since Cargo still reads every member manifest to build the resolve graph. Standalone, only `just gallery*` and the dedicated `gallery` CI job pay it. It also keeps iced/wgpu/winit out of `cargo test -p rawshift-image`, which an `[[example]]` could not have done: Cargo has no optional dev-dependencies. The git dependency itself is a scoped carve-out to the Upstream-First Policy, now recorded in AGENTS.md. The no-git rule exists because git deps prevent publishing; that does not reach a crate which is `publish = false` and outside the workspace. gamut-cmm is the only way to apply an ICC transform to pixels — gamut-icc parses profiles but explicitly does not transform them — and it is not on crates.io yet. Note that gamut-cmm pulls gamut-core/color/icc from the *git* checkout, a different Cargo source from the crates.io copies rawshift uses, so this crate links two `gamut-icc` crates whose types are not interchangeable. ICC data crosses that boundary as bytes and is re-parsed; `src/color.rs` documents it. `pipeline`, `color`, `settings`, and `render` hold no iced types, so `--headless` and the 35 unit tests drive exactly the code the window does — there is no second implementation to drift. Work runs on a blocking worker one source at a time, so a large RAW cannot freeze the window. On this M3 Pro, rawshift cannot decode its own AVIF output: gamut-avif encodes identity-matrix 4:4:4 (AV1 Profile 1) and VideoToolbox's still-image path decodes Main / Profile 0 only. Both halves work in isolation, and nothing in the test suite crosses them. The gallery shows the decode error rather than hiding the column, and the hardware-absence heuristic is deliberately narrow so a decoder that *rejects* a bitstream is counted as a failure rather than excused as a missing backend. Run from the repo root on macOS 26.6, M3 Pro: | Command | Result | | --- | --- | | `cargo fmt --all -- --check` | clean | | `cargo clippy --workspace --all-targets -- -D warnings` | clean | | `cargo clippy -p rawshift-image --all-targets --features full -- -D warnings` | clean | | `cargo test --workspace --features rawshift-image/full` | 646 passed, 0 failed | | `cargo check --workspace` (MSRV job shape) | clean; does not build the gallery | | `cargo fmt --manifest-path examples/gallery/Cargo.toml -- --check` | clean | | `cargo clippy --manifest-path examples/gallery/Cargo.toml --all-targets -- -D warnings` | clean | | `cargo test --manifest-path examples/gallery/Cargo.toml` | 35 passed | | `just gallery-headless` on PNG/JPEG/AVIF/HEIC fixtures | table below | The whole tree — iced 0.14, wgpu, git gamut-cmm, libjxl — builds on the pinned 1.92.0 toolchain. Headless run against the generated fixtures reports the VideoToolbox backend with HEVC + AV1, decodes the HEIC through hardware, reads its colour from the `colr` box (CICP 1/13), and round-trips PNG and JPEG XL bit-exact with JPEG at 31.88 dB and WebP at 28.68 dB. Claude-Session: https://claude.ai/code/session_019wHgpYYx1hJJ5NkMh6XiDB
Cross-references the issues opened while building the gallery, so the gaps documented in code comments point at something trackable rather than saying "tracked separately": - gamut#375 — gamut-avif encodes AV1 Profile 1, which VideoToolbox's still-image path cannot decode. Found by the gallery: rawshift cannot read back its own AVIF output on Apple hardware. - gamut#376 — `ColourPrimaries` chromaticities and an `oetf_for` inverse in gamut-color, the two pieces missing before wide-gamut conversion can live in the library. - gamut#377 — publish gamut-cmm, which retires the git carve-out. - gamut#378 — mark the aom/dav1d submodules `update = none`; the ~1.4 GB fetch is why the gallery sits outside the workspace. - gamut#379 — a metadata-only entry point for gamut-png, which is why a PNG probe cannot report a colour space. - rawshift#66 — extend `convert_to_srgb` to Display P3 and Rec. 2020 (blocked on gamut#376), plus the unblocked encode-side half: writing CICP code points through to the output container. Claude-Session: https://claude.ai/code/session_019wHgpYYx1hJJ5NkMh6XiDB
`pull_request: branches: [master]` meant a stacked PR — one feature branch based on another, to be retargeted to master once the parent lands — got **no** checks. Not pending ones: zero. #67 sat MERGEABLE with an empty status rollup, which reads as "nothing to run" rather than "nothing has been verified". That is worst for exactly the changes a stack introduces. #67 adds a three-OS `gallery` job, and without this the iced/wgpu/winit tree and the git gamut-cmm dependency would never have been built on Linux or Windows before review. Dropping the filter costs CI minutes on stacked PRs. The alternative — leaving them unverified until the parent merges — hides platform failures until the moment the branch is retargeted, which is the worst time to discover them. `push` keeps its `branches: [master]` filter, so this does not double up on every feature-branch push. Claude-Session: https://claude.ai/code/session_019wHgpYYx1hJJ5NkMh6XiDB
`rfd = "0.15"` took the crate's default features, which are `xdg-portal` + `async-std`. That pulled a whole second async runtime alongside the tokio executor iced is already configured with, and made the comment above the iced dependency — claiming rfd shares that executor — untrue. `default-features = false` with an explicit `xdg-portal` + `tokio` keeps rfd's own default Linux backend while dropping async-std entirely (`cargo tree -i async-std` now finds no such package). Worth recording why `xdg-portal` and not rfd's `gtk3` alternative: the portal backend needs no system development packages, so the Linux CI job installs nothing on rfd's behalf. `gtk3` would have required libgtk-3-dev on every runner. 35 gallery tests pass; fmt and clippy clean. Claude-Session: https://claude.ai/code/session_019wHgpYYx1hJJ5NkMh6XiDB
The Linux CI leg failed in `wayland-sys`, whose build script requires
`wayland-client` through pkg-config:
The system library `wayland-client` required by crate `wayland-sys`
was not found.
`default-features = false` on iced dropped `iced_winit`'s default `x11`
and `wayland` features. Those are not optional extras on Linux — they are
the only windowing backends there, so the crate had no way to open a
window on Linux at all, and something in the tree still pulled winit's
wayland support down the non-dlopen path.
Selecting them explicitly fixes both problems at once: `iced_winit/wayland`
brings `winit/wayland-dlopen`, which loads libwayland at runtime instead
of linking it, so neither the CI job nor a Linux user needs
libwayland-dev.
This also corrects a claim I had made in the CI comment and the gallery
README — that winit dlopens X11/Wayland so no packages are needed. That
was only true with these features enabled, which they were not. Both now
say what actually makes it true, and why removing the features breaks it.
macOS and Windows were unaffected (both legs were still building when
this landed), which is exactly why the Linux leg was worth having.
35 gallery tests pass; fmt and clippy clean.
Claude-Session: https://claude.ai/code/session_019wHgpYYx1hJJ5NkMh6XiDB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #65 (
feat/28-videotoolbox-backend). Review that one first; this PR's diff is against it, and the base should be retargeted tomasteronce #65 lands.rawshift had eight CLI examples but nothing showing the decode → encode → decode loop end to end, and nothing making hardware decode visible — #65's VideoToolbox backend's only proof is a device-gated test suite.
examples/galleryis a cross-platform iced GUI: pick images, and each is decoded, re-encoded through every selected output format with every parameter exposed, written to the system temp directory, decoded back, and shown side by side against the source with PSNR / max-delta / bit-exact verdicts. A stage that fails puts its error text in an empty placeholder rather than dropping the tile.Building it forced the colour question, which is why two thirds of this diff is library work.
What's here
Four commits, each independently reviewable:
feat(image)!: tag decoded images with the container's colour space—tag_srgboverwrote every decode result with sRGB, so a Display P3 HEIC and an sRGB PNG came back indistinguishable. Newrawshift-image-core::color_resolvemaps a container's CICP pair and/or ICC profile onto aColorDescription; PNG, JPEG, WebP, AVIF, and HEIC tag from what they already parse.feat(image): preserve the source ICC profile through encode— everyembed_iccpath wrote a synthesised sRGB profile, discarding the source's.feat(gallery): colour-managed decode/encode round-trip GUI— the GUI itself.docs:— cross-references to the six issues filed along the way.Three things worth review attention
1. The tagging change is marked
!, but nothing inside rawshift reads the tagDecode output that was always
SRGBcan now beDISPLAY_P3,REC2020,LINEAR_SRGB, orUNSPECIFIED. I expected this to be the risky change and audited every consumer:convert_to_srgb— the one function that errors on wide-gamut input — has no internal callers, and nothing on the encode path readsRgbImage::color()except the new helper in commit 2. So the blast radius is external only.UNSPECIFIEDis load-bearing rather than a shrug.ColorDescriptionis a CICP pair, and Adobe RGB / ProPhoto have no faithful CICP expression; that value's own doc comment already prescribed this case. Preserving the distinction is what lets commit 2 decide correctly.The colorant tolerance is 4e-3, deliberately loose. The sRGB profiles in circulation genuinely disagree — the ICC's own releases put the green colorant's Z at 0.09708, the HP/Microsoft profile rawshift itself embeds puts it at 0.09500. A tolerance splitting them would make rawshift's own encode output round-trip as
UNSPECIFIED;rawshift_own_srgb_profile_is_recognisedpins that. Display P3, the nearest confusable space, is 0.079 away — twenty times the tolerance.Tone curves are matched by exponent range, not by sampling. A 2.2 power law and the sRGB EOTF differ by more than the sampling tolerance in the toe, so sampling would reject the gamma-2.2 approximation most v2 profiles use.
2. The gallery is outside the workspace, not merely excluded from CI jobs
The approved plan said "workspace member,
--excludeon the heavy jobs". That turned out not to work.gamut-cmmis a git dependency and the gamut repository'saom/dav1dsubmodules make it a ~1.4 GB checkout; as a member, that cost lands on every job that resolves the workspace, including ones naming it in--exclude, because Cargo still reads every member manifest to build the resolve graph. So it sits in[workspace] excludewith its own[workspace]table and lockfile, reached only byjust gallery*and one dedicated CI job.cargo check --workspaceverifiably does not build it. Filed upstream as gamut#378.This also keeps iced/wgpu/winit out of
cargo test -p rawshift-image— which an[[example]]could never have done, since Cargo has no optional dev-dependencies.3. The gamut-cmm git carve-out, and the two gamut trees
AGENTS.mdforbids git dependencies on gamut because they prevent publishing. That reason does not reach a crate which ispublish = falseand outside the workspace: no published crate's tree contains it, andcargo publish -p rawshift-imagenever sees it. The carve-out is recorded inAGENTS.md, scoped to this one package, and expires when gamut-cmm reaches crates.io (gamut#377).The sharp edge: gamut-cmm depends on gamut-core/color/icc by path, so over git those resolve to the git checkout — a different Cargo source from crates.io even at identical version numbers. The gallery therefore links two
gamut-icccrates whoseIccProfiletypes are not interchangeable. ICC data crosses that boundary as bytes and is re-parsed;src/color.rsdocuments it, andicc_authoritative_source_goes_through_gamut_cmmproves the seam works.What the gallery already found
rawshift cannot decode its own AVIF output on Apple hardware.
gamut-avifencodes identity-matrix 4:4:4 (AV1 Profile 1); VideoToolbox's still-image path decodes Main / Profile 0 only. Both halves work in isolation, and nothing in the test suite crossed them — the encoder tests check the bitstream, the hardware tests use externally-generated Profile 0 files. Filed as gamut#375.That also made me tighten the gallery's hardware-absence heuristic: a decoder that rejects a bitstream is counted as a failure, not excused as a missing backend.
a_rejected_bitstream_is_not_treated_as_missing_hardwarepins it.Issues filed
ColourPrimarieschromaticities +oetf_forinverse in gamut-colorupdate = noneconvert_to_srgbto P3/Rec.2020 —blocked-upstreamon gamut#376Per the Upstream-First Policy, none of these is worked around in rawshift. The wide-gamut display path in the gallery falls back to sRGB with a visible warning rather than duplicating gamut's primaries table.
Known gaps, stated rather than hidden
UNSPECIFIEDfor colour:iCCPis DEFLATE-compressed and gamut-png exposes ancillary chunks only from a full decode, which a header-only probe must not do (gamut#379).DISPLAY_P3/REC2020from a CICP box is still encoded with an sRGB profile — rawshift can only synthesise sRGB, and the source carried code points, not a profile. Writing them through to the output container is the right fix (rawshift#66).--all-featuresanywhere; the hw backend pins are mutually exclusive by design.Two things CI caught after this PR was opened
CI was not running on this PR at all.
ci.ymlwas gated topull_request: branches: [master], so a stacked PR got an empty statusrollup — which reads as "nothing to run" rather than "nothing verified". The
trigger now has no branch filter (
pushkeeps itsmasterfilter, sofeature-branch pushes do not double up). This is a repo-wide change and its own
commit; worth a deliberate look.
The gallery did not build on Linux.
default-features = falseon iceddropped
iced_winit'sx11andwaylandfeatures. Those are not optionalextras there — they are the only windowing backends on Linux, so the crate
could not have opened a window at all, and the build fell through to
wayland-sysdemandingwayland-clientvia pkg-config. Selecting themexplicitly fixes both halves:
iced_winit/waylandbringswinit/wayland-dlopen, so libwayland loads at runtime and neither CI nor aLinux user needs libwayland-dev.
That also falsified a claim I had written into the CI comment and the gallery
README — that winit dlopens X11/Wayland so no packages are needed. True only
with those features enabled, which they were not. Both now state what makes
it true and what breaks without it.
Everything passed on my Mac and on the macOS and Windows CI legs; only Linux
caught it. A third fix dropped rfd's default
async-std, which was pulling asecond async runtime alongside iced's tokio and contradicting a comment
claiming they shared one.
Validation
Full CI matrix green (see above). The table below is the local run on this
machine (macOS 26.6, M3 Pro, VideoToolbox active) — the hardware-dependent
evidence CI cannot produce, since hosted runners have no dependable hardware
decode block.
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo clippy -p rawshift-image --all-targets --features full -- -D warningscargo test --workspace --features rawshift-image/fullcargo check --workspace(MSRV job shape)cargo doc --workspace --no-deps --features rawshift-image/fullcargo fmt --manifest-path examples/gallery/Cargo.toml -- --checkcargo clippy --manifest-path examples/gallery/Cargo.toml --all-targets -- -D warningscargo test --manifest-path examples/gallery/Cargo.tomlFixtures were generated (
generate_test_fixtures --features full) so the 32 fixture-backed standard decode tests actually ran rather than skipping.The whole gallery tree — iced 0.14, wgpu, git gamut-cmm, libjxl via cmake — builds on the pinned 1.92.0 toolchain.
Behaviour verified, not just "didn't error"
just gallery-headlesson the generated fixtures:colrbox, not a hardcoded constant.png_round_trip_is_bit_exact_through_the_filesystemasserts the file exists — so re-decode exercises sniffing and container parsing on bytes not produced in-process.adobe_rgb_profile_survives_a_png_round_trip), and an sRGB-tagged image does not adopt an unrelated camera profile (named_colour_space_does_not_adopt_an_unrelated_profile) — the pair that keeps RAW export correct.Not verified
The window itself was not opened in this session — no display was available. The GUI code compiles clean under
clippy -D warnings, and every decision it makes lives in the iced-free modules that--headlessand the 35 unit tests exercise, but the rendered layout has not been looked at by a human. Worth doing before merge.Merge readiness
Ready for review, not merged. Retarget to
masterafter #65 lands. Per DEVELOPMENT.md, no hardware backend changed here, so thejust test-hwpre-release gate is unaffected.