After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 682536 - Lock screen - blur and desaturate the background
Lock screen - blur and desaturate the background
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: lock-screen
3.5.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 687873 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-08-23 13:07 UTC by Allan Day
Modified: 2012-12-17 19:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (1.39 KB, patch)
2012-08-23 13:08 UTC, Allan Day
none Details | Review
ScreenShield: use a combined blur and desaturate effect for the screenshield (13.20 KB, patch)
2012-08-29 20:13 UTC, Giovanni Campagna
none Details | Review
ScreenShield: blur and desaturate the screenshield background (2.68 KB, patch)
2012-08-30 00:40 UTC, Giovanni Campagna
committed Details | Review
MetaBackgroundActor: add a setter for GLSL uniforms (3.29 KB, patch)
2012-11-07 18:06 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: unbreak blurring the background (2.50 KB, patch)
2012-11-07 18:08 UTC, Giovanni Campagna
accepted-commit_now Details | Review
ScreenShield: unbreak blurring the background (2.70 KB, patch)
2012-11-19 19:07 UTC, Giovanni Campagna
committed Details | Review

Description Allan Day 2012-08-23 13:07:10 UTC
This will help to communicate that it isn't the "real" desktop.
Comment 1 Allan Day 2012-08-23 13:08:54 UTC
Created attachment 222221 [details] [review]
patch

Adding Giovanni's patch from bug 619955.
Comment 2 Giovanni Campagna 2012-08-23 13:21:34 UTC
The original patch was put on hold because on machines with slower GPUs (including my GM45, which is an intel gen 4 and it's just two and a half years old) made the animation choppy.

The technical problem is that ClutterOffscreenEffects are not made to be stacked, as each new effect requires the previous one to render to a FBO (which is slow), so you can have blur or effect, but not both, unless you write your
own GLSL snippet.
Much faster would be to paint the blurred and desaturated texture to a FBO and just paint that as the actor, but I'm afraid it's not trivial to write and would not work in general, so it would be a shell specific actor (or maybe an extension to MetaBackgroundActor).
Comment 3 Giovanni Campagna 2012-08-29 20:13:05 UTC
Created attachment 222839 [details] [review]
ScreenShield: use a combined blur and desaturate effect for the screenshield

Introduce and use a ShellBlurDesaturateEffect that combines the GLSL shaders
from ClutterBlurEffect and ClutterDesaturateEffect. This avoids rendering
the blurred background on a second FBO and improves the animation
smoothness.
Comment 4 Giovanni Campagna 2012-08-30 00:40:21 UTC
Created attachment 222902 [details] [review]
ScreenShield: blur and desaturate the screenshield background

The background is the same as the normal desktop, so we blur and
desaturate it to clearly show that it's not the normal system state.
To do so, we don't use standard ClutterEffects, to avoid the FBO
indirection. Instead, we take advantage of MetaBackgroundActor support
for GLSL code and paint the shaded background texture directly.

This is really the best we can do (except applying the effect in software and
uploading the result as a static texture). It's not always 60fps, but
it's smooth enough.
Depends on bug 669798.
Comment 5 drago01 2012-11-06 18:41:41 UTC
(In reply to comment #4)
>It's not always 60fps, but  it's smooth enough.

This means nothing without the information about the hardware used.
Comment 6 drago01 2012-11-06 18:43:30 UTC
(In reply to comment #5)
> (In reply to comment #4)
> >It's not always 60fps, but  it's smooth enough.
> 
> This means nothing without the information about the hardware used.

And screen resolution.

My point is if it just "smooth enough" on powerful hardware and/or low res it makes no sense to have that by default (not without optimization to make it usable on other hardware).
Comment 7 Giovanni Campagna 2012-11-06 19:11:43 UTC
Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller / Mesa DRI Mobile Intel® GM45 Express Chipset
AFAIK, it is the oldest generation supported by the i965 driver, which in turn is required to get the shell running on Intel cards (ie nothing running i915 or i810 has any chance of working, given that we hard require GL 2.1)
Generally speaking, modern AMD/Nvidia cards are more powerful, so this card (which sports a poor 10 cores clocked at 533 Mhz) can be considered a good representative for what we want to support.

Resolution is 1440x900, which is not HD/dual screen but is not a netbook either.
Comment 8 drago01 2012-11-06 19:16:33 UTC
(In reply to comment #7)
> Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller / Mesa
> DRI Mobile Intel® GM45 Express Chipset
> AFAIK, it is the oldest generation supported by the i965 driver, which in turn
> is required to get the shell running on Intel cards (ie nothing running i915 or
> i810 has any chance of working, given that we hard require GL 2.1)
> Generally speaking, modern AMD/Nvidia cards are more powerful, so this card
> (which sports a poor 10 cores clocked at 533 Mhz) can be considered a good
> representative for what we want to support.
> 
> Resolution is 1440x900, which is not HD/dual screen but is not a netbook
> either.

OK fair enough.
Comment 9 drago01 2012-11-06 19:18:53 UTC
Review of attachment 222902 [details] [review]:

Didn't test it but code looks good to me.
Comment 10 Allan Day 2012-11-06 19:44:14 UTC
I can't get it to apply, for some reason.
Comment 11 Giovanni Campagna 2012-11-06 21:53:15 UTC
Attachment 222902 [details] pushed as 4fd6903 - ScreenShield: blur and desaturate the screenshield background
Allan, you can now test it in master.
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-11-06 23:10:00 UTC
Do you even test the code you write?

1) You set the child to a new background actor, not the one that has the pipeline
2) Blur is based on the old broken ClutterBlurEffect code before I fixed it, but no biggie.
3) This is missing the pixel_step uniform, so the blur won't work, and there's no way to make the uniform work, outside of reinventing ClutterShaderEffect.

I'd prefer it if we could find a way to make Clutter not be stupid, and have stacking effects like this work without redirecting to FBOs all the time by having smart pipeline management, but I guess that requires Cogl 2.0.

The other thing we could do is assume that the background actor size doesn't change that often, and hardcode pixel_step in the shader, and regenerate it when useful.
Comment 13 Giovanni Campagna 2012-11-07 17:15:05 UTC
(In reply to comment #12)
> Do you even test the code you write?

Nah, it would be too easy :P

> 1) You set the child to a new background actor, not the one that has the
> pipeline

Ops, yes. I saw Cosimo fixed that already.

> 2) Blur is based on the old broken ClutterBlurEffect code before I fixed it,
> but no biggie.
> 3) This is missing the pixel_step uniform, so the blur won't work, and there's
> no way to make the uniform work, outside of reinventing ClutterShaderEffect.

