From bd7c0f98bc92ff4780ef2db1e508e2512fb08f24 Mon Sep 17 00:00:00 2001 From: colourbill-ctrl Date: Thu, 6 Aug 2026 21:33:57 -0700 Subject: [PATCH] fix(#2001): allow cicpTag as a common optional tag in iccPawgReport C5 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 --- .../ci/regression/pawg-c5-cicp-optional.cpp | 195 ++++++++++++++++++ Build/Cmake/Testing/CMakeLists.txt | 56 +++++ Tools/CmdLine/IccPawgReport/PawgReport.cpp | 8 + 3 files changed, 259 insertions(+) create mode 100644 .github/ci/regression/pawg-c5-cicp-optional.cpp diff --git a/.github/ci/regression/pawg-c5-cicp-optional.cpp b/.github/ci/regression/pawg-c5-cicp-optional.cpp new file mode 100644 index 000000000..d1a9c0fbe --- /dev/null +++ b/.github/ci/regression/pawg-c5-cicp-optional.cpp @@ -0,0 +1,195 @@ +// Copyright (c) 2026 The International Color Consortium. All rights reserved. +// Licensed under the BSD 3-Clause "New" or "Revised" License; see the ICC +// Software License in the repository root and CONTRIBUTING.md. +// +// Regression: iccPawgReport check C5 warned on any profile with a cicpTag (#2001). +// +// C5 asks "Is the profile free of additional tags not required for profile class +// (other than allowed optional tags)". icSigCicpTag was missing from +// kCommonOptional in PawgReport.cpp, 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 this test really guards. IsSpecTag() asks +// CIccInfo::GetTagSigName, which resolves 'cicp' to "cicpTag" through +// CIccTagCreator (IccTagFactory.cpp), so the name does not begin with "Unknown" +// and the tag is never classified as private -- it falls straight through +// IsAllowedForClass() to the C5 warning. A conforming profile was therefore +// reported as non-conforming. +// +// Deliberately built on a plain SDR profile. The base fixture is the tracked +// sRGB v4 ICC preference profile and the cicp values written into it are +// BT.709 primaries / BT.709 transfer / BT.709 matrix / limited range (1/1/1/0) -- +// no HDR transfer function, no HDR metadata. That matters because the defect was +// found while working on HDR profiles but is not HDR-specific: it hits any +// profile author who adds a cicpTag to an ordinary display profile. The corpus +// offers nothing suitable to reuse: of the nine tracked XML carrying a cicpTag, +// eight are under Testing/HDR/ and the ninth, +// .github/ci/test-data/ub-cicp-colorprimaries-1346.xml, is a deliberately +// malformed UB fixture that iccFromXml refuses outright, so it cannot be turned +// into a profile to assess. Hence the synthesised subject below. +// +// Structure is A/B against the same base profile: +// control -- base profile as tracked, no cicpTag -> C5 must be OK +// subject -- same profile + a cicpTag -> C5 must be OK +// The control is what proves the fixture was not already warning for some +// unrelated reason, so a green subject means something. +// +// This test writes its own output to stderr, because capturing the report means +// redirecting stdout with no portable way to restore it. +// +// C5's verdict is not exposed through PawgReport.h (only the compression verdict +// is, per #1775), so the report is captured from stdout and the C5 line read back. +// Asserting on the emitted text is arguably the better target anyway: the text is +// exactly what a user sees and what the issue was reported against. +// +// Exit code 0 = pass, 1 = a case regressed. +#include "PawgReport.h" + +#include "IccProfile.h" +#include "IccTagBasic.h" + +#include +#include + +static int g_failures = 0; + +// Runs DumpPawgReport with stdout redirected to a file, then returns the whole +// report as a string. +static bool captureReport(const char *szProfile, const char *szTmp, std::string &out) +{ + // stdout is redirected and never restored: there is no portable way back to + // the original stream (a CI runner has no /dev/tty and no CONOUT$), so this + // test writes all of its own diagnostics to stderr instead. Each capture + // simply re-points stdout at the next file. + std::fflush(stdout); + + if (!std::freopen(szTmp, "w", stdout)) + return false; + + DumpPawgReport(szProfile, false); + + std::fflush(stdout); + + FILE *f = std::fopen(szTmp, "rb"); + if (!f) + return false; + + char buf[4096]; + size_t n; + out.clear(); + while ((n = std::fread(buf, 1, sizeof(buf), f)) > 0) + out.append(buf, n); + std::fclose(f); + + return true; +} + +// The report prints one line per check; pull the C5 one plus its detail, which +// continues on the following indented line. +static std::string extractC5(const std::string &report) +{ + size_t pos = report.find(" C5 "); + if (pos == std::string::npos) + pos = report.find(" C5\t"); + if (pos == std::string::npos) + return std::string(); + + // Back up to the start of that line, then take this line and the next two so + // the wrapped detail ("standard tags outside ...") is included. + size_t start = report.rfind('\n', pos); + start = (start == std::string::npos) ? 0 : start + 1; + + size_t end = start; + for (int i = 0; i < 3; ++i) { + size_t nl = report.find('\n', end); + if (nl == std::string::npos) { end = report.size(); break; } + end = nl + 1; + } + + return report.substr(start, end - start); +} + +static void checkC5(const char *szCase, const char *szProfile, const char *szTmp) +{ + std::string report; + if (!captureReport(szProfile, szTmp, report)) { + std::fprintf(stderr, "FAIL [%s]: could not capture the report for '%s'\n", szCase, szProfile); + g_failures++; + return; + } + + std::string c5 = extractC5(report); + if (c5.empty()) { + std::fprintf(stderr, "FAIL [%s]: no C5 line in the report for '%s'\n", szCase, szProfile); + g_failures++; + return; + } + + bool bWarn = c5.find("[WARN]") != std::string::npos; + bool bMentionsCicp = c5.find("cicp") != std::string::npos; + + if (bWarn || bMentionsCicp) { + std::fprintf(stderr, "FAIL [%s]: C5 warned%s --\n%s\n", szCase, + bMentionsCicp ? " and named 'cicp'" : "", c5.c_str()); + g_failures++; + return; + } + + std::fprintf(stderr, "ok [%s]: C5 clean\n", szCase); +} + +int main(int argc, char *argv[]) +{ + if (argc < 2) { + std::fprintf(stderr, "usage: %s \n", argv[0]); + return 1; + } + + const char *szBase = argv[1]; + + // Control: the tracked profile exactly as it ships, with no cicpTag. If this + // ever warns, the fixture has changed and the subject case below proves + // nothing. + checkC5("control-no-cicp", szBase, "pawg-c5-control.txt"); + + // Subject: same profile with a plain BT.709 SDR cicpTag attached. + { + CIccProfile *pIcc = ReadIccProfile(szBase); + if (!pIcc) { + std::fprintf(stderr, "FAIL [subject-with-cicp]: could not read '%s'\n", szBase); + g_failures++; + } + else { + CIccTagCicp *pCicp = new CIccTagCicp(); + // BT.709 primaries / BT.709 transfer / BT.709 matrix / limited range. + pCicp->SetFields(1, 1, 1, 0); + + if (!pIcc->AttachTag(icSigCicpTag, pCicp)) { + std::fprintf(stderr, "FAIL [subject-with-cicp]: AttachTag(icSigCicpTag) failed\n"); + g_failures++; + delete pCicp; + } + else { + const char *szSubject = "pawg-c5-cicp-subject.icc"; + if (!SaveIccProfile(szSubject, pIcc)) { + std::fprintf(stderr, "FAIL [subject-with-cicp]: could not write '%s'\n", szSubject); + g_failures++; + } + else { + checkC5("subject-with-cicp", szSubject, "pawg-c5-subject.txt"); + } + } + delete pIcc; + } + } + + if (g_failures) { + std::fprintf(stderr, "%d case(s) regressed\n", g_failures); + return 1; + } + + std::fprintf(stderr, "all cases passed\n"); + return 0; +} diff --git a/Build/Cmake/Testing/CMakeLists.txt b/Build/Cmake/Testing/CMakeLists.txt index 599045228..ba29e0c48 100644 --- a/Build/Cmake/Testing/CMakeLists.txt +++ b/Build/Cmake/Testing/CMakeLists.txt @@ -1038,6 +1038,60 @@ function(iccdev_add_cicp_copy_ctor_test) endif() endfunction() +# #2001: iccPawgReport check C5 warned on every profile carrying a cicpTag, +# because icSigCicpTag was missing from kCommonOptional. The tag is recognised by +# IsSpecTag() (CIccTagCreator resolves 'cicp' to "cicpTag"), so it is never +# bucketed as a private tag and falls straight through IsAllowedForClass() to the +# warning. 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. A/B against one base profile -- the tracked +# plain-SDR sRGB v4 preference profile as the control, and the same +# profile plus a BT.709 cicpTag as the subject -- which is also what shows the +# defect is not HDR-specific. Needs IccProfLib only. +function(iccdev_add_pawg_c5_cicp_test) + if(NOT TARGET "${TARGET_LIB_ICCPROFLIB}") + return() + endif() + + iccdev_add_regression_executable(iccPawgC5CicpTest + "${ICCDEV_REPO_ROOT}/.github/ci/regression/pawg-c5-cicp-optional.cpp" + "${ICCDEV_REPO_ROOT}/Tools/CmdLine/IccPawgReport/PawgReport.cpp" + ) + target_compile_features(iccPawgC5CicpTest PRIVATE cxx_std_17) + target_include_directories(iccPawgC5CicpTest PRIVATE + "${ICCDEV_REPO_ROOT}/Tools/CmdLine/IccPawgReport" + ) + target_link_libraries(iccPawgC5CicpTest PRIVATE ${TARGET_LIB_ICCPROFLIB}) + add_dependencies(check iccPawgC5CicpTest) + + add_test( + NAME iccdev.pawg-c5-cicp-optional + COMMAND "$" + "${ICCDEV_TESTING_DIR}/sRGB_v4_ICC_preference.icc" + ) + # Runs in the ctest output dir because it writes the subject profile and the + # two captured reports beside itself. + set_tests_properties(iccdev.pawg-c5-cicp-optional PROPERTIES + WORKING_DIRECTORY "${ICCDEV_TEST_OUTDIR}" + TIMEOUT 60 + LABELS "iccdev;pawg;cicp;issue-2001;regression" + ) + if(WIN32) + set(_pawg_c5_cicp_windows_env_mods + "PATH=path_list_prepend:$" + "PATH=path_list_prepend:$" + ) + foreach(_runtime_path IN LISTS ICCDEV_WINDOWS_RUNTIME_PATHS) + list(APPEND _pawg_c5_cicp_windows_env_mods + "PATH=path_list_prepend:${_runtime_path}") + endforeach() + set_tests_properties(iccdev.pawg-c5-cicp-optional PROPERTIES + ENVIRONMENT_MODIFICATION "${_pawg_c5_cicp_windows_env_mods}" + ) + endif() +endfunction() + # #1976: two defects on the gamt path. CIccXform::Create() forces bInput false to walk # the B-to-A shaped gamut tag, but m_bInput also drove GetDstSpace()/GetNumDstSamples(), # so the xform advertised the profile's device space while CIccCmm::AddXform() had @@ -3618,6 +3672,7 @@ if(WIN32) iccdev_add_colorimetry_methods_test() iccdev_add_getvalues_nstart_test() iccdev_add_cicp_copy_ctor_test() + iccdev_add_pawg_c5_cicp_test() iccdev_add_gamut_xform_semantics_test() iccdev_add_rangemap_uninitialized_test() iccdev_add_pcs_birefl_illuminant_range_test() @@ -3877,6 +3932,7 @@ iccdev_add_reflectance_observer_illum_range_test() iccdev_add_colorimetry_methods_test() iccdev_add_getvalues_nstart_test() iccdev_add_cicp_copy_ctor_test() +iccdev_add_pawg_c5_cicp_test() iccdev_add_gamut_xform_semantics_test() iccdev_add_rangemap_uninitialized_test() iccdev_add_pcs_birefl_illuminant_range_test() diff --git a/Tools/CmdLine/IccPawgReport/PawgReport.cpp b/Tools/CmdLine/IccPawgReport/PawgReport.cpp index 77e1b480e..f2fb64df1 100644 --- a/Tools/CmdLine/IccPawgReport/PawgReport.cpp +++ b/Tools/CmdLine/IccPawgReport/PawgReport.cpp @@ -1104,6 +1104,14 @@ static const icTagSignature kNamedColorRequired[] = { static const icTagSignature kCommonOptional[] = { icSigCalibrationDateTimeTag, icSigCharTargetTag, + // #2000-adjacent, filed as #2001: cicpTag was missing here, so C5 warned on + // every profile that carries one. The reason it reached the warning rather + // than the private-tag bucket is not obvious from this array: IsSpecTag() + // asks CIccInfo::GetTagSigName, which resolves 'cicp' to "cicpTag" via + // CIccTagCreator, so the name does not begin with "Unknown" and the tag is + // never treated as private -- it falls straight through IsAllowedForClass() + // to the C5 warning. This affects ordinary SDR profiles, not just HDR ones. + icSigCicpTag, icSigChromaticAdaptationTag, icSigChromaticityTag, icSigColorantTableTag,