GNOME Bugzilla – Bug 347075
More Oilify plug-in enhancements
Last modified: 2007-06-18 06:18:54 UTC
Screenshot: http://www.iskunk.org/tmp/oilify-dialog.png The forthcoming patch adds three principal features: 1. Localized control of the mask size via a map; 2. Control of the exponent used in the oilify algorithm; 3. Localized control of the exponent via a map. Minor features added along the way: 4. Processing speed in intensity mode (the common case by far) has been increased by eliminating redundant pixel intensity calculations; 5. The mask area is now a circle centered about a point, instead of a square (which I find to be more intuitive); 6. Comments in the code have been improved a bit, and the deeply nested loops made a bit less confusing by loading up the for-loop specifiers. The patch is _very_ disruptive, so I will also attach a copy of the updated source. There are [at least] two issues that need to be addressed before this can go into CVS, however: 1. How feasible would it be to change the order of the non-interactive arguments? I appended the new ones, but the order is now a bit awkward---in particular I feel that "mode" should be the last one, so not to interpose between mask-related args, and because it's rarely ever going to be changed from the default of MODE_INTEN. 2. Similarly, what about inverting the sense of the "mode" argument? I think it would be more intuitively shown in the dialog as "Oilify channels independently," with a default of false/unchecked, and an underlying variable matching that.
Created attachment 68698 [details] [review] The aforementioned patch Notes on some of the noteworthy changes, in patch order: * Dropped the INTENSITY() macro, in favor of [the newer] GIMP_RGB_LUMINANCE() * fast_powf() is used as a faster approximation of pow() when the exponent is an integer. (I didn't use powf() proper as it's C99) * Some tweaks to the comments, to make them a little clearer * Use a look-up table for the square function, so that we don't even have to do multiplication in the innermost loop * If working in intensity mode, generate an intensity map of the image, so that we don't end up calling GIMP_RGB_LUMINANCE() multiple times per pixel * Many of the for-loops have been reorganized so that the declarations contain _all_ the variables that change in the loop, not just the controlling variable. It was confusing to have one variable in the for-declaration and others in the loop body. * The algorithm now uses a circular-sized mask area instead of a square one * Renamed the "run" variable, as it was shadowing the static run() function.
Created attachment 68699 [details] The updated source file By the way, are tabs kosher?
As mentioned in HACKING, we don't use Tabs in the GIMP source code. You might find tabs in some files, but you are encouraged to convert all tabs to spaces when you see them. Some comments on the patch: (1) Adding parameters to PDB calls is unfortunately not backwards compatible and we try not to break the PDB interface of scripts. It might be better to introduce a new procedure. (2) A plug-in should not allocate large continuous buffers such as the intensity map that your patch introduces. But then, the plug-in already uses a linear buffer for the source pixels. I don't know how feasible it would be to change it to work on the image on a tile-by-tile basis and to allocate the intensity map for a single tile only.
(In reply to comment #3) > As mentioned in HACKING, we don't use Tabs in the GIMP source code. You might > find tabs in some files, but you are encouraged to convert all tabs to spaces > when you see them. Ah, okay, just checking. Both the patch and source file are tab-free. > Some comments on the patch: > > (1) Adding parameters to PDB calls is unfortunately not backwards compatible > and we try not to break the PDB interface of scripts. It might be better to > introduce a new procedure. So you're saying, leave plug-in-oilify alone, and create e.g. plug-in-oilify-2 alongside it? That this one plug-in would provide two procedures? > (2) A plug-in should not allocate large continuous buffers such as the > intensity map that your patch introduces. But then, the plug-in already uses a > linear buffer for the source pixels. I don't know how feasible it would be to > change it to work on the image on a tile-by-tile basis and to allocate the > intensity map for a single tile only. The engine has to be able to read pixels within a given radius around a point, including when this point is at/near the edge of a tile. I haven't seen that there's a general mechanism to deal with this sort of non-locality; algorithms with this requirement all seem to just deal with the whole image at once.
Yes, you should create a new procedure (but don't call it oilify-2, we try to avoid numbers in procedure names). Then implement the old interface by means of calling the new extended oilify function. If you really need the whole drawable in a single memory region, well, then there's probably no way around that. There is however functionality in libgimp that helps you to avoid this. Have a look at gimppixelfetcher. It isn't the most effective mechanism though and should probably be extended to being able to fetch pixel regions instead of single pixels.
(In reply to comment #5) > Yes, you should create a new procedure (but don't call it oilify-2, we try to > avoid numbers in procedure names). Then implement the old interface by means of > calling the new extended oilify function. What should the new procedure be called? (I have zero familiarity with the applicable conventions---you have to tell me.) > If you really need the whole drawable in a single memory region, well, then > there's probably no way around that. There is however functionality in libgimp > that helps you to avoid this. Have a look at gimppixelfetcher. It isn't the > most effective mechanism though and should probably be extended to being able > to fetch pixel regions instead of single pixels. The entire drawable doesn't _need_ to be in memory---for a given region in the output image, you'd just need the same region in the input image plus a border of mask_size/2 thickness---but with the code as formidable as it is now, there's no way I'm adding all the region-bookkeeping necessary to make that work. A better approach is needed, and yeah, GimpPixelFetcher ain't it :]
Have a look at the Procedure Browser. A few plug-ins already register multiple procedures. Perhaps plug-in-oilify-enhanced would do?
It's unfortunate that there hasn't been any further feedback here. The patch contains a number of useful improvements. The intensity map for example would probably give a nice speed-up. If we would only create a source map for the region that is being processed, that would also be a reasonable memory requirement. It would be nice if someone could split the optimizations out of this patch and apply them.
Created attachment 89863 [details] Up-to-date source file Okay. This has SVN changes since last year merged in, and the plug-in now provides two procedures: plug-in-oilify (same as the current one), and plug-in-oilify-enhanced, which has a slightly different [and more appropriate] order of arguments. This code compiles without warnings, and runs without a peep from Valgrind. What has yet to be tested is the procedure interface, because I have nothing to do with script-fu. I'd like to request thorough feedback on this, from the technical to the stylistic. The hard work's been done here, and all that remains are loose ends.
Just some minor stylistic nitpicking: Please don't use (void) image_id; /* unused parameter */ You can use G_GNUC_UNUSED if you want to mark image_id as being unused. I also don't understand the condition in the progress update function: progress += dest_rgn.w * dest_rgn.h; if ((progress % 5) == 0) gimp_progress_update ((double) progress / (double) max_progress); You could end up with the progress update never been called here. Don't #include <math.h>, use libgimpmath/gimpmath.h instead.
(In reply to comment #10) > Please don't use > > (void) image_id; /* unused parameter */ > > You can use G_GNUC_UNUSED if you want to mark image_id as being unused. Heh. There's a glib macro for everything, isn't there? Will do. > I also don't understand the condition in the progress update function: > > progress += dest_rgn.w * dest_rgn.h; > if ((progress % 5) == 0) > gimp_progress_update ((double) progress / (double) max_progress); > > You could end up with the progress update never been called here. Well, assuming (dest_rgn.w * dest_rgn.h) is always the same amount, it'll eventually hit multiples of 5. IIRC, the intent was to prevent the function from being called at every iteration---but since this is in an outer loop, the overhead shouldn't even be an issue. I'll remove the condition. > Don't #include <math.h>, use libgimpmath/gimpmath.h instead. Okay, will switch that over. By the way: One annoying aspect of the plug-in's behavior is the latency of the preview update in the dialog interface---you frob a setting, and get hourglassed for, like, five seconds while it generates the preview. Is there a straightforward way of avoiding this (short of switching off preview) or is that more of an architectural issue with the plug-in interface?
Suppressing progress updates is good. I just failed to see why the pixel count will eventually hit multiples of 5. After thinking about it for a while I have to agree that this is a valid assumption. But there are less obfuscated ways to achieve this. If the preview takes too long, then that's a problem with the plug-in, not with the plug-in interface. The preview function absolutely needs to complete in less than a second.
(In reply to comment #12) > Suppressing progress updates is good. I just failed to see why the pixel count > will eventually hit multiples of 5. After thinking about it for a while I have > to agree that this is a valid assumption. But there are less obfuscated ways to > achieve this. Okay, I've replaced the code in question with this bit: { /* Don't call gimp_progress_update() more than 100 times */ gdouble percentage; progress += dest_rgn.w * dest_rgn.h; percentage = (gdouble) progress / (gdouble) max_progress; if (percentage > prev_progress_percentage + 0.01) { gimp_progress_update (percentage); prev_progress_percentage = percentage; } } Should do the trick. (Is 100 times enough?) > If the preview takes too long, then that's a problem with the plug-in, not with > the plug-in interface. The preview function absolutely needs to complete in > less than a second. It's mainly a problem with larger mask sizes. For every pixel in the output image, the code has to iterate over a square of size (mask_size)x(mask_size) pixels. With a mask size of 8 (the default), it takes about a second. Above that, however, more clever optimizations are going to be needed to keep the run time down. As far as the plug-in interface goes: Right now, the problem is that the widgets become unresponsive while the preview is being generated (with hourglass and all). Longer preview updates would be less of a problem if gimp_preview_invalidate() could cancel an in-progress update and start a new one, allowing the UI to remain responsive. Is there a way to do this with things as they are now?
If your code to suppress progress updates turns out to become that complex, better remove it. There's code in libgimp that does this anyway. No, there's no way to cancel a running update because your plug-in doesn't return control to the main loop until it is done preparing the preview. But calculating a small preview should not take that long. If it does, then the preview function either needs more optimization or you need to trade quality of the preview for speed.
(In reply to comment #14) > If your code to suppress progress updates turns out to become that complex, > better remove it. There's code in libgimp that does this anyway. Do you mean to say, doing this in the plug-in is superfluous? The best thing, of course, would be for gimp_progress_update() to do its own throttling; is that what's going on now? > No, there's no way to cancel a running update because your plug-in doesn't > return control to the main loop until it is done preparing the preview. But > calculating a small preview should not take that long. If it does, then the > preview function either needs more optimization or you need to trade quality of > the preview for speed. Optimization is a different can of worms. I have some ideas on this, but it's not at all straightforward, and will probably need a good hashing-out on the list. As far as this patch goes, however, the performance is no worse than what's already in SVN. Are there any plug-ins that do the main-loop thing in the middle of a preview, however? I'd like to see how this would be implemented.
The discussion here goes way beyond what belongs into the bug-tracker. If you want to discuss such general questions, please use the mailing-list.
Created attachment 90081 [details] oilify.c: final draft candidate It would belong in a new bug, anyway. This one has enough changes already. Anyway, all the review issues you pointed out are addressed here. Anything else needing attention, in this patch?
2007-06-18 Sven Neumann <sven@gimp.org> * plug-ins/common/oilify.c: applied patch from Daniel Richard G. which adds a new PDB entry with more options and improves speed and quality of the algorithm (bug #347075).