This is a bigger problem actually.
The new "unbroken" ClutterBlurEffect is a separable Gaussian blur, and unfortunately separable means two step: first step you blur the thing you're drawing horizontally, second step you blur the intermediate texture vertically and draw on the screen. There is no way to get a real Gaussian blur without two textures (short of making it quadratic in sigma).

> I'd prefer it if we could find a way to make Clutter not be stupid, and have
> stacking effects like this work without redirecting to FBOs all the time by
> having smart pipeline management, but I guess that requires Cogl 2.0.

See above: at least one FBO is needed, although that's still better than two.

> The other thing we could do is assume that the background actor size doesn't
> change that often, and hardcode pixel_step in the shader, and regenerate it
> when useful.

That's even ugh.

I see three options here:
1) Fix pixel_step by extending the MetaBackgroundActor API with a uniform setter, get a box blur and be done with it.
2) Remove the blur altogether and be content with desaturation (which works)
3) Swallow the FBO cost and write a real GLSL two-step Gaussian blur. Probably depends on 1) to upload the Gaussian function uniform and the pixel step.
Comment 14 drago01 2012-11-07 17:24:05 UTC
(In reply to comment #13)
>
> I see three options here:
> 1) Fix pixel_step by extending the MetaBackgroundActor API with a uniform
> setter, get a box blur and be done with it.
> 2) Remove the blur altogether and be content with desaturation (which works)
> 3) Swallow the FBO cost and write a real GLSL two-step Gaussian blur. Probably
> depends on 1) to upload the Gaussian function uniform and the pixel step.

3) is simply not an option ... 4) would be do it in software and upload one texture (cache it so you don't have to do it multiple times for the same background).
Comment 15 Giovanni Campagna 2012-11-07 17:29:49 UTC
4) is even less of an option. If any, I'd rather do GL manually and cache the texture in the GPU (bypassing Cogl and Clutter), than do it in software.
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-11-07 17:41:04 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > Do you even test the code you write?
> 
> Nah, it would be too easy :P
> 
> > 1) You set the child to a new background actor, not the one that has the
> > pipeline
> 
> Ops, yes. I saw Cosimo fixed that already.
> 
> > 2) Blur is based on the old broken ClutterBlurEffect code before I fixed it,
> > but no biggie.
> > 3) This is missing the pixel_step uniform, so the blur won't work, and there's
> > no way to make the uniform work, outside of reinventing ClutterShaderEffect.
> 
> This is a bigger problem actually.
> The new "unbroken" ClutterBlurEffect is a separable Gaussian blur, and

