Fall back to the ICC alternate space when no resolver is configured - #8
sebgoubier wants to merge 1 commit into
Conversation
An ICCBased space raised unsupported-color whenever neither an iccTransformResolver nor an iccKernel was configured. The contract that error points at cannot be met from the public API: BuildHepFromPdfOptions has no field for either, and nothing in src/ supplies one. Every PDF carrying an ICCBased space therefore failed to convert through buildHep with no way to opt in to what the message asked for. ISO 32000-1 8.6.5.5 already defines the way out. The alternate space stands in when the profile cannot be used, it is always available -- /Alternate when supplied and the Device space matching /N otherwise -- and parseIcc already resolves it, checks its component count and keeps it as alternateSpaceIndex. Indexed, Separation and DeviceN all convert through that same index. Only the case where nothing asked for profile fidelity changes. A resolver that is present and then fails still raises, so the invalid-icc-transform-result paths and the kernel-error cases are untouched. /Range still maps components onto [0, 1] first, so the fallback and a resolver-backed profile see identical inputs. Three tests asserted the old refusal and are updated here rather than worked around. That is a deliberate behaviour change, described in the pull request together with the alternative of exposing the resolver through the public options instead. Refs soadzoor#2 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
To be completely honest, I'm not an expert on this, this is the first time I read about ICC profiles. To me, instead of an using an "alternate" way to open these floorplans with ICC profiles, it would make more sense to add support for proper icc-engines, like Mozilla's QCMS, or Little-CMS If they're not available for some reason, we can fallback to the alternate you're proposing here with a warning to the user (because the color's won't be accurate, but it's still better than simply NOT opening them). GPT also found some issues with the "alternate" way, quoted below:
I applied the above-mentioned changes on this commit: 3fe2ef0 Please let me know whether it works for you, and feel free to reopen if needed! |
2 participants
The question first
This one changes behaviour you deliberately test for, so it is a proposal rather than a plain bug fix, and I have made the smaller of the two possible changes.
Three tests currently assert that an ICCBased space without a resolver raises
unsupported-color:test-native-color-semantics.mjs,test-native-pdf-image-color.mjs, andtest-pdf-session-color.mjs. I changed all three. That is not something I would do quietly, so here is the reasoning.What led me here
Real drawings fail to convert with:
The contract that message points at cannot be satisfied from the public API.
BuildHepFromPdfOptionshas no field for an ICC resolver or kernel, and nothing insrc/supplies one. So for anyone converting throughbuildHeporPDFtoHEP.js, every PDF carrying an ICCBased space fails, with no way to opt in to the thing the error asks for.Meanwhile ISO 32000-1 8.6.5.5 already specifies the way out: the alternate space stands in when the profile itself cannot be used. It is always available —
/Alternatewhen the space supplies one, and the Device space matching/Notherwise — andparseIccBasedalready resolves it, validates its component count, and stores it asalternateSpaceIndex.Indexed,SeparationandDeviceNall already convert through exactly that index.What this changes
Only the case where nothing has asked for profile fidelity: no
iccTransformResolver, noiccKernel. Conversion then goes through the alternate space, as 8.6.5.5 says it should.A resolver that is present and fails still raises, unchanged — the
invalid-icc-transform-resultcases intest-pdf-session-icc-transform-resolver.mjspass untouched, and so does the kernel-error path intestIccProfilesAndKernelBoundary./Rangestill maps components onto [0, 1] before the alternate space sees them, so a resolver-backed profile and this fallback receive identical inputs. That is asserted.The alternative
If you would rather keep the strict contract, the other fix is to expose
iccTransformResolver(andiccKernel) throughBuildHepFromPdfOptionsso callers can actually satisfy it. I am happy to write that instead, or to put this fallback behind an option that defaults to today's behaviour. Tell me which you prefer and I will redo it.Checks
test-native-color-semantics.mjs,test-native-pdf-image-color.mjs,test-pdf-session-color.mjs,test-pdf-session-icc-transform-resolver.mjs— all passnpm test—tsc --noEmitclean, 36 / 37 fast files passnpm run test:integration— 41 / 41npm run test:unit— 66 / 68The two failing files (
test-text-lod-core.mjs,test-room-segment-extractor.mjs) also fail onmain; they are Windows-only path failures.Refs #2