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 721396 - Merge new Gaussian Blur
Merge new Gaussian Blur
Status: RESOLVED FIXED
Product: GEGL
Classification: Other
Component: operations
git master
Other All
: Normal normal
: 0.3.0
Assigned To: Default Gegl Component Owner
Default Gegl Component Owner
: 721330 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-01-03 14:52 UTC by Daniel Sabo
Modified: 2015-05-23 10:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests fails with the new gaussian blurs. For test purpose only, do not apply (1.70 KB, patch)
2014-01-05 12:41 UTC, Téo Mazars
none Details | Review

Description Daniel Sabo 2014-01-03 14:52:20 UTC
Adding this bug to consolidate our various gaussian blur issues. Massimo has made most of a new non-buggy gaussian blur operation in the workshop directory. There are still a few issues to resolve that Téo is working on.

* Don't crash when asked for an infinite rectangle in IIR mode.
* Optionally expand the output to include the fringes of the blur (needed by drop shadow and friends, but undesirable when working with fixed sized layers).
Comment 1 Daniel Sabo 2014-01-03 14:54:18 UTC
*** Bug 721330 has been marked as a duplicate of this bug. ***
Comment 2 Daniel Sabo 2014-01-03 14:54:34 UTC
*** Bug 719880 has been marked as a duplicate of this bug. ***
Comment 3 Téo Mazars 2014-01-05 12:41:41 UTC
Created attachment 265366 [details] [review]
tests fails with the new gaussian blurs. For test purpose only, do not apply

The patch attached makes tests use the new gaussian blur implementation, but there are two things that does not work correctly right now:

1) clones.xml simply fails, I don't know why.

2) image-compare.xml fails when clip-extent is "TRUE" (it should not), the image-compare-diff shows only differences on the right and bottom edges, so it could be something on the edge handling?

Both tests use the IIR case.
Comment 4 Téo Mazars 2014-01-05 12:43:20 UTC
Also, Massimo told me he has a more accurate version of the IIR filter, perhaps it would be better to have it landed in master before updating the tests.
Comment 5 Téo Mazars 2014-01-05 13:03:07 UTC
This fixes the two issues of the bug description.

commit f714a2d33be8dee8879251664def46655d422558
Author: Téo Mazars <teomazars@gmail.com>
Date:   Sun Jan 5 13:28:11 2014 +0100

    workshop: add a way to enlarge the output extent for gaussian blur

 operations/workshop/gaussian-blur-iir.c |  4 ++
 operations/workshop/gblur-1d.c          | 88 +++++++++++++++++++++++++--------
 2 files changed, 71 insertions(+), 21 deletions(-)


Though I was not able to make it pass-through in the "IIR + infinite plane" case. Instead, it returns an empty buffer right now. Someone knows how to do that properly?
Comment 6 Téo Mazars 2014-01-05 16:43:59 UTC
After discussion with Massimo, we found that:

- clones.xml differences may come from two reasons : different extent sizes or the use of double instead of float (artifacts are visible with std-dev > 70 when using float).

- image-compare.xml failing may be due to the new handling of boundaries: http://hal.inria.fr/inria-00548616 .

- http://homepage.tudelft.nl/e3q6n/publications/1998/ICPR98LVTYPV/ICPR98LVTYPV.pdf should give more accurate results.

Conclusion: It sill needs some love.
Comment 7 Téo Mazars 2014-08-02 23:33:51 UTC
I though about it today and I think the work remaining should be considered non-blocker. Because the issue appears only on boundaries with IIR.

IIR, as the name suggests, is an approximation of the convolution with a Gaussian filter, and the new approximation used shows differences with the old one, on extent boundaries only. This doesn't mean that there is any regression. Understanding the differences is still something we need to do, but I personally trust more the new implementation than the old one, especially the iir part.

And even though this bug is assigned to me, I won't do it in the near futur... So I suggest we move the new implementation from ./workshop to ./common, udpate the two tests cases failing with new references, and leave this bug open. Note that the name is wrong: gaussian-blur-iir.c contains actually both iir and fir.

Opinion someone? Massimo?
Comment 8 Massimo 2014-08-03 12:15:46 UTC
(In reply to comment #7)
...
> 
> IIR, as the name suggests, is an approximation of the convolution with a
> Gaussian filter, and the new approximation used shows differences with the old
> one, on extent boundaries only. 
...

I'd say not on extent boundaries only, the op in workshop tries to fix 2
problems that are present in the op in operations/common and that are
examplified by the following composition (test.xml):

<gegl>
  <gegl:crop width='1024' height='1024'/>

  <!--
  <gegl:gaussian-blur-iir
  -->
  <gegl:gaussian-blur

    filter='iir' std-dev-y='180' std-dev-x='180'/>

  <gegl:crop width='1024' height='1024'/>
  <gegl:checkerboard x='320' y='320' format="RaGaBaA float"/>
</gegl>


1 are the artifacts caused by chunking the iir convolution, they're visible
  saving the output of this composition and observing its alpha channel 
  (for example, in GIMP, dragging the alpha channel from the channel
  dialog to the layer dialog)

2 is a matter of accuracy, gaussian-blur-iir uses double precision to
  perform the 2-step iir convolution, these artifacts are visible running

  GEGL_CHUNK_SIZE=$((1024*1024)) gegl -i test.xml

  with a gegl built with SDL (one shows a weird vertical pattern, 
  the other not)


BTW when running make check with GEGL_THREADS=4 set, clones.xml fails 
because of the different chunk size in gaussian-blur caused by 
multi-threading.

gblur-1d is also producing different results with multi-threading enabled
because multi-threading does not honour get_cached_region and so it should
implement in-place an appropriate multi-thread work splitting strategy
(per columns when working vertically and per rows when horizontally)
Comment 9 Téo Mazars 2014-08-03 13:30:01 UTC
Hmm, well, yes I was saying that the noticeable differences found on edges are not really expected given the fixes done. I mean, differences are expected but not this one.

I still suggest to make the move, update the references test and to disable the multi-threading in gblur-1d.c until it's fixed (or better yet directly fix the cached_region thing)

As in "it really fixes things without any major regressions but with some minor doubts on the right and bottom edges handling"
Comment 10 Øyvind Kolås (pippin) 2015-04-26 20:52:02 UTC
I've moved the new op out of workshop, not closing this bug quite yet due to the possibly unresolved issues.

commit a73037253476dca595a700d64f3c95aa877e3fad
Author: Øyvind Kolås <pippin@gimp.org>
Date:   Sun Apr 26 16:10:51 2015 -0400

    gaussian-blur: replace with the new one from workshop
Comment 11 Øyvind Kolås (pippin) 2015-05-23 10:41:29 UTC
No additional issues seems to have cropped up, closing bug.