[effects/blur] Update blur to be more natural
ClosedPublic

Authored by anemeth on Jan 18 2019, 11:39 PM.

Details

Summary

This gets rid of the dark area that may appear between very different colors by doing the blur in SRGB colorspace.
This is not enabled for GLES, and will use the previous blur type.

Test Plan

Before:

After:

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
anemeth created this revision.Jan 18 2019, 11:39 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 18 2019, 11:39 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
anemeth requested review of this revision.Jan 18 2019, 11:39 PM
anemeth retitled this revision from Update blur to Update blur to be more natural.Jan 18 2019, 11:42 PM
anemeth edited the summary of this revision. (Show Details)
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: VDG, KWin.
anemeth added a subscriber: ngraham.
anemeth updated this revision to Diff 49854.Jan 18 2019, 11:43 PM

Added wrong file

rooty added a subscriber: rooty.Jan 18 2019, 11:46 PM

a definite +1

looks a lot better and i really havent noticed any difference in speed/performance (i've lost barely 2 fps on low end hardware so... negligible imo)

anemeth edited the summary of this revision. (Show Details)Jan 18 2019, 11:46 PM

Wow, it certainly does look better! I know the example image is rather contrived to show the effect, but that effect is quite dramatic!

anemeth edited the test plan for this revision. (Show Details)Jan 19 2019, 12:15 AM
filipf added a subscriber: filipf.Jan 19 2019, 1:11 AM
zzag added a comment.EditedJan 23 2019, 6:59 PM

Good Lord, somehow I managed to get this review lost in my pile of emails.

What we are facing here is a classical problem of forgetting to convert non-linear sRGB to linear RGB and vice versa. In our case this level of inaccuracy is acceptable, KWin is not an image editor after all, so we don't have to be that accurate (though that's a desirable property).

Also, I don't think that's a right way to convert non-linear sRGB to linear RGB.

zzag added a comment.Jan 23 2019, 7:00 PM

I suggest to leave the blur effect as it is.

In D18377#398675, @zzag wrote:

I suggest to leave the blur effect as it is.

The visual effect is pretty hugely improved IMHO; could you elaborate on what changes @anemeth would need to make to render this patch merge-worthy?

zzag added a comment.Jan 23 2019, 7:29 PM

The visual effect is pretty hugely improved IMHO; could you elaborate on what changes @anemeth would need to make to render this patch merge-worthy?

Well, the blur effect should correctly convert sRGB to linear RGB, though I personally think the blur effect is good enough already. Also, I'd like to point out that an option to enable/disable the "accurate" blur is a no-go.

In D18377#398748, @zzag wrote:

I personally think the blur effect is good enough already.

It's definitely good already, but as the "before" screenshots point out, it suffers from a well-known and common problem that the area between two blurred color appears too dark with many blur implementations. This is a bug, and fixing it is basically a bugfix.

Also, I'd like to point out that an option to enable/disable the "accurate" blur is a no-go.

I agree that an option is undesirable.

zzag added a comment.Jan 23 2019, 8:56 PM
In D18377#398674, @zzag wrote:

Also, I don't think that's a right way to convert non-linear sRGB to linear RGB.

Okay, it looks like it's an approximation.

rapiteanu added a comment.EditedJan 23 2019, 9:07 PM
In D18377#398675, @zzag wrote:

I suggest to leave the blur effect as it is.

This blur affects some KDE parts, like KWin's edge shadow:


In the previous example the shadow should transition from transparent to light blue, not a dark dark brown shape with a shade of blue.

zzag added a comment.EditedJan 23 2019, 9:22 PM

Hmm, it looks like during each render pass we'll be wasting resources on "sRGB - linear RGB" conversions. Intermediate results should be in linear colorspace.


This blur affects some KDE parts, like KWin's edge shadow:


In the previous example the shadow should transition from transparent to light blue, not a dark dark brown shape with a shade of blue.

This is unrelated.

In D18377#398821, @zzag wrote:

Hmm, it looks like during each render pass we'll be wasting resources on "sRGB - linear RGB" conversions. Intermediate results should be in linear colorspace.

The hardware will do these conversions automatically if the texture format is GL_SRGB8_ALPHA8, and GL_FRAMEBUFFER_SRGB is enabled.

rooty added a comment.Jan 25 2019, 9:22 PM
In D18377#398675, @zzag wrote:

I suggest to leave the blur effect as it is.

the specifics need to be worked out, but leaving the blur effect as it is right now is a mistake

it's really unnatural and makes an inordinate number of people (even in the VDG group) shy away from dark themes and lower opacity values (with this blur effect in place, dark themes look a lot more natural)

zzag added a comment.Jan 25 2019, 9:56 PM

