On Tue, Aug 24, 2010 at 10:22 AM, Rupert Weber <gimp@xxxxxxxxxxxxxx> wrote: > On 08/22/2010 02:45 PM, Sven Neumann wrote: >> New code in GIMP should use babl for pixel format conversion. There's no >> need to introduce new API for that as we have babl which is available to >> the core and plug-ins and provides a much superior API. > > The short answer is: No. I won't do that. > For the long answer see further down below. (Sorry if this post becomes > a bit longish) > > First about the current state of affairs: > I posted the last update to the patch to > http://bugzilla.gnome.org/attachment.cgi?bugid=325564 > > From my point of view, it is now done. > > Some performance numbers, comparing redraw times of legacy modes to the > new LCH modes and to current GEGL LCH modes: > (Tested with a very large picture to get measurable numbers; still these > are ca. numbers, obtained with a stop watch) > > Mode | Legacy | New LCH | GEGL/babl > -------------+--------+---------+---------- > Hue | 3.6 | 6.4 | 396 > Saturation | 4.6 | 6.2 | 405 > Color | 4.7 | 4.1 | 431 > Value/Lightn.| 3.5 | 4.1 | 416 > > So I'm in the ball park of the legacy modes, Color is even a little > faster. Compared to current GEGL/babl the new modes are about 60 to 100 > times faster. (Yes, no typo) I hope you're not associating the quite suboptimal way in which GIMP currently uses GEGL, with BABL's speed or lack of speed. BABL just processes raw pixel buffers. A converter function just accepts a source and a destination pointer, along with a pixel count, and should convert that number of pixels. It doesn't have any heavy architecture or processing, aside from what may be in each individual converter function looking at your code in gimpcolorspace.c, making that code work with BABL looks like it's pretty much a case of cut+paste, modify the way the input is referred to, add some registration code. (getting your layer mode code to USE that, is a different issue, and I agree that would be non-trivial, although you might get significant speed gains from it because of greatly reduced function call overhead -- probably about as much as you describe for inlining below.) > > As to accuracy, these are the round-trip pixel errors for Lab and LCH > conversions: > Error after round-trip [in 8bit RGB space]: > L*a*b* L*C*H* > 0: 16561783 ( 98.716%) 0: 16527659 ( 98.513%) > 1: 214941 ( 1.281%) 1: 248244 ( 1.480%) > 2: 492 ( 0.003%) 2: 1313 ( 0.008%) > 3: 0 ( 0.000%) 3: 0 ( 0.000%) > > The worst we get are off-by-two errors. You won't notice without > diff'ing two layers. > If you don't just stack no-op layers on top of each other, out-of-gamut > errors will be *far* greater than these. > > So, as I already said, I consider the patch done now. > > Things I will still be glad to change: > - Location/name of new file, name of exported functions, etc. > - Any bug fixes, of course. > - The open issues I had mentioned earlier (file formats, > GIMP_COMPOSITE_BLEND et al.) > > Things I won't change: > - Optimization. I currently see no further optimization potential > without uglifying the code. > - As to putting the conversion outside the loop: Yes it can be done, > but even if it is done, it doesn't belong in this patch. > The current implementation in gimp_composite_generic.c is symmetric > to the existing layer modes. So any such un-looping would be a > general change to that file, not specific to the new layer modes. > - Inlining: Brutally inlining everything can be done and gives some > 12%-15% performance increase. -- But I don't want to get my hands > dirty with that.. :o) > > And then there is babl. > I feel very bad about that request. Because I expect it to be the first > step in a relatively short sequence of if-we-do-that-why-don't-we's that > will end with these modes not getting in but rather be added to the GEGL > agenda. > As that is effectively what you are asking me to do: work on the GEGL > modes instead (or duplicate them, which would be even sillier). GEGL modes are something else. I believe what Sven was suggesting is to implement your conversion code in a BABL extension, and use that to do color conversion in the layer modes; Not to use GEGL for that whole thing. That said... > But that is not what I signed up for. The idea was to get something done > and usable now. Not something that will be great in some uncertain future. > > When this is done I will be glad to take a look at babl and see if the > conversions can somehow be integrated. But I don't expect that to be a > trivial task. > > The patch is here. Now, and it works. The conversions add 17k of code. > Once GEGL takes over, they'll simply removed again. No one gets hurt. I absolutely agree that this patch should be applied promptly. It implements much-wanted functionality in an effective and efficient way. Separating the new color conversions into their own file in app/composite/ is a smart idea, it will make later BABL migration easy. I only have one thing to criticize about this patch: It adds a single whitespace error :) [at the end of app/composite/gimp-lab-lch.c, there is an extra newline. Whoever commits it can easily amend the commit before pushing it, though, so basically a non-issue] _______________________________________________ Gimp-developer mailing list Gimp-developer@xxxxxxxxxxxxxxxxxxxxxx https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
