From b327f8bcc6746ee0b5aef0018a4bdcc202252e6f Mon Sep 17 00:00:00 2001 From: Nicolas Lemoine Date: Sat, 22 Aug 2026 10:31:48 +0200 Subject: [PATCH 1/2] Fix Imagick grayscale flattening saturated colors 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. --- .../Imagick/Modifiers/GrayscaleModifier.php | 16 +++++++++++++--- .../Gd/Modifiers/GrayscaleModifierTest.php | 15 +++++++++++++++ .../Imagick/Modifiers/GrayscaleModifierTest.php | 15 +++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php b/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php index 77112949c..44fd9abd1 100644 --- a/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php +++ b/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php @@ -4,6 +4,7 @@ namespace Intervention\Image\Drivers\Imagick\Modifiers; +use Imagick; use ImagickException; use Intervention\Image\Exceptions\ModifierException; use Intervention\Image\Interfaces\ImageInterface; @@ -19,15 +20,24 @@ public function apply(ImageInterface $image): ImageInterface { foreach ($image as $frame) { try { - $result = $frame->native()->modulateImage(100, 0, 100); + // turn image to grayscale + $result = $frame->native()->transformImageColorspace(Imagick::COLORSPACE_GRAY); if ($result === false) { throw new ModifierException( - 'Failed to apply ' . self::class . ', unable to modulate image', + 'Failed to apply ' . self::class . ', unable to transform image to grayscale', + ); + } + + // return to srgb colorspace with grayscale image + $result = $frame->native()->transformImageColorspace(Imagick::COLORSPACE_SRGB); + if ($result === false) { + throw new ModifierException( + 'Failed to apply ' . self::class . ', unable to transform image to grayscale', ); } } catch (ImagickException $e) { throw new ModifierException( - 'Failed to apply ' . self::class . ', unable to modulate image', + 'Failed to apply ' . self::class . ', unable to transform image to grayscale', previous: $e, ); } diff --git a/tests/Unit/Drivers/Gd/Modifiers/GrayscaleModifierTest.php b/tests/Unit/Drivers/Gd/Modifiers/GrayscaleModifierTest.php index 7beaf7502..09fffa4a3 100644 --- a/tests/Unit/Drivers/Gd/Modifiers/GrayscaleModifierTest.php +++ b/tests/Unit/Drivers/Gd/Modifiers/GrayscaleModifierTest.php @@ -6,6 +6,7 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\RequiresPhpExtension; +use Intervention\Image\Colors\Rgb\Channels\Red; use Intervention\Image\Modifiers\GrayscaleModifier; use Intervention\Image\Tests\GdTestCase; @@ -21,4 +22,18 @@ public function testColorChange(): void $image->modify(new GrayscaleModifier()); $this->assertTrue($image->colorAt(0, 0)->isGrayscale()); } + + public function testSaturatedColorsKeepDistinctBrightness(): void + { + // blocks.png holds pure blue, green and red in three of its quadrants + $image = $this->readTestImage('blocks.png')->modify(new GrayscaleModifier()); + + $blue = $image->colorAt(160, 120)->channel(Red::class)->value(); + $green = $image->colorAt(160, 360)->channel(Red::class)->value(); + $red = $image->colorAt(480, 360)->channel(Red::class)->value(); + + // any luma based conversion keeps blue darkest and green brightest + $this->assertLessThan($red, $blue); + $this->assertLessThan($green, $red); + } } diff --git a/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php b/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php index 6ccf3e68e..54fca50d2 100644 --- a/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php +++ b/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php @@ -6,6 +6,7 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\RequiresPhpExtension; +use Intervention\Image\Colors\Rgb\Channels\Red; use Intervention\Image\Modifiers\GrayscaleModifier; use Intervention\Image\Tests\ImagickTestCase; @@ -21,4 +22,18 @@ public function testColorChange(): void $image->modify(new GrayscaleModifier()); $this->assertTrue($image->colorAt(0, 0)->isGrayscale()); } + + public function testSaturatedColorsKeepDistinctBrightness(): void + { + // blocks.png holds pure blue, green and red in three of its quadrants + $image = $this->readTestImage('blocks.png')->modify(new GrayscaleModifier()); + + $blue = $image->colorAt(160, 120)->channel(Red::class)->value(); + $green = $image->colorAt(160, 360)->channel(Red::class)->value(); + $red = $image->colorAt(480, 360)->channel(Red::class)->value(); + + // any luma based conversion keeps blue darkest and green brightest + $this->assertLessThan($red, $blue); + $this->assertLessThan($green, $red); + } } From e84775b2712fd5b32165b4bd77c8dee49d618b91 Mon Sep 17 00:00:00 2001 From: Nicolas Lemoine Date: Mon, 24 Aug 2026 21:09:03 +0200 Subject: [PATCH 2/2] Keep the colorspace of the source in grayscale Going back to sRGB moved a cmyk source to another colorspace, which is not what a color operation should do. Read the colorspace of each frame before the conversion and return to it. A cmyk source now stays cmyk and the grey lands in the black channel, cmyk(0 0 0 43) where the source pixel was cmyk(5 58 7 0). An sRGB source is unaffected, the second transform was already going back to sRGB. The frame test uses a sequence whose frames deliberately do not share a colorspace. Built on identical frames it passed against a hardcoded sRGB and against reading the colorspace once outside the loop, so it pinned nothing. --- .../Imagick/Modifiers/GrayscaleModifier.php | 10 ++- .../Modifiers/GrayscaleModifierTest.php | 63 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php b/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php index 44fd9abd1..dabcc8be2 100644 --- a/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php +++ b/src/Drivers/Imagick/Modifiers/GrayscaleModifier.php @@ -20,6 +20,11 @@ public function apply(ImageInterface $image): ImageInterface { foreach ($image as $frame) { try { + // read the colorspace of the frame to be able to restore it, + // getImageColorspace() only reports the frame the iterator is + // currently pointing at + $colorspace = $frame->native()->getImageColorspace(); + // turn image to grayscale $result = $frame->native()->transformImageColorspace(Imagick::COLORSPACE_GRAY); if ($result === false) { @@ -28,8 +33,9 @@ public function apply(ImageInterface $image): ImageInterface ); } - // return to srgb colorspace with grayscale image - $result = $frame->native()->transformImageColorspace(Imagick::COLORSPACE_SRGB); + // return to the colorspace of the source, grayscale is a color + // operation and is not meant to move the image to another one + $result = $frame->native()->transformImageColorspace($colorspace); if ($result === false) { throw new ModifierException( 'Failed to apply ' . self::class . ', unable to transform image to grayscale', diff --git a/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php b/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php index 54fca50d2..ccdaf82bb 100644 --- a/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php +++ b/tests/Unit/Drivers/Imagick/Modifiers/GrayscaleModifierTest.php @@ -6,7 +6,17 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\RequiresPhpExtension; +use Imagick; +use ImagickPixel; +use Intervention\Image\Colors\Cmyk\Channels\Cyan; +use Intervention\Image\Colors\Cmyk\Channels\Key; +use Intervention\Image\Colors\Cmyk\Channels\Magenta; +use Intervention\Image\Colors\Cmyk\Channels\Yellow; +use Intervention\Image\Colors\Cmyk\Colorspace as CmykColorspace; use Intervention\Image\Colors\Rgb\Channels\Red; +use Intervention\Image\Drivers\Imagick\Core; +use Intervention\Image\Drivers\Imagick\Driver; +use Intervention\Image\Image; use Intervention\Image\Modifiers\GrayscaleModifier; use Intervention\Image\Tests\ImagickTestCase; @@ -36,4 +46,57 @@ public function testSaturatedColorsKeepDistinctBrightness(): void $this->assertLessThan($red, $blue); $this->assertLessThan($green, $red); } + + public function testColorspaceIsPreserved(): void + { + $image = $this->readTestImage('cmyk.jpg'); + $this->assertInstanceOf(CmykColorspace::class, $image->colorspace()); + + $image->modify(new GrayscaleModifier()); + + // grayscale is a color operation, it has no business moving a cmyk + // source to another colorspace + $this->assertEquals( + Imagick::COLORSPACE_CMYK, + $image->core()->frame(0)->native()->getImageColorspace(), + ); + + // the grey has to sit in the black channel, the three ink channels + // carry no color anymore + $color = $image->colorAt(0, 0); + $this->assertEquals(0, $color->channel(Cyan::class)->value()); + $this->assertEquals(0, $color->channel(Magenta::class)->value()); + $this->assertEquals(0, $color->channel(Yellow::class)->value()); + $this->assertEqualsWithDelta(43, $color->channel(Key::class)->value(), 2); + } + + public function testColorspaceIsPreservedOnEveryFrame(): void + { + // the frames deliberately do not share a colorspace, an implementation + // that reads the colorspace once outside the frame loop, or that + // hardcodes one, cannot satisfy this + $imagick = new Imagick(); + $imagick->setFormat('tiff'); + + foreach ([Imagick::COLORSPACE_SRGB, Imagick::COLORSPACE_CMYK] as $colorspace) { + $frame = new Imagick(); + $frame->newImage(4, 4, new ImagickPixel('red'), 'tiff'); + $frame->transformImageColorspace($colorspace); + $frame->setImageDelay(10); + $imagick->addImage($frame); + } + + $imagick->setFirstIterator(); + $image = new Image(new Driver(), new Core($imagick)); + + $image->modify(new GrayscaleModifier()); + + foreach ([Imagick::COLORSPACE_SRGB, Imagick::COLORSPACE_CMYK] as $key => $colorspace) { + $this->assertEquals( + $colorspace, + $image->core()->frame($key)->native()->getImageColorspace(), + 'Frame ' . $key . ' did not keep its colorspace', + ); + } + } }