feat: color source/target split by DatCaptainHorse · Pull Request #38 · hgaiser/pixelforge

DatCaptainHorse · 2026-09-10T12:17:04Z

Description

Based/builds off PR #37 (required to be merged before this PR).

Per issue #36 for this one as well, but for the color handling as this includes API breaking changes, it makes more sense as a separate PR.

Basically the current API feels bit detached and allows for results that don't really make sense, such as ColorConverterConfig::full_range being set to true without calling ColorDescription.with_full_range(true) in combination, results in full-range output but the metadata falsely claims it as TV-range.. causing color issues when decoding.

So instead of doing this:

let mut config = ColorConverterConfig::new(1920, 1080, InputFormat::BGRx, OutputFormat::P010);
config.color_space = ColorSpace::SrgbToBt2020Pq;

let mut converter = ColorConverter::new(context.clone(), config)?;
// converter.convert(input_image, output_buffer)?;
...

the color config is set as so:

let config = ColorConverterConfig::new(1920, 1080, InputFormat::BGRx, OutputFormat::P010)
    .with_source(SourceColor::srgb())
    .with_target(TargetColor::Bt2020Pq)
    .with_range(ColorRange::Full);
    
let mut converter = ColorConverter::new(context.clone(), config)?;
let mut encoder = Encoder::new(context, encode_config)?;
// converter.convert(input_image, layout, encoder.input_image())?;

Source -> Target logic, bit clearer I'd say?

WIP/RFC @hgaiser - feel free to suggest changes or reject the API changes outright.

DatCaptainHorse · 2026-09-15T15:09:19Z

@hgaiser any objections to this idea? Or changes wanted? Just poking since not sure if you've seen this yet 🙂

hgaiser · 2026-09-15T15:31:23Z

I haven't had time to look into this yet, sorry :)

hgaiser

Alright took me some time to wrap my head around the situation and what you're proposing but I think I got most of it. To summarize in my own words: you identified that ColorSpace was used in different contexts and carried different meanings, so you opted to split it into a SourceColor and TargetColor enum, correct? I think that's a very valid point.

I like the approach, I think it makes sense. I briefly checked other libraries (libavcodec mainly). They don't seem to have a source / target split but rather enums / fields like AVPixelFormat, AVColorRange, AVColorSpace, AVColorPrimaries. What made you decide to use a split of SourceColor / TargetColor? What if we want bidirectional conversion at some point in the future? Then source/target becomes ambiguous.

DatCaptainHorse · 2026-09-18T15:26:09Z

What made you decide to use a split of SourceColor / TargetColor? What if we want bidirectional conversion at some point in the future? Then source/target becomes ambiguous.

It was mostly to keep the cases with "odd" source/target combos limited and reduce edge-cases, but yeah it does add unneeded complexity to the API and limits it. I'll change things to be more unified, will need more tests for the various combinations but this is a draft PR still, will work on it 👍

