Skip to content

PR Summary #2332 #2333

Description

@xsscx

PR Summary

Moves PCS adjustments out of CIccXform::Apply() and into CIccPcsXform, so that
adjustments which cancel are folded away by Optimize() instead of leaving a float
residual in the output.

Relates to #2330 and #2331 — both are pre-existing defects this work surfaced and
documented. Neither is fixed here; see "Behaviour changes" below for what this branch
does to them.

Why

A PCS adjustment — absolute-colorimetric media-white scaling, the v2-perceptual
black-point shift, or an IIccAdjustPCSXform hint such as BPC — used to be applied in
two different places depending on where a transform sat in the chain. On an interior
PCS connection, CheckPCSConnections() cleared suppression flags and Connect() pushed
the adjustment as CIccPcsSteps that Optimize() could fold. At a chain edge,
nothing cleared those flags, so the device xform did the work itself inside Apply()
and the code in ConnectFirst()/ConnectLast() written to handle it was unreachable
dead code, guarded on predicates that nothing ever satisfied.

Applying defaultcmyk.icc forward and back at perceptual intent showed the symptom: the
two adjustments should cancel exactly, and did on the interior path, but not at the edges.

What changed

  • CheckPCSConnections() hands the adjustment to CIccPcsXform at both chain edges,
    not just at interior connections. The leading-edge condition gained the
    NeedAdjustPCS() term the trailing-edge one already had.
  • Per-side virtual predicates NeedsSrcPcsAdjust() / NeedsDstPcsAdjust() answer at
    Begin() time what used to be decided inside Apply(), with overrides in
    CIccXformMpe and CIccXformNamedColor reproducing their own Apply() guards.
  • Spectral PCS ports convert against the spectral white point
    (relative = absolute / white, absolute = relative × white) instead of receiving the
    XYZ affine. Applies to reflectance, transmission and radiant PCS; bidirectional
    reflectance and sparse-matrix are excluded because their sample vectors are not
    spectra. See docs/superpowers/plans/2026-08-26-spectral-pcs-white-point-conversion.md.
  • CheckSrcAbs(), CheckDstAbs(), AdjustPCS() and the m_AbsLab scratch buffer are
    deleted, along with the m_bSrcPcsConversion / m_bDstPcsConversion flags and the
    NeedAdjustSrcPCS() / NeedAdjustDstPCS() predicates.
  • Design record: docs/pcs-adjustment-placement.md.

Two latent bugs fixed, both in code that had never executed

  1. ConnectLast() halved XYZ values. Its adjustment branch assumed an actual-XYZ
    pixel where an XYZ-PCS xform emits internal XYZ, so the unconditional
    pushXyzToXyzIn() rescaled by 32768/65535 twice. Wrong since it was written; invisible
    because the branch was unreachable. Confirmed fixed arithmetically — the XYZ-PCS
    absolute/relative ratio is now exactly mediaX/illumX.
  2. The spectral interior connection dropped its from-side adjustment. Connect()'s
    six spectral branches pushed only the to-side, while the interior loop cleared both
    flags. Now both sides convert.

Behaviour changes

Every item here is intentional and documented; none is a silent side effect.

