-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix conversion from RGB to signed CMYK #522
Fix conversion from RGB to signed CMYK #522
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the attempt to fix it.
Please, if you could DO ensure any new features or changes to existing behaviours are covered with test cases. IOW, please add a tests for the bug fix (see some samples in #479).
@mloskot can you please have a look at the tests I just added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work with the tests!
I added two nitpick comments.
We also need to update the build configuration, so we can build and run the test program locally and with the the CI builds, so if you could do this:
For Boost.Build, add run default_color_converter_rgb_to_cmyk.cpp ;
below this line
Line 15 in f6a3553
run default_color_converter_impl.cpp ; |
For CMake, remove )
from this line and add default_color_converter_rgb_to_cmyk)
below this line:
gil/test/core/color/CMakeLists.txt
Line 11 in f6a3553
default_color_converter_impl) |
include/boost/gil/color_convert.hpp
Outdated
dst_t const y = channel_invert(channel_convert<dst_t>(b)); // y = 1 - b | ||
dst_t const k = (std::min)(c, (std::min)(m, y)); // k = minimum(c, m, y) | ||
using uint_t = typename channel_type<cmyk8_pixel_t>::type; | ||
uint_t c = channel_invert(channel_convert<uint_t>(r)); // c = 1 - r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: trailing space after r
should be removed
include/boost/gil/color_convert.hpp
Outdated
uint_t c = channel_invert(channel_convert<uint_t>(r)); // c = 1 - r | ||
uint_t m = channel_invert(channel_convert<uint_t>(g)); // m = 1 - g | ||
uint_t y = channel_invert(channel_convert<uint_t>(b)); // y = 1 - b | ||
uint_t k = (std::min)(c,(std::min)(m,y)); //k = minimum(c, m, y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: missing space before k
in //k
The CI are failing because of
I think you can remove include of this header from the test, as you don't seem to be using any fixture. |
I guess I should have been more attentive and avoided that issue/fail. |
We all learn from mistakes. As you see, submitting a PR is an effort. I'm on mobile now and can't verify the result, but I'll take it from the CI, and I'll merge. Thank you! |
Thank you as well. |
Description
Conversion of RGB pixels based on unsigned integral channels to CMYK based on signed integral channels was/is broken (issue #479). It happend because relations used for color correction like
c' = (c - k) * s
etc. didn't work well for signed integral channels.This correction is attempted by doing all color correction calculations of pixel values based on unsigned integral channels then(after the calculations are finished) converting those values to signed ones.
References
Principles of Digital Image Processing - Fundamental Techniques (Book name provided in
color_convert.hpp
)Tasklist