fix(rendering): evaluate Separation/DeviceN tint transforms (full-tint spot colour renders black)#630
Conversation
A `Separation`/`DeviceN` spot colour set at full tint with `scn` rendered as solid black instead of being resolved through its tint transform — blacking out tinted callout boxes and headings on InDesign-exported PDFs. In `page_renderer.rs` the `scn` (`SetFillColorN`) Separation arm never evaluated the tint transform (always `grey = 1 - tint`, so full tint -> black), and the `sc` (`SetFillColor`) arm only handled `FunctionType 2` with a `DeviceCMYK` alternate. Both arms now evaluate the tint transform via a shared `eval_separation_rgb` helper supporting `FunctionType 0` (sampled) and `FunctionType 2` (exponential), mapping the alternate-space output to RGB by component count (1 -> Gray, 3 -> RGB, 4 -> CMYK), and fall back to the grey approximation only when the function can't be evaluated. Adds `tests/separation_color.rs` (renders committed reproducer PDFs and asserts the full-tint fill is light/green-dominant, not black) for both function types, plus `examples/separation-blackout/` with the reproducer PDFs, before/after renders, and `examples/render_separation_demo.rs`.
yfedoseev
left a comment
There was a problem hiding this comment.
Thank you so much for this, @apptekstudios — excellent contribution! 🙏 Resolving the spot colour through its tint transform instead of the grey = 1 - tint approximation is exactly right (§8.6.6.4 — tintTransform shall be a function, §7.10), and you've covered both common function types: Type 2 (exponential) and Type 0 (sampled, with correct MSB-first sample packing per §7.10.2). Committing a minimal reproducer for each function type with before/after PNGs is fantastic and makes the fix easy to trust.
A few notes, with one I'd gently suggest addressing before merge and two as optional follow-ups:
1. (pre-merge) Multi-colorant DeviceN (§8.6.6.5). eval_separation_rgb reads tints.first() and eval_pdf_function_1d is single-input. The spec's own DeviceN example (§8.6.6.5) is a hexachrome space needing a 6-in, 4-out tint transform — i.e. a DeviceN space with more than one colorant has a multi-input function, so evaluating only the first tint would mis-render it. Since the title covers Separation/DeviceN, it'd be great to either handle n-input functions, or scope to Separation + single-colorant DeviceN and keep the grey fallback for true multi-colorant DeviceN (so it degrades safely rather than rendering a wrong colour). Everything else is solid — this is the only one I'd flag as worth tightening first.
2. (follow-up) Type 4 tint transforms. tintTransform may be any function type (§8.6.6.4 → §7.10), and Type 4 (PostScript calculator) is fairly common for spot colours; right now those fall back to grey. Good news: v0.3.59 shipped a Type 4 evaluator (#603) that's noted as "not yet wired into the Separation/DeviceN tint path" — so wiring it here (or leaving it as that tracked follow-up) would close the loop nicely. Totally optional for this PR.
3. (minor) components_to_rgb maps by component count. 3→RGB / 4→CMYK neatly sidesteps parsing the alternate space, which is pragmatic — just noting that a 3-component Lab or ICCBased(N=3, non-RGB) alternate would be interpreted as RGB. Low-frequency, so a short scoping comment is plenty.
Thanks again — this is a well-built, well-tested fix that solves a real and visible problem. 🎨
Extend Separation/DeviceN tint-transform resolution beyond the single-input FunctionType 0/2 + count-based mapping from the previous commit: - Evaluate FunctionType 4 (PostScript calculator) tint transforms, which also makes true multi-colorant DeviceN (n inputs -> m outputs, ISO 32000-1 8.6.6.5) render correctly. Multi-input FunctionType 0/2 and other unevaluable shapes still degrade safely to grey rather than rendering a wrong colour. - Make colour resolution colorant-count aware so a multi-colorant DeviceN is never fed into a single-input evaluator against only its first tint. - Inspect the alternate colour space (arr[2]) instead of guessing from the output arity: Device*/Cal*/ICCBased-by-N map directly, and Lab (8.6.5.4) is converted via CIELAB->sRGB rather than being misread as DeviceRGB. Move the resolution pipeline out of page_renderer into a focused `tint` module (gated with `rendering` alongside its only consumer), de-duplicating cmyk_to_rgb and the colour-space match. Add reproducer PDFs with before/after PNGs and regression tests for the Type 4 Separation, multi-colorant DeviceN, and Lab-alternate cases.
yfedoseev
left a comment
There was a problem hiding this comment.
Following up — no new commit here yet (the CFF #629 and separation-image #631 PRs have iterated). Just flagging that the one pre-merge item from my earlier review is still open:
- Multi-colorant DeviceN (§8.6.6.5):
eval_separation_rgbusestints.first()andeval_pdf_function_1dis single-input, so a DeviceN space with >1 colorant (the spec's hexachrome example needs a 6-in/4-out tint transform) would be mis-evaluated. Either handle n-input functions, or scope to Separation + single-colorant DeviceN and keep the grey fallback otherwise.
Everything else looked great. Happy to re-review as soon as you push. 🙏
|
I’ve tried to add the variant support and proper color space handling. I’ve noted same issue with text from some InDesign pdfs, so have moved the tint functions into a file preparing for re-use there. Not applying to text yet as just need to research further to ensure resolving these funcs doesn’t occur when parsing text alone to ensure not slowing down those operations. |
Collapse the duplicated sc/SC/scn/SCN colour handling into a single resolve_color_to_rgb path and rename tint.rs -> rendering/color_resolve.rs. Fixes stroke Separation/DeviceN (SC/SCN) falling back to grey and replaces the Indexed grey guess with a real palette lookup.
yfedoseev
left a comment
There was a problem hiding this comment.
Following up on my earlier review — thanks for the iterations here, the new commits resolve everything I'd flagged:
- Multi-colorant DeviceN (§8.6.6.5) — my one pre-merge item — is now handled.
eval_separation_rgb/ the unified resolver now evaluate true n-input tint transforms rather thantints.first(), so the hexachrome-style DeviceN case degrades correctly instead of being mis-evaluated against a single colorant. 👍 - Type 4 follow-up done. Wiring the v0.3.59 Type 4 (PostScript calculator) evaluator into the Separation/DeviceN tint path via
evaluate_type4_clampedcloses the loop I'd noted — spot colours with calculator transforms no longer fall back to grey. - Lab/ICC alternate follow-up done. A 3-component Lab alternate is now CIELAB→sRGB instead of being read as out-of-range RGB, with the intentional no-CMM chromatic-adaptation skip documented. That removes the "3-component = assume RGB" ambiguity I mentioned.
And the unify colour-operator resolution refactor is a real bonus — collapsing the five near-duplicate sc/SC/scn/SCN arms into one resolve_color_to_rgb path is a maintainability win and fixed two latent bugs for free (stroke Separation/DeviceN had no arm at all, and the Indexed index/255 grayscale guess is now a real palette lookup).
Two tiny, non-blocking notes (no changes required):
- In the unified grey fallback an unevaluable full-tint Separation now resolves to white (
components[0]) rather than the old black — only fires when the transform genuinely can't be evaluated, so it's a wash; just flagging the behavior change. test_components_to_rgb_count_fallbackonly asserts the 4-component CMYK path despite its name referencing the 3-vs-Lab disambiguation — purely cosmetic.
On CI: I've approved the held workflows and they're running now (first-time-contributor gate). All my review points are addressed, so this is effectively a 👍 from me — I'll give the final approval once the suite comes back green. Thanks again, this turned into a notably broad and well-tested fix.
yfedoseev
left a comment
There was a problem hiding this comment.
Approving. ✅ All the points from my earlier reviews are resolved — multi-colorant DeviceN (my one pre-merge item) now evaluates true n-input tint transforms, Type 4 and Lab/ICC alternates are wired in, and the colour-operator unification refactor is a clean bonus that fixed stroke-Separation and Indexed lookup for free.
Note: this approval is for the code review. The held CI suite is running now (first-time-contributor gate); if anything comes back red there we'll treat it as a separate fix-up, but the review itself is a 👍. Thanks again, @apptekstudios — a notably broad and well-tested fix.
Problem
A
Separation/DeviceNspot colour set at full tint withscnrenders as solid black instead of being resolved through its tint transform. This blacks out tinted callout boxes and headings on InDesign-exported PDFs.In
src/rendering/page_renderer.rs:scn(SetFillColorN)Separation/DeviceNarm never evaluated the tint transform — it always usedgrey = 1 - tint, so a full tint (1.0) became black.sc(SetFillColor) arm only handledFunctionType 2with aDeviceCMYKalternate, so sampled (FunctionType 0) transforms and DeviceRGB/Gray alternates hit the same grey fallback.Fix
Both arms now evaluate the tint transform via a shared
eval_separation_rgbhelper:eval_pdf_function_1dsupportsFunctionType 2(exponential interpolation) andFunctionType 0(sampled, MSB-first packed samples with Domain/Encode/Decode/Range).components_to_rgbmaps the alternate-space output to RGB by component count: 1 → Gray, 3 → RGB, 4 → CMYK (via the existingcmyk_to_rgb).grey = 1 - tintapproximation remains only as the fallback when the function can't be evaluated.Reproducer
examples/separation-blackout/contains two minimal single-page PDFs that fill the whole page via1 scnwith a full-tintSeparationcolour whose tint transform maps tint1.0 → CMYK(0.1, 0, 0.15, 0)— a light green (RGB ≈ 230, 255, 216):separation-type2.pdfFunctionType 2(exponential)separation-type0.pdfFunctionType 0(sampled)Before this change both render solid black
(0, 0, 0); after, both render the expected light green. Before/after PNGs are included in the example directory.Tests
tests/separation_color.rsrenders both committed reproducer PDFs and asserts the centre pixel is light and green-dominant rather than black, covering both function types. No new dependencies are added (the PDFs are committed fixtures; the test reads them viainclude_bytes!).