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

Authored by anemeth on Jan 12 2018, 11:30 PM.

Details

Summary

Updated the old and outdated blur method to use the much more efficient dual kawase blur method.
Now with this we can do virtually infinite blur with very very little performance cost.
The dual kawase blur method is basically downscaling and upscaling an image, but combined with the kawase blur shader.
Comparison: https://i.imgur.com/mh6Cw61.png
Left is old, right is new.
Comparison was done with the strongest blur setting in a VM running on an Intel i7-4790 and a GTX980
We can see here that the performance is even better with this new method.

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
anemeth marked 13 inline comments as done.Jan 16 2018, 5:03 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 16 2018, 5:03 PM
anemeth updated this revision to Diff 25504.Jan 16 2018, 8:11 PM

Added a new function to GLRenderTarget called setRenderTargets
With this function I implemented the changes suggested by @fredrik to remove unnecessary gl calls.
We achieve this by pushing all GLRenderTargets at once.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 16 2018, 8:11 PM
anemeth marked an inline comment as done.Jan 16 2018, 8:12 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 16 2018, 8:12 PM
anemeth updated this revision to Diff 25511.Jan 16 2018, 11:46 PM

Added the .arcconfig file

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 16 2018, 11:46 PM
anemeth updated this revision to Diff 25551.EditedJan 17 2018, 5:41 PM

With one of my previous changes I accidentally introduced an extended blur effect.
Now it's corrected.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 17 2018, 5:41 PM
fredrik added inline comments.Jan 18 2018, 12:13 AM
effects/blur/blur.cpp
29

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

147

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().

effects/blur/blur.h
42

It looks like that was added by Martin in 111db93e05a55496e7f13788207ead008bac80db.

effects/blur/blurshader.h
53

The first letters in the enumerators should also be capitalized.

