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

[Fredrik Höglund]

fredrik added inline comments.
Restricted Application edited projects, added KWin; removed Plasma.

INLINE COMMENTS

> blur.cpp:30
>  #include <QLinkedList>
> +#include <QtMath>
>  

Is this still needed when qPow() is not used?

> anemeth wrote in blur.cpp:145
> This texture is only used by `copyScreenSampleTexture()`
> It could very well be a separate variable, but I just appended another texture to the textures vector and used that instead.
> The reason for first rendering into m_renderTextures.last() and then copying that to m_renderTextures[0] is to eliminate what I call "extended blur".
> Extended blur is when windows or other elements that are not under the blurred area affect the blur effect. This sounds great in theory, and this is how the old blur method worked as well, but it becomes a big issue when the blur radius gets big. For example when you maximize a white window, the completely transparent taskbar also becomes almost completely white because of this effect, so the way I achieve this is by creating a GL_CLAMP_TO_EDGE effect.
> F5656755: c3_clamping.png <https://phabricator.kde.org/F5656755>
> For example if we want to blur a window (red) we have to blur a bigger area. To avoid extended blur I use the copySample shader to create a GL_CLAMP_TO_EDGE effect (blue) when copying the texture from m_renderTextures.last() to m_renderTextures[0]
> 
> Ideally we could specify to only disable extended blur on the taskbar, but I don't see a way to identify a window as the taskbar.

Okay, I see what you mean. I think this is a trade-off between two undesirable effects though, because the clamping causes abrupt changes near the edges of the blurred region when a window is moved. You can see this effect pretty clearly in the video you attached when the blur strength is set to strong. It really comes down to what you think is worse though.

But my concern here is that a fullscreen texture consumes at least 8 MB of VRAM with an HD monitor, and 32 MB with a 4K monitor.

I think it would be better to copy the framebuffer to m_renderTexture[0], and apply the clamping as a post-processing effect, using m_renderTexture[0] as both the source and destination, and targeting only the region outside the red rectangle for rendering. Using the same texture as both the source and destination is allowed in OpenGL when GL_ARB_texture_barrier or GL_NV_texture_barrier is supported, but unfortunately not in OpenGL ES.

The taskbar (and other panels) can be identified by calling EffectWindow::isDock().

> anemeth wrote in blur.h:41
> I don't know why it's 5
> This was in the old blur too: https://github.com/KDE/kwin/blob/master/effects/blur/blur.cpp#L503
> I just move this number to a variable.

It looks like that was added by Martin in https://phabricator.kde.org/R108:111db93e05a55496e7f13788207ead008bac80db.

> fredrik wrote in blurshader.h:53
> The first letter in enums should be capitalized. I think SampleType would also be a better name.

The first letters in the enumerators should also be capitalized.

> kwinglutils.h:477
> +     **/
> +    static void setRenderTargets(QStack <GLRenderTarget*> targets);
> +

This assumes that s_renderTargets is empty when this function is called, which might not be the case.
How about naming it pushRenderTargets(), and have it add the targets to s_renderTargets instead of replacing it?

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/20180118/5b6054c8/attachment-0001.html>