GNOME Bugzilla – Bug 145401
Enhancement: display R, G and B histograms simultaneously
Last modified: 2004-12-22 21:47:04 UTC
I will attach a patch that implements a new pseudo-channel for GIMP's histogram that displays the R, G and B histograms simultaneously. This was inspired by the corresponding new feature in Photoshop CS. I haven't actually used PS CS, but I noticed this in some screenshot. See for instance http://www.oreillynet.com/pub/a/javascript/2004/03/23/digphoto.html . I have called the pseudo-channel "RGB", but Photoshop seems to call it "Colors". Another difference is that in my patch, the part where all subhistograms overlap is white (as comes naturally from the way it is implemented), while in Photoshop it is black. While pondering GIMP's histogram, it might be a good idea to provide pseudo- channels like "Saturation" and "Hue"? And "Luminosity" (er, or is that "Lightness") in addition of "Value" (which is simply the max of R, G, B at each pixel, which isn't perhaps perceptionally that meaningful).
Created attachment 29220 [details] [review] Suggested patch
May I ask why your patch is using a pixbuf? I fail to see the reason for doing this and I consider it an ugly hack that I don't want to see in the histogram view.
Using a pixbuf seemed to much easier and faster than setting up GdkGCs and calling gdk_draw_line() lots of times, especially considering the way the colours of the spikes for the RGB channels mix. With a pixbuf I just plonk 0xFF in the R, G or B byte for the height of the corresponding spike, and the mix happens automatically. Oh well, drawing using a GdkGC with function GDK_OR would do the same, I guess.
Did you check if all places working with GimpHistogramChannel values would work with the additional enum value? I'm pretty sure the levels and curves tools will badly crash.
No, I can't say I have really thoroughly tested it yet. Just wanted to put up something for people to try and comment on. I did note that the "RGB" entry doesn't even show up in the in the menu in the levels tool, though. The levels_set_sensitive_callback() doesn't allow the new enum value through (as it shouldn't). Ditto for curves_set_sensitive_callback().
GdkPixbuf is certainly the slowest way of drawing anything on X11. Apart from that using a pixbuf breaks drawing of the selection as well as drawing of the lines in the background. We need to change this code to use gdk_draw_line. I'd also prefer to see the combination of all channels drawn in black instead of white since that's what the user knows from the other histogram modes. We should probably draw the individual histograms using GDK_OR and draw the MIN (r,g,b) value in black on top of this.
Created attachment 29272 [details] [review] rewritten drawing routines Here's a patch based on your work that draws the histogram without using a pixbuf. It also changes the return value of gimp_histogram_get_value() for the RGB channel. It does now return the minimum value of the R, G and channels. I am not sure what the correct behaviour would be but I need this behaviour for the drawing code.
There are quite a few places in the code that use the GimpHistogramChannel enum and do not yet handle the new enum value. These need to be fixed before this patch can go into CVS.
Thanks! But that attachment still uses a GdkPixbuf? Did you diff in the wrong place?
Created attachment 29274 [details] [review] rewritten drawing routines, really! Oops, looks like I attached the wrong diff.
If you compare with the screenshot from the link above you will notice a difference though. PS seems to draw the channels normalized individually while our code draws the three channels into the same coordinate system. The latter seems to be more logical but I am not sure what's more usable.
Basically applied my patch as is: 2004-07-06 Sven Neumann <sven@gimp.org> Added an RGB histogram based on a patch by Tor Lillqvist. Fixes bug #145401. * app/base/base-enums.[ch]: added GIMP_HISTOGRAM_RGB, don't export it to the PDB. * app/base/gimphistogram.c: implemented histogram functions for the RGB mode. * app/base/levels.c * app/tools/gimpcurvestool.c * app/tools/gimplevelstool.c * app/widgets/gimpcolorbar.c * app/widgets/gimphistogrameditor.c: handle the new enum value. * app/widgets/gimphistogramview.c: for GIMP_HISTOGRAM_RGB mode, draw a histogram that shows the RGB channels simultaneously.
*** Bug 128694 has been marked as a duplicate of this bug. ***