libkwineffects/kwinglutils.h
477 ↗(On Diff #25551)

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?

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 18 2018, 12:13 AM
anemeth updated this revision to Diff 25596.EditedJan 18 2018, 3:01 PM

Made the changes that were suggested by @fredrik
Now only the taskbar doesn't have the extended blur effect.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 18 2018, 3:01 PM
anemeth marked 6 inline comments as done.Jan 18 2018, 3:12 PM
anemeth added inline comments.
effects/blur/blur.cpp
147

Now only the taskbar doesn't have the extended blur effect.

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?
I guess mostly the older and weaker cards would be benefit from this, but the chance is higher that they don't support this extension.
Is it worth the hassle to only implement this for Nvidia users?

I guess it's an unpopular opinion, but it's "only" 8 MB and since cards these days have at least like 1 GB of VRAM it's not a big concern.

How do you suggest we handle this?

152

I'm working on a nicer solution with less magical numbers.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 18 2018, 3:12 PM
anemeth updated this revision to Diff 25615.Jan 19 2018, 12:13 AM

Updated the blur amount value configuration to use less magically numbers and added an explanation comment.
This new method creates an evenly distributed array of blur strength sampled from the minimum and maximum offset values per downsample iteration.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 19 2018, 12:13 AM
anemeth marked 2 inline comments as done.Jan 19 2018, 12:14 AM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 19 2018, 12:14 AM

Thanks for the patience going through this! Some reviews do go through a lot of iterations, but it's generally worth it.

I gave it a quick test. Blur part works nicely.

Changing the blur size at runtime didn't seem to have an effect, it all worked after kwin restart.
Your code looks fine, I can see m_downSampleIterations getting updated, but clearly not everything. Can you check that please.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 19 2018, 12:45 AM
anemeth added a comment.EditedJan 19 2018, 1:15 AM

Changing the blur size at runtime didn't seem to have an effect, it all worked after kwin restart.
Your code looks fine, I can see m_downSampleIterations getting updated, but clearly not everything. Can you check that please.

This exact problem is present in the old blur method as well.
In order to get around this problem (without restarting KWin of course) you have to interact with the blurred area, for example move a window near it, right click on it, etc. and it will be updated properly.
How do I force the windows to be updated?

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 19 2018, 1:15 AM
anemeth added inline comments.Jan 19 2018, 1:59 AM
effects/blur/blur.cpp
147

I checked if I have these extensions. Just to make sure it works I included another extension I know my card supports.

qCDebug(KWINEFFECTS) << "NV_texture_barrier:" << hasGLExtension("NV_texture_barrier");
qCDebug(KWINEFFECTS) << "ARB_texture_barrier:" << hasGLExtension("ARB_texture_barrier");
qCDebug(KWINEFFECTS) << "GL_NV_texgen_reflection:" << hasGLExtension("GL_NV_texgen_reflection");

And the output:

kwineffects: NV_texture_barrier: false
kwineffects: ARB_texture_barrier: false
kwineffects: GL_NV_texgen_reflection: true

If this works so unreliably I don't think we should use this.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 19 2018, 1:59 AM
anemeth added inline comments.Jan 19 2018, 2:13 AM
effects/blur/blur.cpp
147

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.


The red arrow is where they should be but they aren't present.
So my point still stands.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 19 2018, 2:13 AM
anemeth updated this revision to Diff 25638.Jan 19 2018, 1:03 PM

Fixed a bug where updating the blur strength would not update the blurred area.
With this fix every window gets updated when the blur strength is reconfigured.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 19 2018, 1:03 PM
anemeth updated this revision to Diff 25643.Jan 19 2018, 4:14 PM

Moved a shader variable to a uniform, so that the calculation can be done on the CPU instead.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 19 2018, 4:14 PM

I would like to see this reviewed by @fredrik, @mart , @davidedmundson and in general the VDG

@fredrik, @mart , @davidedmundson can you give it a review please?
I think it really matured over the last week and is ready for testing.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 21 2018, 12:23 PM

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.

effects/blur/blur.cpp
29

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

147

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.

192

Typo: iteratoin

229

Does calling effects->addRepaintFull() also work?

libkwineffects/kwinglutils.cpp
1115 ↗(On Diff #25643)

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

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 21 2018, 4:53 PM
anemeth updated this revision to Diff 25720.Jan 21 2018, 6:03 PM

Implemented the changes suggested by @fredrik

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 21 2018, 6:03 PM
anemeth marked 3 inline comments as done.Jan 21 2018, 6:04 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 21 2018, 6:04 PM
anemeth added inline comments.Jan 21 2018, 6:05 PM
effects/blur/blur.cpp
229

It does.
Changed to that.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 21 2018, 6:05 PM
fredrik added inline comments.Jan 22 2018, 7:45 PM
libkwineffects/kwinglutils.cpp
1104 ↗(On Diff #25720)

The reserve() call should be in the else {} clause.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 22 2018, 7:45 PM
anemeth updated this revision to Diff 25782.Jan 22 2018, 8:08 PM

Implemented the change suggested by @fredrik

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 22 2018, 8:08 PM
anemeth marked an inline comment as done.Jan 22 2018, 8:09 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 22 2018, 8:09 PM
fredrik accepted this revision.Jan 22 2018, 8:16 PM
This revision is now accepted and ready to land.Jan 22 2018, 8:16 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 22 2018, 8:16 PM
dos awarded a token.Jan 24 2018, 1:39 AM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 24 2018, 1:39 AM
dos added a subscriber: dos.Jan 24 2018, 1:39 AM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 24 2018, 1:39 AM
hein added a subscriber: hein.Jan 25 2018, 2:24 PM

I'll push this for you (I'm interpreting Martin's resignation as withdrawing objections, knock on wood).

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 25 2018, 2:24 PM
hein added a comment.Jan 25 2018, 2:38 PM

@anemeth The patch doesn't apply cleanly to current master. Would you mind rebasing it please? I don't want to introduce errors at this stage by editing code I didn't review.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 25 2018, 2:38 PM
anemeth updated this revision to Diff 25948.Jan 25 2018, 5:26 PM

Rebased to master.
There was a conflict with D9879
This change reverts those changes since it is superseded by this change because caching blur option has been
removed.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 25 2018, 5:26 PM
This revision was automatically updated to reflect the committed changes.
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 25 2018, 5:32 PM
zzag added a comment.Jan 25 2018, 5:37 PM

File mode bits are slightly off.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptJan 25 2018, 5:37 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJan 25 2018, 6:00 PM
zzag added a comment.EditedMar 26 2018, 4:30 AM

Why does this patch have unrelated changes? E.g. GLRenderTarget::attachTexture, GLRenderTarget::detachTexture, empty GLRenderTarget constructor, GLRenderTarget::setTextureDirty, etc? They aren't used anywhere.

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptMar 26 2018, 4:30 AM
In D9848#234268, @zzag wrote:

GLRenderTarget::attachTexture

This was not changed.
It was used by old blur and simple blur.
Looks like it's not used anymore.

GLRenderTarget::detachTexture

This was used by simple blur, but that has been removed in D10181
I left it in in case something might use it in the future.

empty GLRenderTarget constructor

Also was used by simple blur.

GLRenderTarget::setTextureDirty

I added this because on line (1101 <---> 1124) the disable() function that also sets the texture dirty was moved into a condition.
I don't know where the dirty parameter is used or checked, I didn't go look that far into it.

For functions that are not used feel free to create a patch that removes them.

zzag added a comment.Mar 26 2018, 3:22 PM

GLRenderTarget::setTextureDirty

I added this because on line (1101 <---> 1124) the disable() function that also sets the texture dirty was moved into a condition.
I don't know where the dirty parameter is used or checked, I didn't go look that far into it.

It does nothing pretty much. https://github.com/KDE/kwin/blob/master/libkwineffects/kwingltexture.cpp#L569

For functions that are not used feel free to create a patch that removes them.

No, I just asked why you added those changes, that's all. :)
Also, I'm not a KWin developer so that's not up to me to decide whether they should be removed. ;-)