It's actually still a cheap box blur. I just got rid of some incorrect math.

http://git.gnome.org/browse/clutter/commit/clutter/clutter-blur-effect.c?id=fd375a7bc962517439303203cf7443caaf729e93


Neil had a branch to fix it to be a real separable Gaussian blur:

http://git.gnome.org/browse/clutter/commit/?h=wip/gaussian-blur&id=c16b31e565e9acef713620131300cd8090607f69

Which I don't think we need yet. Running the box blur multiple times is enough to make it look like a Gaussian blur, and we can do that without another FBO, by looping tricks:

    uniform int box_blur_count;

    for (int i = 0; i < 3; i++) {
        if (i >= box_blur_count)
            break;
        /* sample texes, average out */
    }
Comment 17 Giovanni Campagna 2012-11-07 17:49:02 UTC
(By unbroken BlurEffect, I was referring to the branch, yet)

How would the repeated box blur work?
If you sample 3 times the same texels, and then divide by 27 instead of 9, you get the same value at the end.
What you want is sampling the first time the original texture, the second time the texture with a first convolution pass applied, so that nearby pixels are already averaged, and so on.
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-11-07 17:52:28 UTC
I'm a moron, disregard.
Comment 19 Giovanni Campagna 2012-11-07 18:06:52 UTC
Created attachment 228402 [details] [review]
MetaBackgroundActor: add a setter for GLSL uniforms

It doesn't make sense to GLSL shaders without uniforms.
Comment 20 Giovanni Campagna 2012-11-07 18:08:36 UTC
Created attachment 228404 [details] [review]
ScreenShield: unbreak blurring the background

It was missing a uniform, so it had no effect besides desaturation.

Ok, so this is option 1). It has a visible effect, and for releastic photos
that already have some amount of natural blurring it might be enough after all.
Comment 21 Jasper St. Pierre (not reading bugmail) 2012-11-07 18:22:14 UTC
As I said on IRC earlier, I really don't want cp clutter-shader-effect.c meta-background-actor.c.