`ColorSpace` was an enum of pipelines, not of colour spaces. Each variant bundled three separate decisions under one name: what the input already is, what the output should be, and which YUV matrix to use. Two of the four were passthroughs, and for those the library could not say what the output transfer function was: `ColorSpace::Bt2020` means "already BT.2020-encoded, write it through", which covers PQ, HLG and SDR BT.2020 alike. That is why the range and matrix decisions had to be made twice, once on `ColorConverterConfig` and once on `ColorDescription`, with nothing linking them. Neither could be derived from the other. `ColorDescription` to `ColorSpace` was ambiguous because the BT.2020 NCL matrix matched three variants; `ColorSpace` to `ColorDescription` was ambiguous because `Bt2020` could not fill in the transfer characteristics. A setter bridging them would have had to guess, and it is better not to have one than to have one that guesses. So the input and the output are now described separately: let conv = ColorConverterConfig::new(w, h, input_format, output_format) .with_source(SourceColor::scrgb()) .with_target(TargetColor::Bt2020Pq) .with_range(ColorRange::Full); let enc = EncodeConfig::h265(w, h) .with_color_description(conv.color_description()); `SourceColor` says what the pixels are; `TargetColor` says what the stream is and picks the matrix; `ColorRange` replaces the `full_range` bool, which reads better at a call site than `true` did. Because `TargetColor` fixes primaries, transfer and matrix together, `color_description()` derives the encoder's declaration from the conversion itself, with nothing left to guess. The two structs can still disagree if you insist on building the description by hand, but the drift is no longer the path of least effort, and full-range samples tagged limited get expanded twice on playback. Two more things fall out of the split: The reference white now lives on the source variants, because that is whose property it is: it answers what luminance a sample value of 1.0 stands for. `SourceColor::srgb()` carries BT.2408's 203 nits and `SourceColor::scrgb()` carries IEC 61966-2-2's 80, so the correct value follows from the source instead of one 203 nit default serving both. Getting that wrong maps scRGB white about two and a half times too bright, and it was a real downstream bug, not a hypothetical one. `SourceColor::Bt2020Pq` has no such field, PQ being absolute already. And unsupported conversions are now refused. `Bt2020Linear` is newly expressible and implemented, so every source reaches `Bt2020Pq`; only `Srgb` reaches `Bt709`, since the others would need a forward gamma encode or tone mapping that the shader does not do. Those three pairs error out of `ColorConverter::new` and `set_source`/`set_target` instead of silently writing samples that do not match their label. Breaking, hence the `!`: `ColorSpace` is gone, and the three colour fields on `ColorConverterConfig` are replaced. The push constant block grows to 8 u32s and the shader takes `source`, `target` and `range` in place of `color_space` and `full_range`.

The split in the previous commit rearranged which shader stages run for which source and target. Getting one of those wrong does not fail loudly: it produces a plausible picture in the wrong colour, and the only way to catch it is to compute the same thing twice by different means. `convert_color_paths` reimplements the pipeline on the CPU stage for stage, selecting the stages from the source and target rather than from the shader's branches, and compares the luma plane sample by sample. All five supported pairs at 8-bit and 10-bit agree to within one code value, which is the slack the GPU needs to order and fuse its f32 arithmetic differently: Srgb -> Bt709 worst 1.0, mean 0.0002 VUI 1/1/1 Srgb -> Bt2020Pq worst 1.0, mean 0.0005 VUI 9/16/9 Bt709Linear -> Bt2020Pq worst 1.0, mean 0.0003 VUI 9/16/9 Bt2020Linear -> Bt2020Pq worst 1.0, mean 0.0005 VUI 9/16/9 Bt2020Pq -> Bt2020Pq worst 0.0, mean 0.0000 VUI 9/16/9 A passing run means something because the comparison is sensitive to the things it is checking. Modelling scRGB at 203 nits instead of 80 takes the mean to 22.5; dropping the BT.709 to BT.2020 gamut hop takes the worst case to 55. Both were confirmed by hand before this landed. It prints the derived `color_description()` alongside each result, so the VUI code points are visible next to the pixels they describe, and it asserts that the three unsupported pairs are refused rather than quietly converted. Same measurement discipline as `convert_quantization`, and luma only for the same reason: chroma is a 2x2 average, which would mix averaging error in.

