GNOME Bugzilla – Bug 683076
Change the shader of the blur filter effect
Last modified: 2013-08-05 19:24:13 UTC
The blur filter is super broken right now. It wants to be a box blur, but it samples at the wrong locations, and samples the middle texel twice, causing various artifacts. Box blurs are also not that good, but if you run it a few times, it sucks less. Run it a few times so it will suck less.
Created attachment 223010 [details] [review] blur-effect: Fix the blur filter Make sure we don't sample the center twice, and don't sample things that aren't our immediate neighbors.
Created attachment 223011 [details] [review] blur-effect: Run the blur filter three times This gives us a much better approximation of a gaussian blur.
fixing the blur effect shader has been a long standing issue, so thanks for looking into that. I'm not entirely sure that binding the same Snippet to the same layer of a texture is going to work, though - will need to get a review from Neil. also, the box-blur-to-approximate-a-gaussian approach usually works by separating the horizontal from the vertical pass, i.e.: - 1st pass: horizontal - 2nd pass: vertical see, for instance: http://www.gamerendering.com/2008/10/11/gaussian-blur-filter-shader/ and the considerations of the SVG blur filter: http://www.w3.org/TR/SVG/filters.html#feGaussianBlurElement there's some (C++) code in Mozilla's gfx layer for box blur on the CPU side: https://github.com/mozilla/mozilla-central/blob/master/gfx/2d/Blur.cpp that implements the SVG spec. there's also a branch in git written by Neil that changes the blur shader to a gaussian blur, but it was never committed - mostly because it does introduce a measurable performance loss, as far as I remember.
I think the first patch makes sense if we want to stick with the box blur. The second patch looks wrong though. The snippet recalculates the texel sample value for that layer without referencing the previous value. If you add the same snippet three times it's just going to do the work three times without any difference in the appearance. The gaussian blur patch is here: http://git.gnome.org/browse/clutter/commit/?id=0627bb65089e I think it won't necessarily be a performance loss and could potentially be a performance win because it will do less samples for the same radius. Also it will cache the results of the first pass. However because it needs to allocate a second framebuffer it will use more memory. I don't have a good idea whether that is a major problem or not. I haven't measured the performance difference.
I've rebased the patch on master: http://git.gnome.org/browse/clutter/commit/?id=c16b31e565e9
since, AFAIR, the goal is to add this blur to the whole screen, a gaussian blur may be a tad too hard on the GPU. one of the ideas that were floating around since the dawn of time was to have a :quality property for the BlurEffect, using: CLUTTER_BLUR_FASTER CLUTTER_BLUR_BETTER as valid values. we could add a BEST value and use it for the gaussian blur, in case we add a two-pass box blur - or we could add CLUTTER_BLUR_HARDER and CLUTTER_BLUR_STRONGER. I agree that at least the first patch should be committed in the interim, though.
Comment on attachment 223010 [details] [review] blur-effect: Fix the blur filter Attachment 223010 [details] pushed as fd375a7 - blur-effect: Fix the blur filter
Review of attachment 223011 [details] [review]: This is horribly wrong and an embarrassing view into the impaired judgment of my sleep deprived condition. Let it be seen no more.