Thank you for this op, as it is I think this operation still needs work before we include it in the default install, perhaps we could stick it in the workshop folder for now so that more people can opt-in to playing with and improving it before adding it among the ops in common.

First, the copyright statements in the top should be changed to your email and name, and we in GIMP/GEGL code the coding style is to prefer spaces over tabs
in files.

On small images the results are nice, fast good and interesting - however on really big images, the operation ends up allocating a temporary linear buffer the size of the image, which might be optimal for speed on small images, but for high resolution imagery.. this big memory allocation followed by blocking processing makes it not well suited for interactive use, for instance in GIMP - where one might run out of memory, the ui freeze up and work be lost.

Interspersed with the processing, some (infrequent) calls to gegl_operation_progress would make it less blocking.

From quickly looking over the implementation, it looks like it is traversing the image by scanlines and by pixel columns in multiple passes. Maybe using gegl_buffer_get() with rectangles corresponding to scanlines / pixel columns would work well for this, fetching a scanline/column are optimized cases for GeglBuffer.

If the operation would produce correct results with either an extra neighbourhood of radius, or radius * 4, the filter would be able to use the automatic multi-threading that is being opted out of with operation_class->threaded = FALSE;

Thank you very much for your feedback Øyvind Kolås.

We are working to fix the problems you mentioned and we will submit a new patch soon.

With regards to the automatic multi-threading, unfortunately there is no way to use it in this operation. The domain-transform is being implemented as a recursive filter; each pixel depends on all pixels in the same line or column. Further more, the filter must be applied several consecutive times. There is a way to parallelize the domain-transform, but it requires a more complex implementation.

However, we will avoid allocating a temporary linear buffer the size of the image, by using GeglBuffer functions, as you suggested.

Thanks,
Felipe

Created attachment 367734 [details] [review]
New Patch v2

Hello,

I've submitted a new patch in which I fixed the following items:

- Moved the operation to the workshop
- Changed copyrights
- Replaced tabs by spaces
- Avoided big memory allocations
- Used gegl_operation_progress as requested

Now, the operation is not allocating a buffer which has the size of the original image. Instead, the op is using a buffer of size DIMENSION * BLOCK_STRIDE * 4, where DIMENSION is image_width or image_height (the bigger one) and BLOCK_STRIDE is being defined as 1. This new approach worsens filter performance a bit, since the domain-transforms are being calculated dynamically.

About gegl_operation_progress calls, we noticed that for normal images (images that are not very big) the filter runs almost instantly. So, when using the filter inside GIMP with these images, the progress bar would not be necessary. We used glib's timer to call gegl_operation_progress only when necessary, i.e. only when the operation is taking too long.

Waiting for your feedback

Thanks

Since the operation has been moved to the workshop I will push the commit before finding time to play around more with it. In the workshop it more easily gets wider testing. 

One thing I do wonder, is whether 'domain-transform' is a name that will make sense for regular operators of GIMP, or if we are adding yet another rather technical implementation name like "bilateral filter" or "unsharp mask" that at least confuse new users.

The original paper lists more applications than smoothing/denoising :) How about a more specific name such as "Smoothing by Domain Transform" or smth like that?

Hello,

Thanks for pushing the commit!

With regards to the filter name, yes, maybe a more specific name would fit better. "Smoothing by Domain Transform" seems to be a good one :)

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gegl/issues/61.