fix(#2001): allow cicpTag as a common optional tag in iccPawgReport C5 - #2004
Merged
Conversation
colourbill-ctrl
requested review from
ChrisCoxArt,
dwtza,
maxderhak and
xsscx
as code owners
August 7, 2026 04:34
xsscx
enabled auto-merge (squash)
August 7, 2026 10:17
Check C5 asks whether a profile is free of additional tags not required for its class other than allowed optional tags. icSigCicpTag was missing from kCommonOptional, so every profile carrying one drew: [WARN] C5 ... standard tags outside the local class rule table: 'cicp' The reason it reached the warning rather than the private-tag bucket is the non-obvious part, and it is what the added comment records: IsSpecTag() asks CIccInfo::GetTagSigName, which resolves 'cicp' to "cicpTag" through CIccTagCreator (IccTagFactory.cpp:138). The name does not begin with "Unknown", so the tag is never classified as private and falls straight through IsAllowedForClass() to the C5 warning. A conforming profile was reported as non-conforming. This is not HDR-specific. cicpTag is a v4.4 tag and the check does not look at the transfer characteristic, so any profile author who adds one to an ordinary display profile gets the spurious warning. The regression test below is built on a plain SDR profile for exactly that reason. Adds .github/ci/regression/pawg-c5-cicp-optional.cpp and the iccdev.pawg-c5-cicp-optional CTest. It compiles PawgReport.cpp into the test the same way the #1775 compression test does, then drives DumpPawgReport and reads the C5 line back -- C5's verdict is not exposed through PawgReport.h, and the emitted text is what the issue was reported against. Structured A/B against one base profile: control Testing/sRGB_v4_ICC_preference.icc as tracked -> C5 OK subject the same profile plus a BT.709 cicpTag -> C5 OK Pre-fix only the subject fails, naming 'cicp'; the control passes both ways, which is what makes a green subject mean something. The first base profile tried, Testing/ApplyDataFiles/test-profiles/sRGB_D65_MAT.icc, was rejected precisely because its control failed -- it already warns on 'c2sp', 's2cp', 'svcn', 'gbd1'. Those four tags are NOT addressed here. Whether they belong in kCommonOptional or in a class rule table is a separate judgement from the one #2001 reports, and widening this change to cover them would put an unreviewed table edit alongside a one-line fix. The test writes its own diagnostics to stderr: capturing the report means redirecting stdout, and there is no portable way to restore it on a CI runner with no /dev/tty or CONOUT$. Fixes #2001
xsscx
force-pushed
the
fix/2001-pawg-c5-cicp
branch
from
August 7, 2026 10:55
200a3f2 to
bd7c0f9
Compare
Member
PR Status2026-08-07 10:58:29 UTC
Success! Reminder - Please use Stacked PRs https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests |
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.
Fixes #2001.
What was wrong
Check C5 ("Is the profile free of additional tags not required for profile class, other
than allowed optional tags") warned on any profile carrying a
cicpTag:icSigCicpTagwas missing fromkCommonOptionalinPawgReport.cpp.Why it reached the warning rather than the private-tag bucket
The non-obvious part, and what the added comment records.
IsSpecTag()asksCIccInfo::GetTagSigName, which resolves'cicp'to"cicpTag"throughCIccTagCreator(
IccTagFactory.cpp:138). The name does not begin with"Unknown", so the tag is neverclassified as private and falls straight through
IsAllowedForClass()to the C5 warning. Aconforming profile was reported as non-conforming.
Not HDR-specific
cicpTagis a v4.4 tag and C5 does not look at the transfer characteristic, so this hits anyprofile author who adds one to an ordinary display profile. The regression test is built on a
plain SDR profile for exactly that reason.
Test
Adds
.github/ci/regression/pawg-c5-cicp-optional.cppand theiccdev.pawg-c5-cicp-optionalCTest. It compiles
PawgReport.cppinto the test the same way the #1775 compression test does,then drives
DumpPawgReportand reads the C5 line back — C5's verdict is not exposed throughPawgReport.h, and the emitted text is what the issue was reported against.A/B against one base profile:
Testing/sRGB_v4_ICC_preference.iccas trackedcicpTag(1/1/1/0)Pre-fix only the subject fails, naming
'cicp'; the control passes both ways, which iswhat makes a green subject mean something.
The corpus offered nothing to reuse as the subject: of the nine tracked XML carrying a
cicpTag, eight are underTesting/HDR/and the ninth(
.github/ci/test-data/ub-cicp-colorprimaries-1346.xml) is a deliberately malformed UB fixturethat
iccFromXmlrefuses outright, so it cannot be turned into a profile to assess. Hence thesynthesised subject.
The test writes its own diagnostics to stderr: capturing the report means redirecting
stdout, and there is no portable way to restore it on a CI runner with no
/dev/ttyorCONOUT$.Scoped out on purpose — a related gap this did not fix
The first base profile I tried,
Testing/ApplyDataFiles/test-profiles/sRGB_D65_MAT.icc, was rejected as a control because italready warns on C5 — for
'c2sp','s2cp','svcn','gbd1'. Socicpis not the onlytag missing from these tables.
Those four are deliberately not addressed here: whether they belong in
kCommonOptionalorin a per-class rule table is a separate judgement from the one #2001 reports, and folding an
unreviewed table edit into a one-line fix seemed like the wrong trade. Happy to file that
separately if it's wanted.
Pre-flight
-Wall -Wextra -Wpedantic -Werror("strict warnings ENABLED")grep -cE 'warning:'= 0, errors 0-DENABLE_LTO=ON("strict warnings ENABLED")grep -cE 'warning:'= 0, errors 0; new CTest passes there tooctesttool-coverageCMM profile-chain errors,spectral-tiff-previewmissingimagecodecs)ASAN_OPTIONS=detect_leaks=1news a tag and reads a profile, so this was checked rather than assumedNo shell or workflow files changed, so the shellcheck/actionlint pre-flight gates are not
engaged by this diff.