D9848: Updated the blur method to use the more efficient dual kawase blur algorithm.

[Fredrik Höglund]

fredrik added a comment.
Restricted Application edited projects, added KWin; removed Plasma.


  I haven't looked at everything in detail, but in general I think this looks ready to land.
  
  Just a couple of nitpicks, and a question below.

INLINE COMMENTS

> anemeth wrote in blur.cpp:145
> Ok I realize now that the extension name begins with GL_ and this is the reason it didn't find them.
> BUT then I listed all the available extensions and since they are in alphabetical order I can easily check if texture_barrier is available.
> F5662968: Képkivágás2.PNG <https://phabricator.kde.org/F5662968>
> The red arrow is where they should be but they aren't present.
> So my point still stands.

> GL_ARB_texture_barrier is supported since OpenGL 4.4, but KWin uses 3.0
>  GL_NV_texture_barrier is supported since OpenGL 3.0 but it only works on Nvidia right?
>  What about AMD and Intel GPUs then?

The 3.1 backend can and does use some GL 4 extensions, but it must have a fallback path for when they are not available.

The NV prefix only tells you which vendor created the extension, not which vendors support it. The AMD drivers in Mesa have supported texture_barrier since 2011, and the i965 driver has supported it since 2015.

But as I said on IRC, don't worry about it if your driver doesn't support it. We can add a path that uses texture_barrier after this has landed.

> blur.cpp:255
> +    foreach (EffectWindow *window, effects->stackingOrder()) {
> +        window->addRepaintFull();
> +    }

Does calling effects->addRepaintFull() also work?

> blur.cpp:30
>  #include <QLinkedList>
> +#include <math.h> // for ceil()
>  

Nitpick, but it should be <cmath> and std::ceil() in C++.

> blur.cpp:190
> +     *
> +     * The minOffset variable is the minimum offset value for an iteratoin before we
> +     * get blocky artifacts because of the downsampling.

Typo: iteratoin

> kwinglutils.cpp:1115
> +        targets.removeFirst();
> +    }
> +

You could also check if s_renderTargets is empty, and just do s_renderTargets = targets when that's the case.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D9848

To: anemeth, #plasma, #kwin
Cc: luebking, broulik, romangg, zzag, anthonyfieroni, mart, davidedmundson, fredrik, ngraham, plasma-devel, kwin, #kwin, iodelay, bwowk, ZrenBot, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180121/11147237/attachment.html>