GNOME Bugzilla – Bug 125303
Improve color pickers in Levels tool
Last modified: 2004-12-22 21:47:04 UTC
Make Levels tool more practical. Photoshop uses Lightness scale of RGB channels called Luminosity. Gimp uses different weights on each channel so resulting gray value is less valuable. Gray Button uses assumption that user will choose picked gray with L*=50 so image Gamma is changed as well. No other software use this odd idea. Gamma law says that power coefficients can be found using simple division: R'Gamma = log(Grey)/log(R') ; G'Gamma = log(Grey)/log(G') ; B'Gamma = log(Grey)/log(B') ; ' denote means nonlinear RGB data Because GIMP is not color managed inside so you can use well defined luminosity grey value from the NTSC norm: Grey = 0.299*R' + 0.587*G' + 0.114*B' ; Now no matter how bright selected area is - gray balance will not change image brightness. This is important !!! Regards --Jack,
This is still two separate issues in a single bug report. However the Luminance channel addition is already handled in bug #109161. If we add it to the Histogram, it will instantly be available to the Levels and Curves tools as well. So, let's focus on the color-pickers in the Levels tool in this report.
When implementing the color pickers in the Levels tool, I wasn't sure what the gray picker should do so I implemented something that could be useful. I agree that the behaviour should be changed. However I would appreciate if we would discuss what the final version should look like and how it should behave. Currently there are color pickers for the individual channel and an extra set of pickers for "All Channels". I wonder if we really need this. Then, the gray color picker for the individual channels is now located next to the Gamma entry. This indicates that it changes Gamma. So if we change the behaviour, the picker should probably be located to a different spot in the dialog. Alternatively we could leave it in place and only change the behaviour of the global (All Channels) gray-point picker. I would appreciate comments on this and perhaps someone could even come up with a dialog mockup?
I really need some feedback on this or the behaviour will not change for 2.0. And not changing it before 2.0 will make it even more difficult to change it later.
Hello Sven, Sorry for delay but we had a local area network problem. What I collected so far - remove individual channels grey and white/black points. Those tools are useless. All channels buttons (this near auto button) should be in place of individual channels buttons - next to the text fields. Now: 1. grey picker widget should be active (be 'clickable') only in Value channel and adjust RGB channels using Gamma coefficients. This coefficients are visible in current GIMP. Disable grey picker widget in RGB channels. Those coefficients you may count using the formula from my former letter. 2. white/black pickers visible on all channels (just like the current behavior) As far as I know you only need delete a code that changes the brightness and rearrange interface. With kind Regards --Jack,
OK - as I understand it, the behaviour expected is: If the user sets a black point for Value, that will change all channels. If the user sets a white point for Value, same thing If the user selects a grey point then that grey point is transformed using: L = 0.3R + 0.59G + 0.11B (formula slightly different from the one Jack gives, but we use it more or less everywhere. Perhaps we could change this to 0.299, 0.587, 0.114 everywhere, if that's a more accurate metric?) Gamma = log (Value) / log L I'm attaching a patch that does this for the grey picker, but without any UI changes. Jack, is this the behaviour you expect?
Created attachment 22413 [details] [review] Patch (to be applied in app) to change greypoint behaviour for all channels
If this is the behaviour you're expecting, then indeed the white, black & grey pickers should always apply to all channels, and not just to the active channel. What's the better UI, then? To leave the pickers beside the spinbuttons, or to remove them from there and leave them in the "all layers" frame? Is it useful to be able to independently pick the white & black points for an individual channel? Cheers, Dave.
Dave, This absolutely what I expect. I undersand that this Gamma coefficient will be sent to the GUI to Gamma widgets on RGB channels separetly as is now. The color metric should use tri digits precision if you like to be strict but this is less important here. I have a hope you've deleted a brightness change, have you? Thank You --Jack,
Dave, >Is it useful to be able to independently pick the white & black >points for an individual channel? Absolutely - for very hue sensitive scenes where 90% white is off one should do pick red channel maximally and then set other channels manually, little back to the right (smaller lightness) for skin: Proper white point: http://main.pam.szczecin.pl/~piotrlg/t/fshop_ekta_srgb.jpg Adjusted for 3 channels in using Value picker: http://main.pam.szczecin.pl/~piotrlg/t/lcms-ekta-srgb.jpg Jack,
Please modify the patch to use GIMP_RGB_INTENSITY() instead of hardcoding the values that are defined libgimpcolor/gimprgb.h.
This was a really bad idea :D "- levels->gamma[channel] = log (inten) / log (0.5);" Jack
Created attachment 22415 [details] [review] Updated patch (still in app) which uses GIMP_RGB_INTENSITY
Hi, Setting milestone 1.3.x on this to get a comment... Should I commit this, or something similar? Currently, this doesn't affect the behaviour for the individual channels at all. Cheers, Dave.
One thing about the patch: "libgimpcolor/gimprgb.h" must not be included directly. Include "libgimpcolor/gimpcolor.h" instead.
The code introduced by the patch will cause a crash or undefined behaviour when the user clicks on a black spot. "out_light" will become 0 and log(0) is not defined. Please, while you fix the patch, also remove the newline it introduces.
Modified patch applied in CVS. 2003-12-26 Dave Neary <bolsh@gimp.org> * app/base/levels.c: Modify behaviour of levels tool to conserve lightness when using the grey-point color-picker.
IMHO this isn't fixed properly. The new behaviour of the grey picker is fine for the "All Channels" grey point picker but the behaviour of the color picker that is next to the gamma entry should not have been changed. It is supposed to adjust the gamma factor of the image so that the picked pixel gets an intensity of 0.5. It should probably not do this individually on the color channels. It should only modify the gamma value not introducing any color changes.
I don't see how this does anything but confuse things. At the moment we have 15 colour pickers (one for each channel + value + "all channels"). The black & white points for the channels operate on the selected channel (introducing colour casts). Why shouyld gamma behave differently? It makes more sense to me to remove the gamma colour picker entirely from the "per channel" section than to have 2 grey-point colour pickers, one which corrects colour casts and the other which makes a point mid-grey. Cheers, Dave.
But the color picker next to the gamma setting would simply change the gamma setting, nothing else. That seems completely intuitive to me. Surely a lot more intuitive than the current behaviour.
How about my proposal to drop the grey colour picker from the per-channel box altogether? Dave.
Created attachment 24526 [details] [review] Patch to remove grey picker from per-channel frame in levels dialog
OK, will commit this later with some changes to the dialog layout.
2004-02-19 Sven Neumann <sven@gimp.org> * app/tools/gimplevelstool.c (gimp_levels_tool_dialog): applied patch from David Neary that removes gray point pickers for individual channels (bug #125303). Let the levels widgets expand with the dialog.