Draw a Lab colour instead of a grey of its lightness - #51
Merged
Merged
Conversation
labSpace read three numbers and used only the first, drawing a grey of the right weight. Its own comment said so and called it honest, which it was -- but it drops the two axes that carry the colour, and Lab(50, 20, -30) is a mid purple, not a mid grey. It is now the real conversion, through gfx/color v0.23.0's LabToSRGBWP: the CIE inverse under the space's own white point, adapted to D65. Two things came with it. /Range is read and kept on the space, because an image decodes against it. Lab is the ONE space in the format whose default /Decode is not [0 1] per component -- it is [0 100 amin amax bmin bmax] -- so a Lab image with no /Decode of its own was having its lightness read as a hundredth of what it said, and came out black. A Lab space with no /WhitePoint falls back to the equal-energy point (1, 1, 1) rather than to a device space, because Lab has no device namesake. That makes the white-point multiplication the identity, which is the most conservative reading of a file that said nothing, and it is also what GfxLabColorSpace's constructor does, so a malformed file is read the same way by both. This was blocked on a misreading, now corrected in the file's own comment. GfxLabColorSpace::getXYZ does not multiply by the white point, and reading that function alone says poppler disagrees with ISO 32000-2 8.6.5.4. It does not: ::getRGB multiplies immediately after calling it. A hand-built four-pixel Lab document run through pdfimages settles it -- the specification's formula matches 4 of 4 pixels within one level, the no-white-point formula is 13 levels out on a neutral mid tone. Every expected value here is poppler's, taken the same way: it draws Lab(50, 20, -30) with no white point as (131, 109, 171), and we draw (131, 108, 170).
decodeArray is called with the space's own component count, so the closure never sees an index outside the three it has. The rounded total read 100.0% while the function stood at 96.0%, which is why the check has to be per function -- as the gate does it -- and not on the summary line.
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
1 participant
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.
labSpaceread three numbers and used only the first, drawing a grey of the right weight. Its own comment said so and called it "honest about what it is" — and it was, but it drops the two axes that carry the colour.Lab(50, 20, -30)is a mid purple, not a mid grey.It is now the real conversion, through
gfx/colorv0.23.0’sLabToSRGBWP: the CIE inverse under the space’s own white point, adapted to D65. Two things came with it./Range, because an image decodes against itLab is the one space in the format whose default
/Decodeis not[0 1]per component: it is[0 100 amin amax bmin bmax]. A Lab image with no/Decodeof its own was having its lightness read as a hundredth of what it said, and came out black. The range is now read and carried on the space sodecodeArraycan use it.No
/WhitePointmeans the equal-energy pointLab has no device namesake to decline into, unlike
CalGrayandCalRGB.(1, 1, 1)makes the white-point multiplication the identity — the most conservative reading of a file that said nothing — and it is whatGfxLabColorSpace’s constructor uses, so a malformed file is read the same way by both.This was blocked on a misreading, and the correction is the interesting part
#50 left Lab alone on the grounds that poppler’s
GfxLabColorSpace::getXYZdoes not multiply by the white point where ISO 32000-2 8.6.5.4 says to, so a correct Lab and the judge would disagree.That was wrong.
getXYZindeed does not — but::getRGBmultiplies immediately after calling it:What settled it was not a closer reading but a witness: a hand-built four-pixel Lab PDF run through
pdfimages. The specification’s formula matches 4 of 4 pixels within one level; the no-white-point formula is 13 levels out on a neutral mid tone. The comment incalibrated.gonow records this rather than the claim it replaced.Every expected value here is the judge’s
Not ours. poppler draws
Lab(50, 20, -30)in a space with no white point as (131, 109, 171); we draw (131, 108, 170). Thegfxside is measured over 6 069 colours per white point: 0 levels under D50, 1 under D65 (go-gfx/gfx#40).Coverage stays at 100%.
🤖 Generated with Claude Code