Repository navigation
Return sRGB from get_pixel_color instead of raw linear values - #55
Open
floraschouw wants to merge 1 commit into
Open
floraschouw wants to merge 1 commit into
floraschouw wants to merge 1 commit into
Conversation
Gegl stores colour in linear light, so Gegl.Color.get_rgba() returns linear
floats. _get_pixel_color scaled those straight by 255, producing linear
values labelled as sRGB. Painting #ff5733 and reading the same pixel back
returned #ff1808.
Checked against babl on GIMP 3.2.4, which converts the same colours right:
colour get_rgba()*255 (old) R'G'B'A u8 (babl)
#ff5733 255, 24, 8 255, 87, 51
#6495ed 32, 76, 215 100, 149, 237
#28b45a 5, 116, 26 40, 180, 90
#808080 55, 55, 55 128, 128, 128
The conversion is done with the sRGB transfer function rather than by
calling babl. babl cannot be reached safely from this module: if it is not
yet initialised, babl_format() calls babl_fatal() -- "you must call
babl_init first" -- which aborts the process at the C level and cannot be
caught from Python. GIMP imports this file during its plug-in scan, before
that initialisation, so a module-level Babl.format() makes the entire
plug-in fail to register and disappear from the Tools menu.
The transfer function is exact for 8-bit output: it round-trips all 256
levels and matches babl on every colour above. It assumes sRGB, so a non-
sRGB working space would need babl (resolved lazily, from inside a handler);
the previous code assumed sRGB too, and got it wrong even for that.
Two smaller defects went with it:
- int(rf*255) truncated instead of rounding, losing a level on its own:
#6495ed came back as (32, 76, 215) where the correct answer is
(32, 77, 216) before conversion.
- Nothing clamped, so an out-of-gamut or HDR channel above 1.0 could report
a value greater than 255.
Alpha is deliberately left out of the transfer function; it is stored
linearly and takes no gamma encoding.
Fixes gimp-mcp#43
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Fixes #43.
Cause
Gegl stores colour in linear light, so
Gegl.Color.get_rgba()returns linear floats._get_pixel_colorscaled those straight by 255 and labelled the result sRGB. Paint#ff5733, read the same pixel back, get#ff1808.Checked against babl on GIMP 3.2.4, which converts the same colours correctly:
get_rgba()*255(old)R'G'B'A u8(babl)#ff5733#6495ed#28b45a#808080Why this does not call babl
My first attempt resolved babl's
R'G'B'A u8format at module level. That breaks the plug-in completely, and the failure is worth recording because it is not a normal exception:babl_format()callsbabl_fatal()when babl is not yet initialised, which aborts the process at the C level. It cannot be caught from Python --except Exceptiondoes not see it. GIMP imports plug-in files during its plug-in scan, before that initialisation, so the plug-in never registers and the whole Tools > MCP menu disappears with it.So the conversion uses the sRGB transfer function directly. For 8-bit output it is exact: it round-trips all 256 levels and matches babl on every colour in the table above.
The trade-off is that it assumes an sRGB working space, where babl would honour the image's actual colour space. The previous code assumed sRGB too and got it wrong even for that, so this is strictly better; doing it properly would mean resolving babl lazily from inside a handler rather than at import.
Two smaller defects in the same two lines
int(rf*255)truncated instead of rounding, losing a level independently of the gamma bug:#6495edcame back as(32, 76, 215)where correct rounding gives(32, 77, 216)before conversion.Alpha is deliberately excluded from the transfer function; it is stored linearly and takes no gamma encoding.
Verification
Against a running GIMP 3.2.4 on Windows 10 -- paint, then read the same pixel back:
#ff5733#ff1808#ff5733cornflowerblue#204cd7#6495edrgb(40,180,90)#05741a#28b45a#808080#373737#808080#000000#000000#000000Every colour now round-trips exactly, including the mid-grey that is most sensitive to the transfer function.
python -m py_compilepasses andruff checkis unchanged at 183 findings. The module also imports cleanly under GIMP's own Python 3.14, which is the check that catches the babl abort described above.