fix(#1976): report the gamut transform's destination as gamt, and classify on zero - #1979
Merged
Conversation
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza,
maxderhak and
xsscx
as code owners
August 5, 2026 05:06
…ssify on zero Two independent defects on the gamt path, both reachable on the existing corpus. The gamut transform could never be used. ICC.1:2022 9.2.29 allows gamutTag to be lut8Type, lut16Type or lutBToAType -- all B-to-A shaped -- so the icXformLutGamut case in CIccXform::Create() forces bInput false to traverse the tag in its stored direction. m_bInput also drives GetDstSpace() and GetNumDstSamples(), whose !m_bInput branches answer with the profile's device space. CIccCmm::AddXform() had correctly recorded PCS -> icSigGamutData, so the two disagreed and Begin()'s trailing output-count guard rejected the chain with icCmmStatBadSpaceLink. All seven gamt-bearing profiles under Testing/ failed this way; the path had no working caller. SetGamutXform() now marks the transform so its reported destination stops following the traversal direction, which leaves m_bInput doing only the one job it is meant to do. CIccCmm::IsInGamut() applied an 8-bit threshold to a tag that need not be 8-bit, returning true for anything below 1/255 -- a faithful float rewrite of the earlier (unsigned)(v*255.0) truncation, so the assumption predates that rewrite. Since 1/255 is exactly 257/65535, every 16-bit code below 258 read as in gamut. All seven corpus gamt tables store 8-bit values replicated x257, so wherever a non-zero node exists the smallest is exactly 257 and the old test judged it correctly; it is interpolation between a zero node and a 257 node that falls underneath. Sweeping 3444 Lab coordinates through CMYK-3DLUTs.icc yields 92 out-of-gamut results the old test called in gamut, the smallest ~7.1e-15. The comparison is now against zero, as the tag definition specifies; NaN and infinity both fail it and so report out of gamut, which is the safe verdict. CIccXformMpe::Create() gets the same marking. It has no in-tree caller but is public API and its gamut case forces bInput false identically. The two AddXform paths that build gamut transforms both route through CIccXform::Create(), so they are covered; the tag-based AddXform overload has no gamut concept and is untouched. Neither Create() gamut case touches bUseSpectralPCS, so GetSrcSpace() already returns the PCS that AddXform records and the source side needs no change. Adds iccdev.gamut-xform-semantics, which pins the connection metadata and the exact-zero classification. Each assertion is red-green against one half of the fix: reverting only IsInGamut fails the four classification assertions, reverting only the destination accessors fails the connection assertion. The test builds its gamt profile in memory because every gamt-bearing profile under Testing/ is generated from XML rather than tracked, so loading one would depend on generation order. Reported by @xsscx, whose ci-qa-maintainer-test-branch established the bisect to 1f0a9dd (2015) and confirmed 321dbb1 (#1177) as a faithful rewrite rather than the origin of the threshold.
xsscx
force-pushed
the
issue-1976-gamut-semantics
branch
from
August 5, 2026 11:11
c96b45d to
49e2362
Compare
Member
I will add to TODO for the tool coverage. Thank You. |
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.
Part of #1976.
Fixes both defects reported there. Deliberately not
Fixes #1976— see "What thisdoes not include" at the end; the close is @xsscx's call.
Reported by @xsscx, whose
ci-qa-maintainer-test-branchestablished the bisect to1f0a9dd(2015) and confirmed321dbb1(#1177) as a faithful rewrite rather than theorigin of the threshold. Both findings reproduce; the analysis below is my own
verification, with one correction to the framing.
1. The gamut transform could never be used
ICC.1:2022 9.2.29 allows
gamutTagto belut8Type,lut16TypeorlutBToAType— allB-to-A shaped — so
CIccXform::Create()'sicXformLutGamutcase forcesbInputfalse totraverse the tag in its stored direction. But
m_bInputalso drivesGetDstSpace()andGetNumDstSamples(), whose!m_bInputbranches answer with the profile's device space:AddXformm_Header.colorSpace(e.g.CMYK)icSigGamutDataBegin()'s trailing output-count guard comparesGetDestSamples()against the lastxform's
GetNumDstSamples()and rejected the chain withicCmmStatBadSpaceLink.Every gamt-bearing profile in the corpus failed. A full tag-table scan of all 255
.iccunderTesting/finds exactly 7 carryinggamutTag. Measured on masterc0cd71dbwith
iccApplyNamedCmm <lab> 3 0 <profile> 30:Testing/V2/v2CmykLut16.icclut16TypeInvalid space linkTesting/CMYK-3DLUTs/CMYK-3DLUTs.icclutBtoATypeTesting/CMYK-3DLUTs/CMYK-3DLUTs2.icclutBtoATypeTesting/hybrid/ICC/CMYK_Hybrid_Profile.icclutBtoATypeTesting/hybrid/ICC/CMYK-W_Overprint_Profile.icclutBtoATypeTesting/hybrid/ICC/CMYK-S_Overprint_Profile.icclutBtoATypeTesting/hybrid/ICC/CMYK-STop_Overprint_Profile.icclutBtoAType7 of 7 red, 7 of 7 green. The path had no working caller in the tree.
SetGamutXform()now marks the transform so its reported destination stops following thetraversal direction, leaving
m_bInputdoing only the one job it is meant to do.Two scope decisions
A setter, not a
SetParams()parameter.SetParams()is documented as the API foroverridden
Createfunctions, so an added parameter is a public surface change everyout-of-tree subclass inherits.
CIccXformalready has this exact idiom one line above —ShareProfile(),SetPcsAdjustXform()— soSetGamutXform()matches it and changes noexisting signature.
The source side is deliberately untouched. Extracting both
case icXformLutGamut:blocks confirms neither assigns
bUseSpectralPCS, so it stays false andGetSrcSpace()already returns
m_Header.pcs— exactly whatAddXformrecords. Overriding the sourceaccessors too would be redundant.
2.
IsInGamut()applied an 8-bit threshold to a tag that need not be 8-bit1/255is exactly257/65535, so every 16-bit code below 258 read as in gamut. The tagdefines zero as in gamut and every non-zero value as out.
Correction to the report's framing
The report cites a measured
1.5259e-05(=1/65535) as the misclassified value. That isright about the encoding but not about how the defect is reached on real profiles.
Every
gamtvalue in all 7 corpus tables is≡ 0 (mod 257)— 8-bit values replicated ×257— so wherever a non-zero node exists at all, the smallest is exactly
257/65535 == 1/255,which the old test classified correctly. What exposes the threshold is
interpolation between a zero node and a non-zero one, where intermediate values fall
underneath.
Sweeping 3444 Lab coordinates through
CMYK-3DLUTs.icc:So the defect is real and reachable on tracked corpus data, but through the interpolator
rather than through a stored node. Without that measurement the change looks unreachable
on the corpus, so it is recorded in the code comment and the test.
The comparison is now against zero. NaN and infinity both fail it and so report out of
gamut — the safe verdict for a value the transform could not produce meaningfully.
Coverage of the other gamut paths
CIccXformMpe::Create()gets the same marking: no in-tree caller, but it is public API andits gamut case forces
bInputfalse identically.CIccCmm::AddXform()andCIccNamedColorCmm::AddXform()both route throughCIccXform::Create()with the lut typeintact, so they are covered without further change. The tag-based
AddXformoverload hasno gamut concept — it never mentions
icSigGamutData— and is untouched.Test
iccdev.gamut-xform-semantics(.github/ci/regression/gamut-xform-semantics.cpp), pinningboth the connection metadata and the exact-zero classification, registered in both call
lists in
Build/Cmake/Testing/CMakeLists.txt.It builds its gamt profile in memory: all seven gamt-bearing profiles under
Testing/are generated from XML by the suite rather than tracked, so a fixture-loading test would
depend on generation order.
Each assertion is red-green against one half of the fix — verified by ablation, and
re-verified after every restructuring of the test:
IsInGamutrevertedApply)The sub-cutoff assertion uses
256.0f/65535.0f— the largest 16-bit code the old thresholdswallowed — rather than a
nextafter()step, so it is an exact binary value on everyplatform and names the real boundary.
Pre-flight
-Wall -Wextra -Wpedantic -Werror, strict warnings ENABLEDgrep -cE 'warning:'= 0, errors = 0ASAN_OPTIONS=detect_leaks=1git diff --checkThe single failure in every lane is
iccdev.spectral-tiff-preview— a missing local pythonimagecodecsmodule, failing identically on unmodified master here, not a regression.All 7 corpus profiles above were also re-run under ASan+UBSan with leak detection: rc=0,
zero findings.
What this does not include
ci-qa-maintainer-test-branchalso carries synthetic.icc/.xml/.json/.tiffixtures,a shell regression driver, and an
iccApplyProfilesintent-30 TIFF path. None of that ishere: the two library defects reproduce on profiles already in the corpus, so the added
fixtures are not needed to pin them, and the tool-level coverage is a separate scope call.
That is why this is
Part ofrather thanFixes— if you consider the tool-levelregressions in scope for #1976, they still need doing.