Fix Imagick grayscale flattening saturated colors - #1516
Open
nlemoine wants to merge 1 commit into
Open
Conversation
The Imagick GrayscaleModifier used modulateImage(100, 0, 100), which sets HSL saturation to zero. HSL lightness of any fully saturated color is (max+min)/2, so every saturated hue ended up on the same mid grey and all tonal information was lost. Use transformImageColorspace() to GRAY and back to sRGB instead, the same approach the vips driver already uses. This produces a luma based grey and also handles CMYK sources correctly. The existing tests only asserted that the result is grayscale, which the flat mid grey satisfied. Added a test on both drivers checking that pure blue, red and green keep a distinct and correctly ordered brightness.
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.
GrayscaleModifieron the Imagick driver turns every saturated color into the same mid grey. All the tonal information is gone.Repro, on a 200x200 PNG with four flat quadrants (red, green, blue, yellow), sampling each quadrant centre after
->grayscale():Cause: the modifier calls
modulateImage(100, 0, 100), which sets HSL saturation to zero. HSL lightness of a fully saturated color is(max+min)/2 = 127.5, so any saturated hue lands on 128 no matter how bright it looks.Fix:
transformImageColorspace()toCOLORSPACE_GRAY, then back toCOLORSPACE_SRGB. The vips driver already does the same withInterpretation::B_WthenInterpretation::SRGB.I first tried a color matrix with Rec. 601 coefficients, it would have matched GD almost exactly. But it breaks on CMYK sources. The matrix hits the C, M and Y channels, so the tonality comes out inverted, red becomes light and yellow becomes dark. The colorspace transform handles CMYK fine.
Other things I checked: alpha and semi transparency survive, animated GIFs get every frame converted,
colorspace()still reportsRgb\Colorspace. Encoding to png/jpeg/gif/webp is unchanged.Tests:
testColorChangeonly assertedisGrayscale(), and128,128,128satisfies that, so nothing caught it. I addedtestSaturatedColorsKeepDistinctBrightnesson both drivers, using the existingblocks.pngfixture. It checks that pure blue, red and green stay distinct and in the right order (blue darkest, green brightest). That ordering should hold for any luma based conversion, so the same test works for GD and Imagick. On the old Imagick code it fails withFailed asserting that 128 is less than 128.