Created attachment 222221 [details] [review]
patch

Adding Giovanni's patch from bug 619955.

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

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.

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.

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

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

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.

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

Review of attachment 222902 [details] [review]:

Didn't test it but code looks good to me.

I can't get it to apply, for some reason.

Attachment 222902 [details] pushed as 4fd6903 - ScreenShield: blur and desaturate the screenshield background
Allan, you can now test it in master.

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.

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

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

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.

(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 */
    }

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

I'm a moron, disregard.

Created attachment 228402 [details] [review]
MetaBackgroundActor: add a setter for GLSL uniforms

It doesn't make sense to GLSL shaders without uniforms.

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.

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?

*** Bug 687873 has been marked as a duplicate of this bug. ***

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

So what's the outcome here? Live happily with desaturation and some useless (and maybe even optimized away) sampling?

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

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

Review of attachment 228404 [details] [review]:

Looks good and does indeed work.

Created attachment 229396 [details] [review]
ScreenShield: unbreak blurring the background

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

Review of attachment 229396 [details] [review]:

Sure.

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

Attachment 229396 [details] pushed as 7bed964 - ScreenShield: unbreak blurring the background

Testing this, I can see desaturation but no blurring. Is that to be expected?

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.

(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?

No, it's fully hard coded. You need to add more samplers if you want to change that.

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

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.

http://git.gnome.org/browse/clutter/log/?h=wip/gaussian-blur

We can also add a quick three-pass box blur, which looks a lot like a Gaussian blur.

(In reply to comment #38)
> http://git.gnome.org/browse/clutter/log/?h=wip/gaussian-blur

Does not help with performance.

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

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

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

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.