After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 347075 - More Oilify plug-in enhancements
More Oilify plug-in enhancements
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Plugins
git master
Other All
: Normal enhancement
: 2.4
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2006-07-10 04:58 UTC by Daniel Richard G.
Modified: 2007-06-18 06:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
The aforementioned patch (27.55 KB, patch)
2006-07-10 05:03 UTC, Daniel Richard G.
reviewed Details | Review
The updated source file (26.53 KB, text/plain)
2006-07-10 05:05 UTC, Daniel Richard G.
  Details
Up-to-date source file (28.10 KB, text/plain)
2007-06-13 02:50 UTC, Daniel Richard G.
  Details
oilify.c: final draft candidate (28.01 KB, text/plain)
2007-06-16 17:21 UTC, Daniel Richard G.
  Details

Description Daniel Richard G. 2006-07-10 04:58:13 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.
Comment 1 Daniel Richard G. 2006-07-10 05:03:32 UTC
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.
Comment 2 Daniel Richard G. 2006-07-10 05:05:51 UTC
Created attachment 68699 [details]
The updated source file

By the way, are tabs kosher?
Comment 3 Sven Neumann 2006-07-10 06:20:15 UTC
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.
Comment 4 Daniel Richard G. 2006-07-10 06:36:24 UTC
(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.
Comment 5 Sven Neumann 2006-07-13 08:35:06 UTC
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.
Comment 6 Daniel Richard G. 2006-07-14 05:53:59 UTC
(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 :]
Comment 7 Sven Neumann 2006-07-15 16:43:29 UTC
Have a look at the Procedure Browser. A few plug-ins already register multiple procedures. Perhaps plug-in-oilify-enhanced would do?
Comment 8 Sven Neumann 2007-05-09 16:25:27 UTC
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.
Comment 9 Daniel Richard G. 2007-06-13 02:50:24 UTC
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.
Comment 10 Sven Neumann 2007-06-13 07:31:16 UTC
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.



Comment 11 Daniel Richard G. 2007-06-13 14:01:39 UTC
(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?
Comment 12 Sven Neumann 2007-06-13 19:03:37 UTC
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.
Comment 13 Daniel Richard G. 2007-06-14 02:55:35 UTC
(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?
Comment 14 Sven Neumann 2007-06-14 06:03:20 UTC
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.
Comment 15 Daniel Richard G. 2007-06-14 23:34:42 UTC
(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.
Comment 16 Sven Neumann 2007-06-16 14:14:36 UTC
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.
Comment 17 Daniel Richard G. 2007-06-16 17:21:41 UTC
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?
Comment 18 Sven Neumann 2007-06-18 06:18:54 UTC
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).