Added the option to turn on noise behind the blurred area.
The lowest strength value disables it completely, so it is optional and is disabled by default.
Details
Details
- Reviewers
fredrik - Group Reviewers
KWin VDG - Commits
- R108:cc0325af4152: Added noise blur effect
Edit: this new screenshot shows the updated noise generation.
Edit2: separated the screenshots so you can flick through them to clearly see the differences
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
| effects/blur/blur.cpp | ||
|---|---|---|
| 590 | Using a screen-sized texture for this seems excessive. I suggest creating a 32x32 or 64x64 texture, and setting the wrap mode to GL_REPEAT. I also suspect that the pixels are too small on a 4K monitor for the effect to be noticeable. It would probably be better to use a scalable noise pattern, such as Perlin or simplex noise: https://en.wikipedia.org/wiki/Perlin_noise It might also be faster to generate the noise value for each pixel in the shader, based on the fragcoord. | |
| effects/blur/blurshader.cpp | ||
| 215 | This only needs to be done once after the shader has been linked. | |
| 382 | Always adding the noise value will make the pattern invisible when blurring a white area. Could this shader also do the final upscaling, so the two operations are combined? | |
Comment Actions
- Reduced noise texture size to 128x128 and added GL_REPEAT wrap mode
- Now works properly with screen scaling
- Now the glUniform1i only run once
- Corrected noise generation to only make the pixel color darker, not change the color
| effects/blur/blurshader.cpp | ||
|---|---|---|
| 382 | Fixed invisible problem. Combining upscaling and noise would add too much complication. | |
Comment Actions
@fredrik: I think with perlin noise blurred background will be foggy/cloudy + simplex noise is covered by a patent.
IMHO, noise should be a separate effect.
Comment Actions
Noise only makes sense to me when blur is enabled so it should be bundled together with blur.
Comment Actions
Honestly, I don't see any sense behind adding noise. Putting together blur and noise under "blur" is wrong, isn't it?
Comment Actions
If this makes sense, then it makes sense to have it here in the Blur settings - or even to set some sensible default value that makes a nice blur effect and not expose yet another slider, because honestly I can't even see any difference between any of the images presented thus far.
Comment Actions
Ah, I see now.
Can you explain why one would want to introduce noise here? To my eyes, the more noise, the less good it looks.
Comment Actions
This may be just personal preference.
Recently Windows 10 with their fluent design introduced noise too.
It looks really good in my opinion. Maybe not on the highest settings, but on 7/15 it looks subtle enough. If you want I could decrease the maximum noise value to 10/15.
I implemented the new blur because other OS and DEs had better blur than Plasma and I wanted it to not fall behind. This noise is similar too in that sense.
Here is an example of Windows 10 and Plasma side by side, using the noise:
Millions of people are using it and getting used to it, surely they would like to see a similar thing in Plasma, since they are familiar with it.
Comment Actions
Noise is often used to break the gradient visual artifacts that appear due to screens' inability to show colors precisely. You often get the impression there are lines orthogonal to the gradient direction which ruins the gradient.
This is one of the reasons wallpapers in the 4.x series which were full of big gradients always had some level of noise in them (more precisely, they had two slightly off gradients mixed with a noise map).
It is also good when mimicking the real world - you usually have a brushed glass if you want to blur objects behind it - and brushed glass provides noise.
Comment Actions
I see what you mean now, and looking closely, I can see how adding a small amount of noise can make a blur look better. Now that I know what I'm looking for, with no noise, I can see those unattractive lines.
However, I believe this is a situation where we should choose an optimal level of noise and hardcode it into the blur effect, rather than making it optional with a slider. The amount of blur shown in your screenshot with the slider on tick 6 looks about perfect to me. With none, can see lines, and with any more noise than that, it starts to look too "noisy" and looks like a rendering problem.
Comment Actions
Maybe hardcoding is a good idea, I have nothing against that. Though, I'm not sure hardcoding to 6 is the best one - I'm torn between that one and the next :)
Comment Actions
@ngraham @ivan
My favourite noise value is 6 on the slider as well and after investigating what Windows 10 uses I found it's around 6 as well.
I specifically say 6 on the slider because the slider goes from 0 to 14 so 6 actually is 5.
If the slider has to go at all costs than that is the value I'm comfortable with hardcoding.
HOWEVER at least there should be a checkbox to enable/disable it.
Should it be enabled by default?
Comment Actions
Since Blur is itself an optional feature, I would approve of adding noise by default, hardcoded to level 5/6). Basically we want to make sure that if you use blur, it looks as good as possible. Now that I've seen the lines on the noiseless blur, I can't un-see them! :)
Comment Actions
I'm in general against too many options. But in this case I think the noise slider is fine as long as it does not complicate the code too much (it doesn't seem so from a quick glance). Only advanced users will touch this settings dialog anyway. Just as others have already recommended, set the Noise strength default value directly to 6.
Comment Actions
For now I just set the noise default to 6/15
Looks like it's not an easy choice to hardcode it.
| effects/blur/blur.cpp | ||
|---|---|---|
| 239 | Yeah, you're right, scaling factor is shared among all displays, which sucks ofc. Mark it as done. | |
Comment Actions
Yeah, you're right, scaling factor is shared among all displays, which sucks ofc. Mark it as done.
That's not true on wayland; and in fact that will completely clash with the wayland scaling as you're using a value that is X specific, and we even hide the UI to set this config value on wayland.
This needs some code to change.
On wayland, see the screen object for the scale factor, but you probably don't need it. We have a separate concept of compositor and device co-ordinates. All this is in compositor co-ordinates so should "just work" on multi screens with multi DPI magically (assuming my code is correct)
For X, I see what you're trying to do, makes sense in principle. Though given it can't work with multiple screens and we have these problems with many other pixel space things personally I wouldn't bother.
You do have another option that avoids config loading and having multiple code paths; take the qApp->logicalDPI and divide it by 96.0. It should roughly be the same thing.
Comment Actions
Changed how the screen dpi is calculated.
@davidedmundson How does Wayland handle DPI then? Is it assumed automatically based on the resolution and display size?
What if I have a 40" 4k monitor and want to use it with 96 DPI or 192 DPI or even 1.5 scaling factor?
Comment Actions
How does Wayland handle DPI then? Is it assumed automatically based on the resolution and display size?
What if I have a 40" 4k monitor and want to use it with 96 DPI or 192 DPI or even 1.5 scaling factor?
I phrased it confusingly.
There is a UI config for it, it's managed by kscreen.
But it is a completely different UI with different keys than the one you were using due to the different single vs multi issue.
Comment Actions
@fredrik: I think with perlin noise blurred background will be foggy/cloudy + simplex noise is covered by a patent.
This is what Perlin noise looks like at a smaller scale if you are basing that assumption on the wikipedia image:
| effects/blur/blur.cpp | ||
|---|---|---|
| 595 | I suggest using QImage::Format_Grayscale8 or Alpha8 when the channels have the same value. | |
| effects/blur/blurshader.cpp | ||
| 382 |
Subtracting reverses the problem though, making the effect invisible over black backgrounds. I would subtract 0.5 from the noise value before adding it instead, so the range becomes [-0.5..+0.5] instead of [0...1].
Doing both in one pass is usually faster though, because you only rasterize half as many pixels. Isn't combining the passes a matter of copying the code from the main function in the upscaling shader, and adding the noise value after dividing the sum by 12? The blur effect would just need to switch from the upscale shader to the noise shader before doing the final pass. | |
Comment Actions
- Set the noise image format to QImage::Format_Grayscale8
- Added QImage::Format_Grayscale8 and QImage::Format_Alpha8 to kwingltexture.cpp because they were missing
- Increased the noise image size to 256x256 to reduce the visible tiling effect
- Fixed the invisible effect problem (this time for sure)
- Combined the noise shader with the upscale shader, so it can do both in one pass
- Fixed scaling problems, only works on integer scaling, else the noise image would be distorted
- Noise texture now follows the window
| effects/blur/blur.cpp | ||
|---|---|---|
| 150 | Should this be m_renderTargetStack? | |
| 594 | With the image being 8-bit, it would make more sense to manipulate the data directly, i.e.: uint8_t *pixels = (uint8_t *) noiseImage.scanLine(y);
for (int x = 0; x < noiseImage.width(); x++) {
pixels[x] = qrand() % m_noiseStrength;
} | |
| 605 | Does scaling the texture coordinates not work? You know I always worry about memory usage :) | |
| 684 | The noise texture is technically not a target. | |
| effects/blur/blurshader.cpp | ||
| 391 | This still subtracts the noise value. See the first part of my comment above. | |
| libkwineffects/kwingltexture.cpp | ||
| 128 | Please make these values identical to Grayscale8. | |
| effects/blur/blur.cpp | ||
|---|---|---|
| 150 | Whoops, you're right. Fixed. | |
| 605 |  According to this formula the 256x256 grayscale texture would take 64KB and the 2x scaled 512x512 texture would take 256KB | |
| 684 | Separated the 2 texture sizes into 2 functions | |
| effects/blur/blurshader.cpp | ||
| 391 | Yes, it substrats (darkens) the noise, but this time I added an abs() function to it. Before | After
----------------
100 95 (substracted normally)
10 5 (substracted normally)
8 3 (substracted normally)
5 0 (substracted normally)
4 1 (4 - 5 = -1 ---> abs(-1) = 1)
2 3 (3 - 5 = -3 ---> abs(-3) = 3)
0 5 (0 - 5 = -5 ---> abs(-5) = 5) | |
| effects/blur/blur.cpp | ||
|---|---|---|
| 605 | I could not get it working with scaling the coordinates without distortion or glitches. | |
Comment Actions
Aside from the darkening issue, I think this looks good now.
| effects/blur/blur.cpp | ||
|---|---|---|
| 605 | Alright, fine :) | |
| effects/blur/blurshader.cpp | ||
| 391 | Right, I missed the abs(). The downside though is that the stronger the noise, the more the brightness of the image is reduced. If you are concerned about the cost of an additional subtraction, you can avoid that by changing the texture format to GL_R8_SNORM. That's a GL 3.1 feature though, so it would need a fallback path. | |