it's really unnatural

Is it really that unnatural? Is it really very noticeable? We haven't received any bug reports about that. In fact people praised the new blur effect. This issue was "noticed" only after a post on reddit. Though I'd like to point out that the rabbit hole goes much deeper. For example, Firefox/Google Chrome/Gwenview/Eye of GNOME/etc don't properly scale down images. Does it make the scaled down images look really unnatural? No.

Yes, that's not good to blur textures without converting colors to the linear color space, but please don't exaggerate the whole situation. Let's think rationally.

shy away from dark themes

That's not true.


Also, I'd like to point out that some of my comments above are no longer actual because Fredrik proposed more simpler solution. :-)

I tried experimenting what fredrik suggested without luck so far.
@fredrik @zzag is this the correct approach? This gives the exact same result as not using sRGB textures.

I tried experimenting what fredrik suggested without luck so far.
@fredrik @zzag is this the correct approach? This gives the exact same result as not using sRGB textures.

Basically yes, but it's important to keep in mind that the framebuffer contents are effectively sRGB even though the framebuffer format is not.
So in other words, GL_FRAMEBUFFER_SRGB must be disabled when doing the initial copy from the framebuffer to the first sRGB texture, so the gamma curve is not applied twice.

anemeth updated this revision to Diff 50329.Jan 26 2019, 3:43 PM

Use sRGB textures for blur

anemeth added a comment.EditedJan 26 2019, 3:46 PM

@fredrik swoops in and solves every problem!
how it looks with this diff:

zzag added a comment.Jan 26 2019, 4:18 PM

Basically yes, but it's important to keep in mind that the framebuffer contents are effectively sRGB even though the framebuffer format is not.
So in other words, GL_FRAMEBUFFER_SRGB must be disabled when doing the initial copy from the framebuffer to the first sRGB texture, so the gamma curve is not applied twice.

May it cause issues on GLES?

rooty added a comment.EditedJan 27 2019, 8:53 AM

hey guys so i don't understand any of what's being talked about here but i've noticed that with this latest patch, latte-dock turns very very dark, could this be a bug in latte-dock unrelated to this patch?

before:


after:

(the opacity is set to 50%, if i lower it to 5% it looks just as dark)

