Adds color conversion to SRGB for images with CMYK palette #25277
Conversation
… color palette For ImageMagic, when the original image was in CMYK palette, the final image after resize was kind of "in negative". I decided to force conversion to RGB, but when converting to RGB from CMYK with IMagixk - colors were not natural. So I picked SRGB (I found this hint in StackOverflow)
|
Hi @krisdante. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
|
Those Exceptions (Code style failing) were there before. I think migrating that is a separate topic and I will do that with another pull request. |
|
@magento run Functional Tests CE, Functional Tests B2B, Functional Tests EE |
There was a problem hiding this comment.
Hi @krisdante,
Your change looks good to me. Could you cover your changes with unit test?
| /** | ||
| * Converts colorspace to RGB when required | ||
| */ | ||
| protected function ensureColorspace() |
There was a problem hiding this comment.
Please make this method private and add return type, like this.
| protected function ensureColorspace() | |
| private function ensureColorspace(): void |
|
Hi @krisdante, |
|
I need to think about it. The tricky part with unit testing that is that you cannot check colours programatically. They will be fine. They are just displayed brokenly to a human eye when wrong colour space conversion will happen. |
|
Also merging this before #26036 would be nice. |
There was a problem hiding this comment.
I need to think about it. The tricky part with unit testing that is that you cannot check colours programatically. They will be fine. They are just displayed brokenly to a human eye when wrong colour space conversion will happen.
The test here was to upload the image attached to this bug, and check if it is looking good in Magento.
It is really hard to program this as integration test.
@krisdante yeah, that's good point for sure, but I didn't see any integration tests for image magic, but this would be really good thing if you could write it!
By unit test we could just check that your code was executed and in case if you'll write integration - it will be enough. If you'll have any issues with integration tests - unit test we still could write without any issues.
Also I wrote few comments to existing functionality, please check them
Could you update your PR accordingly?
| case \Imagick::COLORSPACE_CMYK: | ||
| case \Imagick::COLORSPACE_UNDEFINED: | ||
| $this->_imageHandler->transformImageColorspace(\Imagick::COLORSPACE_SRGB); | ||
|
|
There was a problem hiding this comment.
| break; |
| $colorspace = $this->_imageHandler->getColorspace(); | ||
| switch ($colorspace) { | ||
| case \Imagick::COLORSPACE_RGB: | ||
| return; |
There was a problem hiding this comment.
I think we could just remove this case.
0e3d6f2 to
8726e9b
Compare
|
Hi @krisdante, |
|
@krisdante I am closing this PR now due to inactivity. |
|
Hi @krisdante, thank you for your contribution! |
Description (*)
Adds color conversion to SRGB for images with CMYK and Unrecognizable color palette
For ImageMagic, when the original image was in CMYK palette, the final image after
resize was kind of "in negative". I decided to force conversion to RGB, but when
converting to RGB from CMYK with IMagixk - colors were not natural.
So I picked SRGB (I found this hint in StackOverflow)
Fixed Issues (if relevant)
Manual testing scenarios (*)
See the issue for a detailed path to reproduce / test
Questions or comments
It might be that "normal" RGB images will get converted to SRGB in some cases. I tested a few RGB images and I do not see the quality problem.
I fixed a few lines in this file were phpcs was complaining.
Contribution checklist (*)