Addresses hgaiser's review on hgaiser#38. His objection to the source/target split was that the names stop meaning anything if conversion ever runs the other way, and he is right. libavcodec is not really the counterexample he reached for, since its colour fields describe one image and swscale takes a source frame and a destination frame each carrying their own set, so it has a source and a target too, expressed as two instances of one type. That is the choice: one symmetric type used at both ends, or two asymmetric ones. Taking the symmetric option turns out to remove a type rather than add one. `TargetColor::Bt709` was the same colour space as `SourceColor::Srgb`, and `TargetColor::Bt2020Pq` the same as `SourceColor::Bt2020Pq`; the only thing separating them was the direction they were used in, which belongs to the slot and not to the value. So `TargetColor` is gone and `SourceColor` is renamed: ColorConverterConfig::new(w, h, input, output, ColorSpec::scrgb(), // source slot ColorSpec::Bt2020Pq, // target slot ColorRange::Full) Not every space can be a target. There are no H.273 code points for linear light, so `Bt709Linear` and `Bt2020Linear` are source-only, which `ColorSpec::is_encodable` answers and `ColorConverter::new` enforces. That makes `ColorConverterConfig::color_description` partial, returning `Option`. The partiality is confined: `ColorConverter::color_description` is infallible, because a converter only exists if its target passed that check, and callers doing the normal thing never see the `Option`. Also in this round, from the rest of the review: The `RGBA16F` doc still explained itself in terms of the old `ColorSpace` variants. It now says the `ColorSpec` decides the interpretation, not the pixel format. `set_target` claimed an encoder's session parameters were fixed when it was built. They are not: `Encoder::set_color_description` drains in-flight encodes, rebuilds them, invalidates the header cache and requests an IDR. The doc points there instead of implying the caller is stuck. Same on `set_range`. The module prose takes hgaiser's suggested wording, and two doctests that the constructor change had broken now pass. One bug found on the way: `unsupported_conversion` had a match arm returning the string "unreachable" for the PQ target. An edit meant to remove it in the previous round silently did not apply, and I never checked. With the unified type there are two real reasons a pair can be refused, and both now say which. Verified on an Arc A310. All ten supported source, target and format combinations still agree with the CPU model to within one code value, the quantizer example still reports the same four rows, and the refusals now cover both kinds: three SDR-target pairs and two unencodable targets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Carrying nits in the variants forced a constructor per space, and the call sites ended up mixing two spellings of the same thing: ColorSpec::scrgb(), // a method ColorSpec::Bt2020Pq, // a variant Keeping the data and dropping the constructors is not an option on stable: default values on enum fields are still unstable (rust-lang#132162), so every call site would have to spell out `ColorSpec::Bt709Linear { reference_white_nits: 80.0 }`, which is worse than the constructors were. So the data comes out and `ColorSpec` is field-less. Every space is now named one way: ColorSpec::Bt709Linear, ColorSpec::Bt2020Pq, The reference white becomes something a space knows rather than carries. `ColorSpec::reference_white_nits()` returns the standard figure, 203 for the SDR-referred spaces per BT.2408 and 80 for scRGB per IEC 61966-2-2, so the right value still follows from the source without anyone choosing it. `ColorConverterConfig::with_reference_white_nits` overrides it for the rare conversion that needs a different one, which puts the override on the conversion where it belongs rather than on the colour space, where it never made sense. One property is lost and worth naming: `scrgb()` made it impossible to build scRGB with the wrong white, and now an override can set it to 203. That is a deliberate call rather than a default, which is the trade for uniform call sites. Field-less also means `Eq` and `Hash` derive, and the discriminant is the shader's, so `as u32` replaces the private `shader_id()` helper. Verified on an Arc A310: all ten supported combinations give the same numbers as before, in particular `Bt709Linear -> Bt2020Pq` still at 80 nits, so the standard figure reaches the shader. Both refusal kinds still fire, and the quantizer example is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`ColorConverterConfig::new` takes the source, target and range, so `with_source`, `with_target` and `with_range` could only ever restate what the constructor had just been told. Two ways to say one thing, and the builder form read as though the constructor's arguments were defaults that could be left out, which they are not. `with_reference_white_nits` stays. It is the one colour setting `new` does not take, because the spec's own figure is right nearly always and an override is worth making deliberate. The unit tests used the builders to vary one slot at a time; they take a helper that passes both slots instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Two things hgaiser asked for on hgaiser#38, and the repo restructure in 936d412 made the second one easy to answer. The checks are tests, not examples. They demonstrate nothing a caller wants to copy; they compute what the converter should produce and compare. As examples they ran only when someone deliberately ran them, which is approximately never. As integration tests `cargo test -- --ignored` runs them on any machine with a video device, alongside `roundtrip` and `verify_all`. `convert_quantization` and `convert_color_paths` become one file, since they shared about two hundred lines of Vulkan scaffolding and a synthetic frame. Three tests come out of it: - `quantizers_round_rather_than_truncate`, over NV12 and P010 in both ranges - `every_conversion_matches_the_model`, the five supported ColorSpec pairs at both bit depths, which also asserts the derived declaration describes the target - `unsupported_conversions_are_refused`, both refusal kinds, which also pins `color_description()` and `is_encodable()` to the same answer The examples printed their findings and returned an error; tests assert. The quantization one now fails on either evidence of truncation, a rounding hypothesis that fits worse than a truncating one or a mean signed error above 0.05, rather than only on the first. Confirmed by making the shader truncate again and watching it fail, then restoring it and checking the SPIR-V came back byte-identical. `SrcImage` gained a `Drop`, since a test leaves scope without the explicit teardown an example's `main` had. Measured on an Arc A310, unchanged from the examples: all ten source, target and format combinations within one code value of the model, quantizer bias at -0.0006 and +0.0017 for NV12, -0.0012 and +0.0040 for P010. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The colour docs were written for someone who already knew the standards. They named specs (H.273, ITU-R BT.2408, IEC 61966-2-2) without saying what those mean, described "the luminance a sample value of 1.0 represents" where "how bright white is" would do, and explained the design's reasoning where a caller only wanted to know what to pass. Same facts, fewer words, no citations standing in for explanations: before: The two linear specs are source-only because H.273 has no code points for linear light, so a decoder cannot be told a stream is in one. after: The linear spaces cannot be a target, because a video file has no way to record that it holds linear light. before: The luminance a sample value of 1.0 represents in this space, in nits, where that is a meaningful question to ask. after: How bright this space's white is, in nits. Error messages got the same treatment, since they are read by people who hit them rather than people studying the API. The variant docs now say what each space is for rather than reciting its primaries and transfer function. About sixty lines shorter overall, with nothing dropped that a caller needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`Srgb` and `Bt709Linear` are the same colours, differing only in the curve, but nothing said so. The `Srgb` variant never mentioned BT.709 at all, so the two read as unrelated families and the name looked like a mistake. Also notes that scRGB values above 1.0 are allowed and mean brighter than SDR white, since that is the whole reason the space is useful here and is not guessable from "linear light". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

