Add IccProfilePlot: data-first profile visualization API + CLI#1517
Open
colourbill-ctrl wants to merge 9 commits into
Open
Add IccProfilePlot: data-first profile visualization API + CLI#1517colourbill-ctrl wants to merge 9 commits into
colourbill-ctrl wants to merge 9 commits into
Conversation
e0c04a6 to
cd3895b
Compare
Member
PR Status2026-06-22 19:34:57 UTC @colourbill-ctrl I will be making changes to the Failing Dockerfile at 2026-06-22 19:38:00 UTC and likely aquick Rebase on master with a clean linear commit history.
|
Member
PR Status2026-06-22 19:56:44 UTC
|
colourbill-ctrl
added a commit
that referenced
this pull request
Jun 23, 2026
Clears the error/security-level CodeQL alerts on the new IccProfilePlot tree (PR #1517): * iccProfilePlot.cpp raster dump: restrict the output file to 0644 via fchmod instead of inheriting a permissive umask (no longer group/world-writable, CWE-732), and check the fwrite/fclose result so a short or failed write is reported as an error rather than a good dump (CWE-252). * MiniTIFF.cpp WriteTIFF: reject non-finite/negative dpi before the float->uint32 cast of the resolution numerator, so the cast is always well-defined (CWE-369/float-to-int). * IccVizModel.cpp buildCurveGraph: defensive floor on the sample count that drives i/(float)steps (divide-by-zero guard). * IccVizModel.cpp enumerateLutCurves: clamp the profile-supplied per-group channel count into a local bound before the loop, so a malformed LUT cannot drive an unbounded iteration (CWE-400/CWE-834). * iccProfileVisualize.cpp: document the two fixed chart-layout scale constants as non-zero compile-time constants (they are divisors but can never be zero) — the divide-by-zero query is taught to skip such constants in the companion commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 23, 2026
colourbill-ctrl
added a commit
that referenced
this pull request
Jun 23, 2026
CodeQL) Clears iccdev/describe-without-validate on IccVizModel.cpp: describeCurve() formatted non-CIccTagCurve curves via Describe() (which walks the curve data) while the module's validation gate (curveValidate) lived in a sibling function outside the query's window. Add the curve's own Validate() immediately before Describe() so the formatter never touches unvalidated, possibly-malformed data first (CWE-476). Status is advisory -- per this module's design a malformed curve is still described (and surfaced as a diagnostic upstream), not dropped. Output is unchanged; the LUT/gamma fast-paths above are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
colourbill-ctrl
added a commit
that referenced
this pull request
Jun 23, 2026
xsscx's PR #1517 QA report (#1550) found both tools returning exit 0 on real failures, so scripts and CI silently treat them as success: - Finding 1: iccProfileVisualizePlot returned 0 even when a profile failed to parse, threw, or produced no output (the upstream reference tool returns 0 unconditionally). Track a per-file status and return nonzero if any input fails. Deliberate divergence from the reference. - Finding 2: iccProfilePlot raster wrote an "error" field into its JSON but still exited 0 when the optional out.raw could not be opened or fully written. printRaster() now reports success and main() exits 4. - Finding 3: README implied chroma:xy (and other ids) are always present; note that enumerated ids are profile-dependent. CTest: iccdev_add_iccprofileplot_exitcode_test() registers two WILL_FAIL regressions (viz on a non-ICC input; raster with the out.raw path pointing at a directory, which icOpenRegularWriteBinaryFile rejects). Both reproduce the exit-code contract portably; verified red-green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closed
Clears the error/security-level CodeQL alerts on the new IccProfilePlot tree (PR #1517): * iccProfilePlot.cpp raster dump: restrict the output file to 0644 via fchmod instead of inheriting a permissive umask (no longer group/world-writable, CWE-732), and check the fwrite/fclose result so a short or failed write is reported as an error rather than a good dump (CWE-252). * MiniTIFF.cpp WriteTIFF: reject non-finite/negative dpi before the float->uint32 cast of the resolution numerator, so the cast is always well-defined (CWE-369/float-to-int). * IccVizModel.cpp buildCurveGraph: defensive floor on the sample count that drives i/(float)steps (divide-by-zero guard). * IccVizModel.cpp enumerateLutCurves: clamp the profile-supplied per-group channel count into a local bound before the loop, so a malformed LUT cannot drive an unbounded iteration (CWE-400/CWE-834). * iccProfileVisualize.cpp: document the two fixed chart-layout scale constants as non-zero compile-time constants (they are divisors but can never be zero) — the divide-by-zero query is taught to skip such constants in the companion commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ro-profile The division-by-zero-profile query matches denominators by name (...Scale, ...Range, ...Steps, etc.). A file-scope `const float kScale = 0.85f;` matches purely on its name yet can never be zero or non-finite at run time, producing false positives (e.g. the IccProfilePlot chromaticity/ab chart scale constants). Add an isCompileTimeNonzeroConstant() exemption: skip a divisor that is a read of a const/constexpr variable whose initializer folds to a non-zero compile-time constant. A non-constant const initialised from a profile field has no getValue() and is still reported, so genuine profile-derived divisors remain covered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1547) buildClutRaster() (IccVizModel.cpp) derived its tile-packing geometry from the CLUT grid arrangement, not from the CLUT's actual sample array, so a malformed / non-square CLUT drove the computed input index past the end of clut->GetData() -- a 4-byte heap-buffer-overflow read (CWE-125) that ASan trapped both while Enumerate() probed the tag and on an explicit `raster` render. Bound every read against the true element count (NumPoints() nodes x outputChannels) and skip out-of-range nodes, leaving the pre-zeroed sample. PDFWriter::CloseFile() (MiniPDF.cpp) cleared its owning m_objects vector on the success path -- and early-returned on the write-failure path -- BEFORE the unified delete loop at the end of the function, leaking every PDFObject including the per-page PDFPage allocations from AddPage() (the 3456-byte / 36-object direct leak in the CI report). Route both paths through the single cleanup loop so all objects are freed. Add two CLI-driven CTest regressions (the bugs live in tool sources, so the tests drive the real binaries): iccdev.iccprofileplot-clut-oob renders the A2B0 CLUT of a minimized fuzz profile through iccProfilePlot (ASan guard), and iccdev.iccprofileplot-pdf-leak runs iccProfileVisualizePlot over the stock sRGB v4 profile under forced detect_leaks=1 (LSan guard). Both are red-green verified against the pre-fix sources. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Diffing buildClutRaster() against the reference iccProfileVisualize layout revealed a port transcription error: the per-row input stride n010 was tileWidth*outputChannels but must be tileHeight*outputChannels (x indexes the tileWidth dimension, so one x-step skips a full column of tileHeight samples). The two coincide only for square CLUTs; for a non-square CLUT the wrong stride over-indexes and walks clutData off the end -- the actual cause of the #1548 heap-buffer-overflow read, not merely "malformed input". Restore the reference stride. The defensive NumPoints()-based bound added in 06dd7cd is retained as defense-in-depth for genuinely malformed grid metadata; with the stride corrected it is a no-op on well-formed (incl. non-square) CLUTs. Verified: the fuzz profile now renders both A2B0/A2B1 rasters cleanly under ASan with the bound disabled, and the A2B0 (68x65, non-square) render changes by 21KB vs the bound-only version -- i.e. the bound-only fix had been silently dropping valid samples. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eps)
Two code-scanning alerts on this PR fail the required check:
* high cpp/world-writable-file-creation (iccProfilePlot.cpp): the raster dump
used a bare fopen(outFile,"wb"); a post-open fchmod does not satisfy the
standard query (the file briefly exists with the default umask). Route the
dump through icOpenRegularWriteBinaryFile() -- the same helper the Mini{TIFF,
PDF} writers use -- which creates via open(O_CREAT, 0644) after a regular-file
check, so the file is never world-writable and not a symlink-followed special
file (CWE-732, CWE-59). Also clears the custom iccdev/world-writable-output.
* error iccdev/division-by-zero-profile (IccVizModel.cpp): the existing steps
floor `if (steps < 1)` was not recognised as a zero guard (the query credits a
comparison against 0, not 1). Write it as `if (steps <= 0)` -- identical for an
int -- so the non-zero invariant on the i/(float)steps denominator is explicit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeQL) Clears iccdev/describe-without-validate on IccVizModel.cpp: describeCurve() formatted non-CIccTagCurve curves via Describe() (which walks the curve data) while the module's validation gate (curveValidate) lived in a sibling function outside the query's window. Add the curve's own Validate() immediately before Describe() so the formatter never touches unvalidated, possibly-malformed data first (CWE-476). Status is advisory -- per this module's design a malformed curve is still described (and surfaced as a diagnostic upstream), not dropped. Output is unchanged; the LUT/gamma fast-paths above are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
xsscx's PR #1517 QA report (#1550) found both tools returning exit 0 on real failures, so scripts and CI silently treat them as success: - Finding 1: iccProfileVisualizePlot returned 0 even when a profile failed to parse, threw, or produced no output (the upstream reference tool returns 0 unconditionally). Track a per-file status and return nonzero if any input fails. Deliberate divergence from the reference. - Finding 2: iccProfilePlot raster wrote an "error" field into its JSON but still exited 0 when the optional out.raw could not be opened or fully written. printRaster() now reports success and main() exits 4. - Finding 3: README implied chroma:xy (and other ids) are always present; note that enumerated ids are profile-dependent. CTest: iccdev_add_iccprofileplot_exitcode_test() registers two WILL_FAIL regressions (viz on a non-ICC input; raster with the out.raw path pointing at a directory, which icOpenRegularWriteBinaryFile rejects). Both reproduce the exit-code contract portably; verified red-green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What this is
A new, standalone command-line tool —
iccProfilePlot— underTools/CmdLine/IccProfilePlot/, built on a new data-first visualization model,IccVizModel. WhereiccProfileVisualizerenders a finished PDF/TIFF report,IccVizModelreturns the underlying data for each visualization:A caller then draws, colour-manages, or exposes individual graphs and their data however it likes — into a PDF, an SVG, JSON, or an interactive UI.
Developed in parallel to
iccProfileVisualize, on purposeThis is effectively a major refactor of the
iccProfileVisualizevisualization code toward a data-first API. For regression and safety, that refactor is being carried out outside theiccProfileVisualizecodebase rather than in place —iccProfileVisualizeis left completely untouched, so there is no risk to the actively-developed tool, and the two can be diffed and validated independently. The intent is a parallel track that can later converge, not an in-place rewrite.Faithful to the original where it counts
As much as possible, the original guard rails, commenting, and structure have been preserved, so the two read alike:
Skipping <tag>: invalid CLUT width,WARNING - tile count overflow., …).A silent mode lets the caller turn the CLI/stderr diagnostic echo on or off to suit its context: by default the model echoes diagnostics to stderr exactly like
iccProfileVisualize(so the CLI behaves identically out of the box); a library embedding (e.g. WASM/browser) callsSetSilent(true)to suppress it, while still receiving every reason as structured data. A per-callVerbositycan override the global switch either way.The API
See the usage guide at the top of
IccVizModel.hpp, echoed as a worked example iniccProfilePlot.cpp:Pick a visualization by
Kind, supply aCIccProfile*, and render the one you want as graph data or raster samples.Scope note: the demo tool does not emit PDF (yet)
iccProfilePlotis a demonstration consumer of the API — it lists visualizations and serializes the returned graph/raster data (JSON / raw samples). It deliberately does not emit a PDF at this point; producing a finished report would require drawing the returned data into MiniPDF (the rendering layericcProfileVisualizeuses), which is intentionally left out so the model and this tool stay dependency-light (IccProfLibonly). The commented walkthrough iniccProfilePlot.cppshows exactly where a PDF generator would slot in.Build / test
Build/Cmake/Tools/IccProfilePlot/CMakeLists.txt, registered in the top-level CMake; builds clean againstIccProfLibvia the standard CMake build.iccProfilePlot <profile> list | graph <id> | raster <id> [out.raw].🤖 Generated with Claude Code
Update — second commit (
bbd8109): a PDF/TIFF report tool on the engineThe scope note above ("the demo tool does not emit PDF yet") is now addressed: a
second tool draws finished artifacts through the engine.
iccProfileVisualize(in the sameTools/CmdLine/IccProfilePlot/dir — a parallel,data-first reimplementation; the standalone tool of the same name is left untouched)
reproduces that tool's PDF (tone curves, chromaticity, named-colour scatters) and
per-tag TIFF output. Instead of walking the profile and computing geometry inline,
it enumerates and renders through
IccVizModeland draws the returned data withthe dependency-free Mini{PDF,SVG,TIFF} writers. The engine is the single source of
the maths; the tool is just a drawing front-end.
Changes (all under
Tools/CmdLine/IccProfilePlot/, plus the tool's CMake target)iccProfileVisualize.cpp—processLuts()callsiccviz::Enumerate(),then per descriptor
RenderGraph/RenderRaster, drawing each with the Miniwriters (
renderChromaticityGraph,renderCurveGraph, the reusedgraphNamedColorsPDF, andWriteTIFFstraight from the raster). The originaldrawing primitives and reference-geometry XObjects are preserved verbatim; the
now-superseded inline extractors were removed (lineage noted in comments).
MiniPDF/MiniSVG/MiniTIFF— the dependency-free writers carriedover from the standalone tool. No third-party libraries.
IccVizModel.{hpp,cpp}—Enumerate(pIcc, Order order = Order::Canonical):Canonicalis the existing fixed, cross-module/WASM-safe order; the opt-inOrder::TagTablereorders the same descriptors to the profile's tag-table pagesequence so a report matches the original. Also v5 named colours in
collectNamedColors(icSigTagArrayType: NamedColorArray / ColorantInfoArray —PCS float arrays → Lab, names via UTF8/UTF16/Text/MLU, tint-% suffixes), with the
PCS basis aligned to the profile header PCS.
Build/Cmake/Tools/IccProfilePlot/CMakeLists.txt— registers aniccProfileVisualizePlotexecutable (distinct output name so it doesn't clashwith the standalone
iccProfileVisualizeon install). Verified to configure andbuild in-tree.
Testing
Built both tools against the same
IccProfLiband ran them over a 90-profile corpus(RGB matrix, CMYK / N-colour CLUT, named/spot, wide-gamut, iccMAX), comparing every
artifact byte-for-byte against the original standalone
iccProfileVisualize:Order::TagTable)IccVizModel.cppcompiles clean under-Wall -Wextra. One intentional difference: apresent-but-malformed curve (e.g.
bad-bluecurve.icc) is enumerated and rendered witha diagnostic, where the original silently dropped the page — consistent with IccViz's
"surface the reason" design.