Created attachment 358161 [details] [review]
meta-background-actor: Style fix

Created attachment 358162 [details] [review]
meta-background-actor: Rename shader snippets

Add the word 'vignette' in order to give some context on what the
snippets do. This will be useful later when we land other snippets
for the gradient effect.

Created attachment 358163 [details] [review]
meta-background-actor: Add gradient effect

Add a gradient effect that goes from the top to the bottom. The
height and the initial dark tone of the gradient are customizable.

Created attachment 358164 [details] [review]
meta-background-actor: Add gradient properties

Make the gradient parameters as GObject properties. Doing so
we can animate the gradient from gnome-shell by using Tweener.js.

Review of attachment 358161 [details] [review]:

Sure

Review of attachment 358162 [details] [review]:

On the other hand, it might make sense to add a 'vignette-' prefix to the brightness variable?

::: src/compositor/meta-background-actor.c
@@ +247,3 @@
         {
+          static CoglSnippet *vignette_vertex_snippet;
+          static CoglSnippet *vignette_fragment_snippet;

The existing names seem fine for local variables, but OK.

Review of attachment 358163 [details] [review]:

needs-work for the unhandled height == 0 case, the rest are comments, questions and suggestions ...

::: src/compositor/meta-background-actor.c
@@ +116,3 @@
+#define GRADIENT_FRAGMENT_SHADER_CODE                                                    \
+"float min_brightness = 1.0 - gradient_tone_start;\n"                                    \
+"float gradient_y_pos = min(position.y, gradient_height_perc) / gradient_height_perc;\n" \

Possible division by zero!

To keep the shader code simple, I suggest one of the following:
 - use something like

    enabled = enabled != FALSE && height != 0

   in set_gradient() (that is, treat height == 0 the same as a disabled)

 - make sure to never set the uniform to 0 with something like

     gradient_height_perc = MAX (0.0001, priv->gradient_height / (float)priv->texture_area.height);

   in setup_pipeline()

@@ +117,3 @@
+"float min_brightness = 1.0 - gradient_tone_start;\n"                                    \
+"float gradient_y_pos = min(position.y, gradient_height_perc) / gradient_height_perc;\n" \
+"float pixel_brightness = (1.0 - min_brightness) * gradient_y_pos + min_brightness;\n"   \

Mmmh, if min_brightness is clearer than gradient_tone_start, then maybe make that the parameter? Or otherwise, why not use

  pixel_brightness = gradient_tone_start * gradient_y_pos + min_brightness;

here?

@@ +154,3 @@
 
+  gboolean gradient;
+  double gradient_tone_start;

Not entirely sold on that name, but it's hard to come up with something better (i.e. that makes it more obvious that values in the [0,1] range are expected, and what they mean). gradient_max_darkness isn't really better, but all I can come up with off-hand :-(

@@ +155,3 @@
+  gboolean gradient;
+  double gradient_tone_start;
+  int gradient_height;

Any reason for exposing a pixel height to the outside and transform that to a relative height when setting the uniform, rather than just exposing the relative height directly?

For what it's worth, the current code misses updating the gradient parameters on texture size changes, which wouldn't be an issue in the first place if the shader parameter and GObject property were both relative ...

Review of attachment 358164 [details] [review]:

I don't think this makes sense as a separate commit.

In case of the vignette, the code was first moved from a different class that didn't expose properties, then the properties were added - that's why two commits made sense, however here you're adding a second effect and expose its parameters, which already has a precedent with the vignette ...

Comment on attachment 358161 [details] [review]
meta-background-actor: Style fix

Attachment 358161 [details] pushed as 7ba44e7 - meta-background-actor: Style fix

(In reply to Florian Müllner from comment #7)
> Review of attachment 358163 [details] [review] [review]:
> ::: src/compositor/meta-background-actor.c
> To keep the shader code simple, I suggest one of the following:
>  - use something like
> 
>     enabled = enabled != FALSE && height != 0
> 
>    in set_gradient() (that is, treat height == 0 the same as a disabled)
Done.


>  - make sure to never set the uniform to 0 with something like
> 
>      gradient_height_perc = MAX (0.0001, priv->gradient_height /
> (float)priv->texture_area.height);
> 
>    in setup_pipeline()
Done.
 

> @@ +117,3 @@
> +"float min_brightness = 1.0 - gradient_tone_start;\n"                      
> \
> +"float gradient_y_pos = min(position.y, gradient_height_perc) /
> gradient_height_perc;\n" \
> +"float pixel_brightness = (1.0 - min_brightness) * gradient_y_pos +
> min_brightness;\n"   \
> 
> Mmmh, if min_brightness is clearer than gradient_tone_start, then maybe make
> that the parameter? Or otherwise, why not use
> 
>   pixel_brightness = gradient_tone_start * gradient_y_pos + min_brightness;
> 
> here?
I think it is more natural to think how much dark the gradient should be, this is why min_brightness is not the parameter. I prefer to keep the formula
(1.0 - min_brightness) * gradient_y_pos + min_brightness
as it is. Since it reflect the canonical way to map an interval from [0,1] to [x,1] where x in this case is min_brightness.


> @@ +154,3 @@
>  
> +  gboolean gradient;
> +  double gradient_tone_start;
> 
> Not entirely sold on that name, but it's hard to come up with something
> better (i.e. that makes it more obvious that values in the [0,1] range are
> expected, and what they mean). gradient_max_darkness isn't really better,
> but all I can come up with off-hand :-(
I've renamed gradient_tone_start to gradient_max_darkness, it is more consistent since in the code min_brightness is already present.


> @@ +155,3 @@
> +  gboolean gradient;
> +  double gradient_tone_start;
> +  int gradient_height;
> 
> Any reason for exposing a pixel height to the outside and transform that to
> a relative height when setting the uniform, rather than just exposing the
> relative height directly?
Because we want gnome-shell to being able to set the gradient depending on the panel height (e.g., 3*panel_height). MetaBackgroundActor already has all the informations necessary to transform it to a relative height. If we want to set directly the relative height I don't mind to change it.
 

> For what it's worth, the current code misses updating the gradient
> parameters on texture size changes, which wouldn't be an issue in the first
> place if the shader parameter and GObject property were both relative ...
I solved this by using the size of the monitor and by invalidate the pipeline when a new monitor with a different height is set.

Created attachment 359144 [details] [review]
meta-background-actor: Rename shader snippets

Add the word 'vignette' in order to give some context on what the
snippets do. This will be useful later when we land other snippets
for the gradient effect.

Created attachment 359145 [details] [review]
meta-background-actor: Rename brightness to vignette_brightness

The brightness is about the vignette. Add a 'vignette_' prefix in order
to give more context. Keep the property name as it is, doing so we don't
break any plugin (e.g., gnome-shell).

Created attachment 359146 [details] [review]
meta-background-actor: Add gradient effect

Add a gradient effect that goes from the top to the bottom. The
height and the initial dark tone of the gradient are customizable.

Review of attachment 359144 [details] [review]:

Sure

Review of attachment 359145 [details] [review]:

LGTM

Review of attachment 359146 [details] [review]:

OK with the style nit fixed

::: src/compositor/meta-background-actor.c
@@ +919,3 @@
+void
+meta_background_actor_set_monitor (MetaBackgroundActor *self,
+                                   int             monitor)

Style nit: wrong argument alignment

::: src/meta/meta-background-actor.h
@@ +74,3 @@
+
+void meta_background_actor_set_monitor  (MetaBackgroundActor *self,
+                                         int                 monitor);

Dto.

Created attachment 359358 [details] [review]
meta-background-actor: Add gradient effect

Add a gradient effect that goes from the top to the bottom. The
height and the initial dark tone of the gradient are customizable.

Attachment 359144 [details] pushed as f381d7c - meta-background-actor: Rename shader snippets
Attachment 359145 [details] pushed as cbc4563 - meta-background-actor: Rename brightness to vignette_brightness
Attachment 359358 [details] pushed as 068791f - meta-background-actor: Add gradient effect