DatCaptainHorse · 2026-09-18T16:34:41Z

@hgaiser unified the source/target colors into ColorSpec enum. ColorConverter checks the given color params if the shader supports their conversion, returning error result explaining why not.

It's still 2 parameters in new, if that's too much, another option I see is unifying InputFormat and OutputFormat like the source/target colors were here and then unify them both under some kind of generic format config on top..

Lots of options, let me know what you think 🙂

hgaiser

Looks good to me :)

DatCaptainHorse · 2026-09-21T16:47:14Z

@hgaiser I don't think this PR needs anything else, feel free to merge when available, I got plenty more PRs incoming :P

hgaiser · 2026-09-22T07:57:46Z

@hgaiser I don't think this PR needs anything else, feel free to merge when available, I got plenty more PRs incoming :P

Oh dear (but also <3). I'll be away for about two weeks so might take a while :p

hgaiser reviewed Sep 16, 2026

View reviewed changes

DatCaptainHorse and others added 6 commits September 18, 2026 18:55

DatCaptainHorse force-pushed the feat/color-source-target-split branch from d6bb4c2 to 9c74384 Compare September 18, 2026 15:59

DatCaptainHorse and others added 2 commits September 18, 2026 19:05

hgaiser approved these changes Sep 18, 2026

View reviewed changes

DatCaptainHorse marked this pull request as ready for review September 18, 2026 22:34

hgaiser merged commit 7fb6f93 into hgaiser:main Sep 22, 2026
7 checks passed

Navigation Menu

Repository navigation

feat: color source/target split - #38

feat: color source/target split#38
hgaiser merged 8 commits into
hgaiser:mainfrom
DatCaptainHorse:feat/color-source-target-split

DatCaptainHorse commented Sep 10, 2026

Uh oh!

DatCaptainHorse commented Sep 15, 2026

Uh oh!

hgaiser commented Sep 15, 2026

Uh oh!

hgaiser left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DatCaptainHorse commented Sep 18, 2026

Uh oh!

DatCaptainHorse commented Sep 18, 2026

Uh oh!

hgaiser left a comment

Uh oh!

DatCaptainHorse commented Sep 21, 2026

Uh oh!

hgaiser commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

DatCaptainHorse commented Sep 10, 2026

Description

Uh oh!

DatCaptainHorse commented Sep 15, 2026

Uh oh!

hgaiser commented Sep 15, 2026

Uh oh!

hgaiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DatCaptainHorse commented Sep 18, 2026

Uh oh!

DatCaptainHorse commented Sep 18, 2026

Uh oh!

hgaiser left a comment

Choose a reason for hiding this comment

Uh oh!

DatCaptainHorse commented Sep 21, 2026

Uh oh!

hgaiser commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants