GNOME Bugzilla – Bug 721396
Merge new Gaussian Blur
Last modified: 2015-05-23 10:41:29 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).
*** Bug 721330 has been marked as a duplicate of this bug. ***
*** Bug 719880 has been marked as a duplicate of this bug. ***
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.
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.
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?
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.
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?
(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)
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"
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
No additional issues seems to have cropped up, closing bug.