[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.EditedWed, Jan 23, 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.Wed, Jan 23, 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.Wed, Jan 23, 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.Wed, Jan 23, 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.EditedWed, Jan 23, 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.EditedWed, Jan 23, 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.Fri, Jan 25, 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.Fri, Jan 25, 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.Sat, Jan 26, 3:43 PM

Use sRGB textures for blur

anemeth added a comment.EditedSat, Jan 26, 3:46 PM

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

zzag added a comment.Sat, Jan 26, 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.EditedSun, Jan 27, 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.Sun, Jan 27, 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.Sun, Jan 27, 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.EditedSun, Jan 27, 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.Sun, Jan 27, 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.Sun, Jan 27, 8:50 PM
  • Handle GLES

@zzag @fredrik what do you think now?

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

Updated the summary and the screenshots

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

Looks good to me.

This revision is now accepted and ready to land.Thu, Jan 31, 1:21 PM
zzag added a comment.Thu, Jan 31, 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.Thu, Jan 31, 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.Thu, Jan 31, 4:36 PM
This revision was automatically updated to reflect the committed changes.