I was just looking over some of the additions to libgimp, and had a few
comments about the colorspace functions...
This style:
void gimp_rgb_to_hsv (gint *red /* returns hue */,
gint *green /* returns saturation */,
gint *blue /* returns value */);
of having a argument named one thing and return another makes me a
little uneasy, but since it is so clearly documented in the definition
of the function and it means not having six parameters, I suppose it can
be okay.
I'll admit up front that I haven't actually run the numbers on this next
point, but when I was a lad my father always told me that function calls
were slow (and ones to shared libraries moreso)... And as I believe the
most common case of a color-space conversion in a plug-in would be to
convert the entire pixel region, would it not be prudent to provide a
vector form of the conversion routines?
So instead of this:
gint chan_r[], chan_g[], chan_b[];
for(xx=0;x<width;x++) {
for(yy=0;y<width;y++) {
to_hsv(chan_r++,chan_g++,chan_b++);
}
}
you have this:
guint buflen = height * (width + rowstride);
array_to_hsv(chan_r, chan_g, chan_b, buflen);
which puts the loop inside function call, where it's faster (as to_hsv
itself makes no function calls) and cleaner looking for the client.
You could implement the single-conversion form (which isn't going to be
speed-critical) as a call to the array form with a length of 1, to avoid
code duplication.
Looking at this usage makes us question the 3 vs 6 parameter choice
again too.
1) The RGB buffer is probably a guchar, while the definition says the
output is gint (I assume because hsv space has a different range
than rgb space).
2) You may not want your input buffer overwritten.
I think the 6-parameter version:
if(DontOverwrite) {
guchar *r, *g, *b;
gint *h, *s, *v;
h = g_new(gint, buflen);
s = g_new(gint, buflen);
v = g_new(gint, buflen);
} else {
gint *r, *g, *b;
gint *h, *s, *v;
h=r, s=g, v=b;
}
to_hsv(r,g,b, h,s,v, buflen);
is cleaner than with 3:
if(DontOverwrite) {
guchar *r, *g, *b;
gint *h, *s, *v;
bufcopy(r,h,buflen);
bufcopy(g,s,buflen);
bufcopy(b,v,buflen);
} else {
gint *h, *s, *v;
}
to_hsv(h,s,v, buflen);
...as you avoid the copying with the 6-parameter version.
Note that the above arguments apply whether the pixel iterator loop is
in front of or behind the function call, either way.
The other question that begs asking if you offer making array versions
is, do you use three seperate buffers, or a single buffer like RGB data
is commonly in? I am not sure of this one-- people will surely want
rgb data in one buffer, but my experience suggests that if you are
splitting into HSV (or similar) color space, you are likely working with
the channels independantly, so would rather have seperate buffers... It
seems like people will always want it "the other way" here, and
providing all options gets undesirably complex. (It wouldn't be hard,
but it would make a bit of a mess, so it would have to be worth it.)
I will defer to the wisdom of folks like Mitch on these issues, who have
been spending far more time in many more plug-ins that I have, but I
wanted to be sure they were raised.
Regards,
- Kevin
--
acapnotic@xxxxxxxxxxxxxxxxxxxxx | OpenPGP encryption welcome here, see X-DSA-Key