Is there anything wrong with clutter_actor_set_shader / clutter_actor_set_shader_param, besides the fact that they're deprecated?
Comment 22 Giovanni Campagna 2012-11-07 18:32:55 UTC
*** Bug 687873 has been marked as a duplicate of this bug. ***
Comment 23 Giovanni Campagna 2012-11-11 17:43:22 UTC
(In reply to comment #21)
> As I said on IRC earlier, I really don't want cp clutter-shader-effect.c
> meta-background-actor.c.
> 
> Is there anything wrong with clutter_actor_set_shader /
> clutter_actor_set_shader_param, besides the fact that they're deprecated?

The fact they're deprecated is a good reason not to use them alone, but also cogl_use_program() does not work with the new CoglPipeline API, as every pipeline generates a shader internally.
Note that I'm not copying clutter-shader-effect.c, because I'm not creating textures or FBOs. I'm just punching through the MetaBackgroundActor API to make the custom shaders (which were introduced for another bug) more flexible.
Comment 24 Giovanni Campagna 2012-11-19 18:30:24 UTC
So what's the outcome here? Live happily with desaturation and some useless (and maybe even optimized away) sampling?
Comment 25 drago01 2012-11-19 18:32:48 UTC
(In reply to comment #24)
> So what's the outcome here? Live happily with desaturation and some useless
> (and maybe even optimized away) sampling?

No ... this is just broken .. we should go with the uniform setter for now until we get a better API (clutter 2.0?).
Comment 26 drago01 2012-11-19 18:33:48 UTC
Review of attachment 228402 [details] [review]:

Given that clutter does not allow us to have a better API in 1.x we should use this and probably revisit once we have 2.0
Comment 27 drago01 2012-11-19 18:39:29 UTC
Review of attachment 228404 [details] [review]:

Looks good and does indeed work.
Comment 28 Giovanni Campagna 2012-11-19 19:07:15 UTC
Created attachment 229396 [details] [review]
ScreenShield: unbreak blurring the background

It was missing a uniform, so it had no effect besides desaturation.
Comment 29 Jasper St. Pierre (not reading bugmail) 2012-11-19 19:11:52 UTC
Review of attachment 229396 [details] [review]:

Sure.
Comment 30 Giovanni Campagna 2012-11-19 19:20:03 UTC
Comment on attachment 228402 [details] [review]
MetaBackgroundActor: add a setter for GLSL uniforms

Attachment 228402 [details] pushed as e73946f - MetaBackgroundActor: add a setter for GLSL uniforms
Comment 31 Giovanni Campagna 2012-11-19 19:22:27 UTC
Attachment 229396 [details] pushed as 7bed964 - ScreenShield: unbreak blurring the background
Comment 32 Allan Day 2012-12-14 11:16:56 UTC
Testing this, I can see desaturation but no blurring. Is that to be expected?
Comment 33 Giovanni Campagna 2012-12-14 18:22:23 UTC
Not with master shell.
Minimal blurring is implemented and I'm pretty sure it works.

Note it is a very bad one pass box blur, not a two-pass Gaussian blur, but it's the best we can do without degrading performance.
Comment 34 Allan Day 2012-12-14 19:01:24 UTC
(In reply to comment #33)
> Not with master shell.
> Minimal blurring is implemented and I'm pretty sure it works.
> 
> Note it is a very bad one pass box blur, not a two-pass Gaussian blur, but it's
> the best we can do without degrading performance.

It's hard to tell here. Is there a way to increase the blur?
Comment 35 Giovanni Campagna 2012-12-14 19:04:31 UTC
No, it's fully hard coded. You need to add more samplers if you want to change that.
Comment 36 Allan Day 2012-12-14 19:06:59 UTC
(In reply to comment #35)
> No, it's fully hard coded. You need to add more samplers if you want to change
> that.

What's a sampler? :) Seriously though, I think the blur should be more distinct...
Comment 37 Giovanni Campagna 2012-12-14 19:09:12 UTC
Which means we need to reimplement this from scratch with a two-pass Gaussian blur...

I'm reopening the bug, but it's not an easy task, so don't expect a patch soon.
Comment 38 Jasper St. Pierre (not reading bugmail) 2012-12-14 19:13:03 UTC
http://git.gnome.org/browse/clutter/log/?h=wip/gaussian-blur
Comment 39 Jasper St. Pierre (not reading bugmail) 2012-12-14 19:13:33 UTC
We can also add a quick three-pass box blur, which looks a lot like a Gaussian blur.
Comment 40 drago01 2012-12-14 19:14:02 UTC
(In reply to comment #38)
> http://git.gnome.org/browse/clutter/log/?h=wip/gaussian-blur

Does not help with performance.
Comment 41 Giovanni Campagna 2012-12-14 19:46:04 UTC
(In reply to comment #40)
> (In reply to comment #38)
> > http://git.gnome.org/browse/clutter/log/?h=wip/gaussian-blur
> 
> Does not help with performance.

Maybe we can get decent performance if for the first pass we blur the background texture directly, rather than blurring the FBO-rendered background actor.
And, given that we never transform or scale the actor, we can consider rendering once the fully blurred background into a texture, and render that on screen without effects. That should help with animations, at least.
Comment 42 Owen Taylor 2012-12-14 20:43:53 UTC
(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #38)
> > > http://git.gnome.org/browse/clutter/log/?h=wip/gaussian-blur
> > 
> > Does not help with performance.
> 
> Maybe we can get decent performance if for the first pass we blur the
> background texture directly, rather than blurring the FBO-rendered background
> actor.
> And, given that we never transform or scale the actor, we can consider
> rendering once the fully blurred background into a texture, and render that on
> screen without effects. That should help with animations, at least.

The idea of working off the background image and retaining a blurred/desaturated version makes sense to me. Things you could do beyond that:

 - Maybe do the blur in software rather than in shaders - for large diameter blurs, software blurs can be quite efficient compared to shaders, since you can do FIFO box filtering. (See the code in meta-shadow-factory.c)

 - Possible approach to make things even more efficient and use less memory - scale the background down by a factor of two or so, blur on the scaled down image, scale back up when rendering the texture. I don't think it will look good to complete use bilinear filtering instead of a high-quality blur, but if there are no high-frequency components, bi-linear filtering fr a 2x scale should be fine.
Comment 43 Allan Day 2012-12-17 10:09:00 UTC
A reminder:

The original design for the lock screen suggested that a different background image be used - which could be set in system settings. This would allow someone to customise their lock screen and would ensure that the lock screen is clearly differentiated from the real session.

This bug was meant as a stop-gap until the time when you can in fact specify the lock screen image. If it is going to be a lot of work, maybe we should just concentrate on getting that separate background image working...
Comment 44 Giovanni Campagna 2012-12-17 19:58:32 UTC
Ok, let's stop pretending we will implement a nice blur and mark this fixed once and for all.
The story continues at bug 688210.