konsole's blur looks a lot better though
blur elsewhere varies - the panel looks almost identical to what it looked like before, except slightly darker, and the popups look the same as before (hence there's more of a contrast between panel and popup):

before:


after:

anemeth updated this revision to Diff 50373.Jan 27 2019, 11:26 AM

Fix taskbar blur

@rooty please test it again with this

In D18377#400445, @zzag wrote:

May it cause issues on GLES?

There are a number of issues with GLES unfortunately:

  • You have to enable sRGB rendering by specifying { EGL_GL_COLORSPACE_KHR, EGL_GL_COLORSPACE_SRGB_KHR } in the eglCreateWindowSurface() attributes.
  • GL_FRAMEBUFFER_SRGB is enabled by default, and can only be toggled when GL_EXT_sRGB_write_control is supported.
  • On GLES2 you also need GL_EXT_sRGB.

Given all this, it's probably best to limit this to desktop GL for now.

rooty added a comment.Jan 27 2019, 1:51 PM

@rooty please test it again with this

excellent - it works perfectly now

Given all this, it's probably best to limit this to desktop GL for now.

you mean just put a bunch of "if opengles do this else do that" in it?

what the important or blur on plasma when no use of it on any theme or application by default

rooty added a comment.EditedJan 27 2019, 2:13 PM

what the important or blur on plasma when no use of it on any theme or application by default

almost all the themes and a small but substantial number of applications do...

what the important or blur on plasma when no use of it on any theme or application by default

almost all the themes and a small but substantial number of applications do...

i know that but i mean breeze the official theme
and kde applications
we need yo use this magnificent blur effect
better than that
i hope we do this

zzag added a comment.Jan 27 2019, 5:03 PM

This revision is not a place to discuss where the blur effect has to be used. Please leave comments that are related to this change.

In D18377#400909, @zzag wrote:

This revision is not a place to discuss where the blur effect has to be used. Please leave comments that are related to this change.

sorry for that

anemeth updated this revision to Diff 50395.Jan 27 2019, 8:50 PM
  • Handle GLES

@zzag @fredrik what do you think now?

anemeth updated this revision to Diff 50579.Jan 30 2019, 10:01 PM
  • Add brackets to if for convention
anemeth edited the summary of this revision. (Show Details)Jan 30 2019, 10:38 PM
anemeth edited the test plan for this revision. (Show Details)

Updated the summary and the screenshots

fredrik accepted this revision.Jan 31 2019, 1:21 PM

Looks good to me.

This revision is now accepted and ready to land.Jan 31 2019, 1:21 PM
zzag added a comment.Jan 31 2019, 1:27 PM

Please re-title the change, it should look something like "[effects/blur] ...". Also, please remove the video link from the summary.

anemeth retitled this revision from Update blur to be more natural to [effects/blur] Update blur to be more natural.Jan 31 2019, 4:31 PM
anemeth edited the summary of this revision. (Show Details)
In D18377#402740, @zzag wrote:

Please re-title the change, it should look something like "[effects/blur] ...". Also, please remove the video link from the summary.

done

ngraham accepted this revision.Jan 31 2019, 4:36 PM
This revision was automatically updated to reflect the committed changes.

I am using Plasma 5.16, this patch probably can create TOO DARKISH results so the natural part of the commit is not valid and probably breaks users experience also:

Konsole fully transparent:

My transparent Konsole is also noticeably darker now, but it actually seems to me like the background color of the Konsole color scheme got somehow changed. It doesn't match the color in the regular color scheme anymore.

My transparent Konsole is also noticeably darker now, but it actually seems to me like the background color of the Konsole color scheme got somehow changed. It doesn't match the color in the regular color scheme anymore.

I get exactly the same darkish results with Latte Docks that are fully transparent but have enabled Blur, this is probably a bug in Blur effect

I just downloaded the KDE Neon 5.16 iso from the website, installed it and updated it
I can't reproduce the issue

ndavis added a subscriber: ndavis.Jun 16 2019, 5:29 PM

I am using Plasma 5.16, this patch probably can create TOO DARKISH results so the natural part of the commit is not valid and probably breaks users experience also:

Konsole fully transparent:

Are you sure this problem is not caused by using a dark background color in Konsole? It makes sense that the blurred area would be dark unless you set the transparency to 100%.

anemeth added a comment.EditedJun 16 2019, 5:46 PM

Could you please try lowering the noise strength all the way down?
Noise caused some problems very similar to this a few months ago but I """fixed it"""


Does the problem persist if you use 1x scaling ?

zzag added a comment.Jun 16 2019, 6:05 PM

I just downloaded the KDE Neon 5.16 iso from the website, installed it and updated it

You need an Intel graphics card to reproduce this bug.

In D18377#480824, @zzag wrote:

I just downloaded the KDE Neon 5.16 iso from the website, installed it and updated it

You need an Intel graphics card to reproduce this bug.

Yep, I am at Intel driver, scale is 1x, noise at blur is at minimum and problem still occurs

https://bugs.kde.org/show_bug.cgi?id=408790 I am not the only one with issue, the issue was reported also from Latte users that probably use Intel driver

In D18377#480824, @zzag wrote:

I just downloaded the KDE Neon 5.16 iso from the website, installed it and updated it

You need an Intel graphics card to reproduce this bug.

That's strange. I have Intel HD Graphics 520 and I'm not seeing this bug.

In D18377#480824, @zzag wrote:

I just downloaded the KDE Neon 5.16 iso from the website, installed it and updated it

You need an Intel graphics card to reproduce this bug.

Yeah, the problem is probably that the GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING is GL_LINEAR with the Intel driver, and that means that the final conversion from linear to sRGB is not happening in the upscaleRenderToScreen() pass.

We should probably not use sRGB textures when the driver reports that the framebuffer color encoding is linear.

However, it might also be a bug in the driver that it doesn't pick an sRGB format for the default framebuffer. We should discuss that with the Intel developers, but I would appreciate it if someone who has an Intel GPU could investigate this a bit further first. At the very least confirm that glGetFramebufferAttachmentParameteriv() in fact returns GL_LINEAR.

In D18377#480824, @zzag wrote:

I just downloaded the KDE Neon 5.16 iso from the website, installed it and updated it

You need an Intel graphics card to reproduce this bug.

Yeah, the problem is probably that the GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING is GL_LINEAR with the Intel driver, and that means that the final conversion from linear to sRGB is not happening in the upscaleRenderToScreen() pass.

We should probably not use sRGB textures when the driver reports that the framebuffer color encoding is linear.

However, it might also be a bug in the driver that it doesn't pick an sRGB format for the default framebuffer. We should discuss that with the Intel developers, but I would appreciate it if someone who has an Intel GPU could investigate this a bit further first. At the very least confirm that glGetFramebufferAttachmentParameteriv() in fact returns GL_LINEAR.

I can test if you want:

  1. Is there any command that can provide that information?
  2. Do you have an small testing app that I can compile and return to you the output?
zzag added a comment.Jun 17 2019, 6:54 AM

Please discuss this issue in https://bugs.kde.org/show_bug.cgi?id=408594, not in this review request.