Change Effect
Colorimetric PCS-edged chains results move ~1e-7 relative from re-associating the same affine math (~4e-6 absolute in Lab, measured on defaultcmyk.icc)
Spectral chain edge (#2331) was an XYZ affine over samples 0-2; now nothing. Not yet the conversion it should get — see the issue
Spectral interior connection from-side was dropped; now converts
MCS port via a BPC hint (#2330) was an XYZ affine over channels 0-2; now nothing
XYZ-PCS NegClip the pushed chain is pure affine and does not clamp. Reachable only through an IIccAdjustPCSXform hint with Scale > 1
bUsePCSConversions == true previously selected the in-Apply() path at interior connections; that path no longer exists. No in-tree caller passes true
CheckSrcAbs() / CheckDstAbs() / AdjustPCS() deleted an out-of-tree CIccXform subclass calling them now fails to compile. Deliberate: retaining them would have let such a subclass double-apply the adjustment silently, since m_bAdjustPCS reads true and the suppressing flags are gone

Verification

.github/ci/regression/pcs-adjust-placement.cpp is new: 176 assertions covering
interior connections, both chain edges, the per-side predicates, the spectral conversion
in all four intent/tag combinations, the excluded PCS types, and both issue reproductions.

Numeric assertions use a 1e-5 relative band throughout, never bit-equality. The
adjustments under test are ~3.5e-3 or larger; re-associating the same affine math moves
results ~1e-7. That gap is the oracle, and it separates "applied exactly once" from
"dropped, doubled, or inverted" while tolerating legitimate float reassociation.

Before deleting the in-Apply() path, its reachability was measured rather than argued:
the two guarded branches were instrumented and the whole suite run for 0 firings, with
a positive control — commenting out the four handover calls produced 64 firings, split
59 destination / 5 source — to prove the instrument fires when the path is live.

BPC black-point deltas were measured against a master baseline binary and are
byte-identical across both BPC intents, with a runtime probe confirming the changed math
actually executes in those chains rather than the null being an artefact. Recorded in
docs/superpowers/plans/2026-08-26-pcs-adjust-bpc-deltas.md, which also carries the
base...HEAD contract matrix.

Checklist

  • Signed all Commits in PR — not done, see Notes
  • Built locally according to docs/build.md
  • Followed the guidelines in Contributing document
  • Ran relevant CTest/profile tests from docs/ctest.md — 76/79; the three not run have no binaries in this configuration, unchanged against a master baseline worktree at the same configuration
  • Updated documentation for user-visible behavior changes
  • Ran sanitizer coverage for memory-safety or parser changes — not done, see Notes
  • Added or updated regression coverage for behavior changes
  • Attached a base...HEAD contract matrix for cross-cutting changes
  • Reviewed active and suppressed automated findings from review threads and summaries
  • For Python package changes, followed docs/python-packaging-release.md — n/a
  • Did not change maintainer-owned workflow, CTest, CPack, sanitizer, release, or security infrastructure unless requested by an iccDEV maintainer
  • New source files include the ICC copyright and BSD 3-Clause license header — one new file, see Notes
  • Code style matches nearby code: 2-space indent, K&R braces, m_ members

Notes for review

Three checklist items need a maintainer's call rather than a tick:

  1. Commits are not signed, by decision. All 33 are unsigned: the machine this was
    developed on has no signing key configured, so nothing was skipped — there was
    nothing to sign with. Signing them retroactively means rewriting the branch, which
    would invalidate roughly fifteen commit SHAs cited across the resumption record, the
    contract matrix, the design note and this description — a large part of what makes
    this work auditable. Nothing in CI verifies signatures. Weighed and left as-is; ask
    if maintainers want the rewrite anyway.
  2. Sanitizer coverage was not run. clang++ is not available on the machine this was
    developed on, so the ASAN/UBSAN build in docs/build.md never executed. Given the
    branch deletes code on the per-pixel apply path and adds a cached member, a sanitizer
    run before merge is worth having. Doxygen ran clean for the changed documents, but with
    HAVE_DOT=NO because Graphviz is absent, so CI's exact warning set was not reproduced.
  3. The new regression helper carries the full ICC Software License block, matching
    IccProfLib/IccApplyBPC.cpp. Most existing files under .github/ci/regression/ carry
    no licence header at all, so this is stricter than local convention rather than looser —
    flagging it in case the convention is deliberate.

Also worth a maintainer's opinion: five planning documents under
docs/superpowers/plans/ are included. docs/pcs-adjustment-placement.md is genuine
project documentation and belongs in the tree, and the BPC delta document carries the
contract matrix the checklist requires. The remaining specs and plans are development
artefacts; happy to drop them from the branch if they do not belong upstream.

Metadata

Metadata

Assignees

Labels

BuildBuild system, CMake, compiler, or packagingDocumentationDocumentation-only or documentation-related changePerfMaintainer indicates Performance Subject MatterPythonPython, Cython, PyPI, pip, or packaging scopeQAMaintainer indicates topic of Quality AssuranceQuestionQuestion or support requestTestingCTest, regression, or test coveragebugConfirmed or suspected defectciContinuous integration workflow changesfeatureFeature request or enhancementneeds-triageMaintainer triage requiredsecuritySecurity, sanitizer, or fuzzer-relevant report

Type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions