I'm not generally against adding post-processing image effects into Clutter, though unlike Jasper I don't really see the need to have a ColorMatrix effect in the API: even if it's "more correct" from the OOP perspective, the handling is pretty different, and the shader used is also pretty much different.

(In reply to comment #1)
> I'm not generally against adding post-processing image effects into Clutter,
> though unlike Jasper I don't really see the need to have a ColorMatrix effect
> in the API: even if it's "more correct" from the OOP perspective, the handling
> is pretty different, and the shader used is also pretty much different.

Yeah, for some reason I thought that Clutter already had a generic ColorMatrix that you used for calculations but not an effect for it, and it would be quite simple to make into an effect.

I'd like to point out that, by the official GNOME 3.2 schedule, we ought to be API frozen by Wednesday — which means the window for merging new API should now be considered frozen for everything that is not a last minute fix. like gtk+, we're usually less strict about API freeze dates — I use the term "slushy" — but adding three new classes at the last minute is not really feasible at this point.

as I said in comment 1, I'm not against including these effects in Clutter, given that we already have Colorize and Desaturate, but they'll have to wait for the next merge window, which will be in October, when we branch off from master for the 1.9 cycle.

Created attachment 196143 [details] [review]
Three clutter effects classes - 1. Invert lightness, 2. Modify brightness, 3. Modify contrast

Made by carving out the relevant code from the patch to gnome-shell, and clutter-izing it.

Review of attachment 196143 [details] [review]:

overall, it looks good apart from minor issues.

you should also:

  • add the three new get_type() functions to docs/reference/clutter/clutter.types
  • add the newly added functions and types to docs/reference/clutter/clutter-sections.txt
  • add the three newly added classes to docs/reference/clutter/clutter-docs.xml.in

::: clutter/clutter-brightness-effect.c
@@ +242,3 @@
+{
+  ClutterBrightnessEffect *effect = CLUTTER_BRIGHTNESS_EFFECT (gobject);
+  ClutterColor *aColor = clutter_color_new (127, 127, 127, 255);

don't allocate the Color; just use:

  static const ClutterColor default_color = { 127, 127, 127, 255 };

clutter_value_set_color() will copy the color passed to it.

@@ +281,3 @@
+   * The brightness change to apply to the actor
+   *
+   */

missing: Since: 1.10

@@ +334,3 @@
+ * clutter_actor_add_effect()
+ *
+ * Return value: the newly created #ClutterBrightnessEffect or %NULL

missing:

  the (transfer full) annotation
  Since: 1.10

@@ +350,3 @@
+ * Add each of the red, green, blue components of the @brightness to
+ * the red, greeen, or blue component of the actor's colors.
+ */

missing:

  Since: 1.10

@@ +356,3 @@
+{
+  g_return_if_fail (CLUTTER_IS_BRIGHTNESS_EFFECT (effect));
+  if (clutter_color_equal (&effect->brightness, &brightness))

this is wrong: you're passing the address of a pointer. it should be:

  if (clutter_color_equal (&effect->brightness, brightness))
    return;

@@ +367,3 @@
+/**
+ * clutter_brightness_effect_get_brightness:
+ * @effect: a #ClutterBrightnessEffect

missing the @brightness argument

@@ +370,3 @@
+ *
+ * Retrieves the brightness value used by @effect
+ */

missing:

  Since: 1.10

::: clutter/clutter-contrast-effect.c
@@ +29,3 @@
+ *
+ * #ClutterContrastEffect is a sub-class of #ClutterEffect that changes the
+ * overall contrast of a clutter actor.

use #ClutterActor to let gtk-doc link the page.

@@ +254,3 @@
+{
+  ClutterContrastEffect *effect = CLUTTER_CONTRAST_EFFECT (gobject);
+  ClutterColor *aColor = clutter_color_new (127, 127, 127, 255);

no need to allocate the Color

@@ +255,3 @@
+  ClutterContrastEffect *effect = CLUTTER_CONTRAST_EFFECT (gobject);
+  ClutterColor *aColor = clutter_color_new (127, 127, 127, 255);
+  switch (prop_id)

please, keep a new line between declarations and statements.

@@ +293,3 @@
+   * The contrast change to apply to the actor
+   *
+   */

missing: Since: 1.10

@@ +346,3 @@
+ *
+ * Return value: the newly created #ClutterContrastEffect or %NULL
+ */

missing the (transfer full) annotation and Since: 1.10

@@ +363,3 @@
+ * color is less than the midpoint, subtract the contrast; otherwise, add the
+ * contrast.
+ */

missing: Since: 1.10

@@ +370,3 @@
+  g_return_if_fail (CLUTTER_IS_CONTRAST_EFFECT (effect));
+
+  if (clutter_color_equal (&effect->contrast, &contrast))

you're taking the address of the pointer here as well.

@@ +381,3 @@
+/**
+ * clutter_contrast_effect_get_contrast:
+ * @effect: a #ClutterContrastEffect

missing the @constrast argument; remember the (out caller-allocates) annotation.

@@ +387,3 @@
+void
+clutter_contrast_effect_get_contrast (ClutterContrastEffect *effect,
+                                        ClutterColor           *contrast)

please, line up the arguments.

::: clutter/clutter-contrast-effect.h
@@ +53,3 @@
+ClutterEffect *clutter_contrast_effect_new (void);
+
+void        clutter_contrast_effect_set_contrast (ClutterContrastEffect *effect,

please, line up the functions and the arguments

::: clutter/clutter-invert-lightness-effect.c
@@ +189,3 @@
+
+static void
+clutter_invert_lightness_effect_set_property (GObject      *gobject,

don't add set_property() if there are no writable properties.

@@ +203,3 @@
+
+static void
+clutter_invert_lightness_effect_get_property (GObject    *gobject,

don't add get_property() if there are no readable properties.

@@ +275,3 @@
+ * clutter_actor_add_effect()
+ *
+ * Return value: the newly created #ClutterInvertLightnessEffect or %NULL

missing the (transfer full) annotation and Since: 1.10

Any reason not to use the ClutterShaderEffect base class ?

(In reply to comment #6)
> Any reason not to use the ClutterShaderEffect base class ?

effects inside Clutter should try to avoid using the ShaderEffect class directly, in case we want to change the internal implementation.

Created attachment 196497 [details] [review]
New patch that incorporates E. Bassi's suggestions

Thanks for the review, Emmanuele.  I've modified the files as you suggested.  Let me know if I missed anything.

Review of attachment 196497 [details] [review]:

there are still a couple of missing Since tags here and there, and I'm don't liket the instantiating a shader inside the instance init() function. other shader-based effects in Clutter instantiate the Cogl shader on demand inside the first pre_paint(), when we know we have a GL context associated with a drawable surface.

Created attachment 197596 [details] [review]
Patch in response to latest review (comment 9).

Added a couple more Since tags -- hopefully, I found them all.  Also, modified the three classes to instantiate the glsl shader in their pre_paint() methods.

Review of attachment 197596 [details] [review]:

looks good, thanks for working on this. I think we can land it on master, and if we find other issues we can open new bugs.

I'm by no means an image processing expert, but these shaders don't look correct to me. Aren't the brightness and contrast adjustment values meant to be a single variable and not a 3-component vector? The formulas given on Wikipedia look like this:

if (brightness < 0.0)  value = value * ( 1.0 + brightness);
                  else value = value + ((1 - value) * brightness);
value = (value - 0.5) * (tan ((contrast + 1) * PI/4) ) + 0.5;

http://en.wikipedia.org/wiki/Image_editing#Contrast_change_and_brightening

It doesn't look like there should be any need for the three conditionals in the contrast shader (it's effectively just adding a constant) and the brightness shader looks like it should multiply by a value rather than add to it.

The conditional in the contrast is checking the incoming color value is comparing the incoming color value and not the contrast adjustment, that can't be right, can it?

The brightness shader as it stands is utterly trivial so I think it would be worth implementing with cogl_pipeline_set_combine_function rather than explicitly using a shader so that we can get portability to fixed-function hardware.

Using three separate shader effects is much less efficient than combining all three shaders into one. Each separate effect implies a separate rendering pass to a new offscreen buffer. In Gnome Shell I would highly recommend combining these effects into one seeing as they are always used together. For Clutter I think it would make sense to at least combine the brightness and contrast effects because you are likely to want to use both of them if you are using one.

(In reply to comment #11)
> Review of attachment 197596 [details] [review]:
> 
> looks good, thanks for working on this.

You're welcome.  Thanks for the reviews.

> I think we can land it on master, and
> if we find other issues we can open new bugs.

Done.

reverted after talking with Neil. let's figure out the kinks - we're still in the early 1.9 days.

(In reply to comment #12)
> I'm by no means an image processing expert,

Neither am I :-)

> but these shaders don't look
> correct to me. Aren't the brightness and contrast adjustment values meant to be
> a single variable and not a 3-component vector? The formulas given on Wikipedia
> look like this:
> 
> if (brightness < 0.0)  value = value * ( 1.0 + brightness);
>                   else value = value + ((1 - value) * brightness);
> value = (value - 0.5) * (tan ((contrast + 1) * PI/4) ) + 0.5;
> 
> http://en.wikipedia.org/wiki/Image_editing#Contrast_change_and_brightening
> 

The impetus for these effects is users who have different needs in terms of lightness-inversion, brightness, and contrast modifications.  For example, with respect to brightness, some prefer to change just the red channel while leaving green and blue alone, while others change the blue channel only, while still others simply want to change overall brightness (all three channels). Similarly for contrast.  That's why it uses a 3-component vector.

That being said, the equations I used for brightness and contrast are based on the now defunct gnome-mag CORBA service.  It's what gnome magnifier users have been using for some time.  I've yet to take a close look at the equation you cite above -- for all I know, the two equations give the same result.  I'll investigate.


> It doesn't look like there should be any need for the three conditionals in the
> contrast shader (it's effectively just adding a constant)

I don't follow.  Sometimes it's subtracting, and that depends on whether the current value is above or below the midpoint.  Using red as an example, if the current red is less than the midpoint, then subtract the (red) contrast value; otherwise, add the contrast value.  Darker colours become darker-er while brighter ones become brighter-er, thereby increasing the contrast between the two.

> and the brightness
> shader looks like it should multiply by a value rather than add to it.

Why?  One advantage of adding is that it allows for a larger range and finer control, although I'm not sure if this addresses your point.

> 
> The conditional in the contrast is checking the incoming color value is
> comparing the incoming color value and not the contrast adjustment, that can't
> be right, can it?

I assumed that the incoming color was accurate in the sense that it was the value unaffected by any other shader or manipulation.  Perhaps a bad assumption.  But if so, how does one get the "actual" value?

> 
> The brightness shader as it stands is utterly trivial so I think it would be
> worth implementing with cogl_pipeline_set_combine_function rather than
> explicitly using a shader so that we can get portability to fixed-function
> hardware.

I'm by no means a cogl expert, and if that is more efficient with the same result, then I have no objection.

> 
> Using three separate shader effects is much less efficient than combining all
> three shaders into one. Each separate effect implies a separate rendering pass
> to a new offscreen buffer. In Gnome Shell I would highly recommend combining
> these effects into one seeing as they are always used together.

The first few implementations did use a single shader -- see, for example, https://bugzilla.gnome.org/show_bug.cgi?id=639851#c2.  But, since some users simply want to invert, otherwise leaving the brightness and contrast alone, it seemed to me that having to also apply brighness and contrast changes in the same glsl code (where they would be no-ops) was inefficient.  You have to apply the effect class even when only one of the (inner) effects is used.  The inefficiency you describe only occurs if the effects are combined by the same user.

> For Clutter I
> think it would make sense to at least combine the brightness and contrast
> effects because you are likely to want to use both of them if you are using
> one.

I don't share your intuition.  Some users prefer to modify brightness only.  Some prefer changes only to contrast.  Still others want both.

But that's beside the point.  What matters is where the greater inefficiency lies.  Again, I'm not a clutter/cogl expert, and if it's the case that it's more efficient to combine them into one "multiplexor" effects class, then I'm not against that.  I can haul out the older code and re-jig the implementation.

Then again, maintainability is a consideration.  If at some point in the future one wants to modify the contrast effect implementation, then it's easier to change a single contrast-effect class.  Compare that with teasing out the contrast portion of a brightness-contrast-effect class, making sure that only the contrast part was changed, and leaving the brightness calculation unaffected.  That's relatively more complicated.

It's a balance between efficiency, maintainability, usability, and so on.

I've been reading up image processing, and although I'm sill not an expert, I know a bit more than before.

With respect to contrast:
A function that changes contrast either increases or decreases the difference between two values, centred on the range of values.  For example, if the value range is [0, 1.0] centred on 0.5, than an increase in contrast is accomplished by decreasing values less than 0.5, and increasing the values above 0.5, clamping at 0 and 1 respectively. In the limit, the maximum change in contrast reduces all the values below 0.5 to 0, and all the values above to 1.0.  Decreases in contrast are done in the opposite maner, where values below the midpoint are increased and values above are decreased.  In the limit here (no contrast), all the values are equal to 0.5.

A graph of the situation is available in Poynton's "Digital Video and HDTV Algorithms and Interfaces" -- see http://books.google.com/books?id=ra1lcAwgvq4C&pg=PA130&lpg=PA130&dq=poynton+Digital+Video+and+HD&source=bl&ots=bPfaGFXY5a&sig=MGa0tJo4nXDuwgkS0sGRynvDRfo&hl=en&ei=pU-cTtHXBo670QHBuMGhBA&sa=X&oi=book_result&ct=result&resnum=1&ved=0CD0Q6AEwADgK#v=onepage&q&f=false, pg 29.

This is what the GLSL shader does in the ContrastEffect class.  I've attached screen shots of it in action showing a decrease in contrast and an increase.  The top half of each image shows the desktop where no contrast change effect is applied.

The additional wrinkle is that the shader allows for a contrast change on a per-channel basis.  If one wants to change the contrast only in the red channel, then set the effect's contrast value such that only the red values are affected, and so on.

As for brightness:
There are numerous ways of manipulating RGB values to change perceived brightness.  Simply adding or subtracting from the RGB is one way to do so.  See the graph in Poynton's book, just above the figure for contrast mapping.  Perhaps the name is problematic since there are other ways to manipulate brightness (e.g., manipulating the "L" in an HSL colour representation).  The class could be named IntensityBrightnessEffect since it modifies the intensity of the red, green, and blue channels.  If someone wants to write another based on the definition of brightness as, say, the mean of the RGB values, then they could call it MeanRGBBrightnessEffect, or something similar.

So:  I don't think these algorithms are "wrong" with a capital W, and do count as brightness and contrast changes.  There may be better algorithms -- if so, what are they?

Created attachment 199267 [details]
Screen shot showing decrease in contrast using ClutterConrastEffect

Created attachment 199268 [details]
Screen shot showing increase in contrast using ClutterContrastEffect

(In reply to comment #12)
> 
> The brightness shader as it stands is utterly trivial so I think it would be
> worth implementing with cogl_pipeline_set_combine_function rather than
> explicitly using a shader so that we can get portability to fixed-function
> hardware.

I was going to try to re-implement the brightness change effect using your suggestion, but I can't find this function, neither in the documentation nor the code base.

Where is it?

Something which properly manipulates the brightness would have to take into account the gamma of the output display device. I don't think we want to make clutter depend on colord or anything, so the naïve algorithm is fine.

The problem is that we can only represent 255 values per channel, so you get "clipping" where whites can't get any whiter or blacks can't get any blacker. A way to solve this would be to convert to HSV and adjust V, but I'm not sure we want to do this for every pixel.

(In reply to comment #19)
> (In reply to comment #12)
> > 
> > The brightness shader as it stands is utterly trivial so I think it would be
> > worth implementing with cogl_pipeline_set_combine_function rather than
> > explicitly using a shader so that we can get portability to fixed-function
> > hardware.
> 
> I was going to try to re-implement the brightness change effect using your
> suggestion, but I can't find this function, neither in the documentation nor
> the code base.
> 
> Where is it?

I think he means cogl_pipeline_set_layer_combine.

(In reply to comment #20)

> The problem is that we can only represent 255 values per channel, so you get
> "clipping" where whites can't get any whiter or blacks can't get any blacker. A
> way to solve this would be to convert to HSV and adjust V, but I'm not sure we
> want to do this for every pixel.

I'm not sure that would solve it.  For example, the rgb triple (33, 128, 255)
has a V of 255.  Adding a constant 25 to all channels, and clipping results in
(58, 233, 255), which also has a V of 255.  How would you adjust V to
compensate?

Is this stuck, or is there any progress towards resolving this ?

Cogl now has the CoglSnippets API, which allows "merging" shaders together inside the same pipeline:

blog post outlining the API:
http://www.busydoingnothing.co.uk/blog/2011/12/07#cogl-shader-snippets

API reference:
http://docs.clutter-project.org/docs/cogl-2.0-experimental/unstable/cogl-2.0-experimental-Shader-snippets.html

it should be possible to create a single shader covering all three operations, and control each one using a different uniform, to avoid creating three different framebuffer objects.

(In reply to comment #24)
> Cogl now has the CoglSnippets API, which allows "merging" shaders together
> inside the same pipeline:
> 
> blog post outlining the API:
> http://www.busydoingnothing.co.uk/blog/2011/12/07#cogl-shader-snippets
> 
> API reference:
> http://docs.clutter-project.org/docs/cogl-2.0-experimental/unstable/cogl-2.0-experimental-Shader-snippets.html
> 
> it should be possible to create a single shader covering all three operations,
> and control each one using a different uniform, to avoid creating three
> different framebuffer objects.

Thanks Emmanuele (and Neil).  I'm close to uploading a patch using two shaders: one for brightness-contrast, and another for the inversion.  I'll complete that anyway, and then take a look at the blog and API.

Created attachment 206136 [details] [review]
Two effects classes using shaders to (1) invert lightness and (2) modify brightness and contrast

Uploading this patch mostly to show the logic.

After researching articles, books, and the GIMP source code brightness/contrast effects, I ended up using Neil's suggestion above (comment 12).  The inversion shader is the same as before.

The patch is against the clutter-1.8 branch since that's what GNOME Shell is using, and that's where I tested, as part of the magnifier.

Next steps are to see what's different between 1.8 and master, and to look into Coglsnippets.

first of all, thanks again for looking into this.

I'll give a more detailed review ASAP.

(In reply to comment #26)

> The patch is against the clutter-1.8 branch since that's what GNOME Shell is
> using, and that's where I tested, as part of the magnifier.

actually, the Shell has long since switched to Clutter 1.9, which is what 3.4 will be using.

> Next steps are to see what's different between 1.8 and master, and to look into
> Coglsnippets.

that would be much appreciated.

thanks again!

Created attachment 206895 [details] [review]
Patch for master:  two effects using shaders to (1) invert lightness and (2) modify brightness and contrast

These are the same effects as 206136, but are patches for the master branch.

There wasn't a lot of difference between clutter-1.8 and master, but I may have missed something.  Let me know if you find anything.

Created attachment 207572 [details] [review]
Add lightness, brightness, and contrast effects/v2.0

Exactly like attachment 206895 [details] [review], but with the missing clutter.h change.

Created attachment 207573 [details] [review]
Port new effects to the CoglSnippet API/v1.0

Initial dumb port to the CoglSnippet API.

This will require a sanity check/review from Neil.

Unintended side-effect that needs to be fixed: if I apply either effect on an actor that already has an offscreen effect, the effects will operate on the transparent pixels as well; simple way to check: just apply either effect on the rotating rectangle inside the test-shader-effects interactive test. We probably don't want that to happen: the alpha channel should be left alone if it's fully transparent.

Review of attachment 207573 [details] [review]:

::: clutter/clutter-brightness-contrast-effect.c
@@ +79,3 @@
  */
+static const gchar *brightness_contrast_decls =
+  "uniform sampler2D tex;\n"

The snippet doesn't need to define its own sampler uniform any more because Cogl will provide one.

@@ +84,3 @@
+
+static const gchar *brightness_contrast_source =
+  "cogl_texel = texture2D (cogl_sampler, cogl_tex_coord.st);\n"

I think it would be neater to do this as a 'post' string on the COGL_SNIPPET_HOOK_FRAGMENT hook instead. That way the snippet won't need to do its own texture lookup. Take a look at how ClutterColorizeEffect works for an example. The blur effect only does it this way because it needs to sample the texture multiple times so it needs to take over the sampling part.

@@ +85,3 @@
+static const gchar *brightness_contrast_source =
+  "cogl_texel = texture2D (cogl_sampler, cogl_tex_coord.st);\n"
+  "vec4 color = cogl_color_in * cogl_texel;\n"

The shader shouldn't multiply the pipeline's color in here because this is only the texture lookup hook so it should only do texture lookups. The generated fragment hook will multiply in the color again.

(In reply to comment #30)

> Unintended side-effect that needs to be fixed: if I apply either effect on an
> actor that already has an offscreen effect, the effects will operate on the
> transparent pixels as well

Do you mean this is a side-effect of the port to the snippets API? If not then it could be because the shader is operating on the colors as if they are not premultiplied. For example, any fully transparent color after being premultiplied becomes (0,0,0,0). If the shader is used to apply a brightness of 0.5 then the resulting color will be (0.5,0.5,0.5,0). This doesn't make any sense as a premultiplied color so it's going to do the wrong thing. It might make sense to unpremutiply the alpha, do the calculation to apply the effect and then repremultiply the alpha. I'm not sure why that problem wouldn't also apply to the shader before it was converted to the snippet API though.

I think it would be good to do some more optimising on the brightness/contrast shader. Conditionals in a shader tend to cause big performance problems so it would be good if we could avoid them. Both calculations for the brightness can be expressed in the form ax + c where a and c are constants we can put in a uniform and x is the color value. We can precalculate the values for a and c outside of the shader to avoid the conditional. When b is negative, a will be (1+b) and c will be 0. When b is positive a will be (1-b) and c will equal b. GLSL can do operations on all of the components of a vector simultaneously so we should try to combine the three statements of each effect into one. Then the brightness/contrast shader would look like this:

uniform vec3 brightness_a, brightness_c;
uniform vec3 contrast;

cogl_color_out.rgb = cogl_color_out.rgb * brightness_a + brightness_c;
cogl_color_out.rgb = (cogl_color_out.rgb - 0.5) * contrast + 0.5;

I'm not sure about the lightness-inversion actor. Joseph, could you explain the algorithm for that a little or point to where it came from?

(In reply to comment #32)
> 
> I'm not sure about the lightness-inversion actor. Joseph, could you explain the
> algorithm for that a little or point to where it came from?

Hi Neil,

The prerequisites for the inversion algorithm are:
- as much as possible, the hue should not change.  Reds should remain red, greens remain green, yellow remain yellow, and so on.
- pure colours should not change.  For example, pure red (1,0,0) should not change.
- putting it colloquially, the grey levels should invert.  White becomes black, and vice versa, darker colours become lighter, and lighter colours darker.

I used the third method defined here for calculating lightness: http://en.wikipedia.org/wiki/HSL_and_HSV#Lightness

L = 1/2 (M + m) where M is the maximum of (r,g,b) and m is the minimum.

Here is a screen capture of a colour wheel showing the effect:  http://aegis.idrc.ocad.ca/uploads/images/ColourWheel.png.  Note that the pure colours (the colours around the wheel) don't change, and, generally, the hues remain the same.  But, the grey levels are inverted, as seen in the (red) colour triangle.  In particular, black becomes white, and vice versa.

On another note:
> I think it would be good to do some more optimising on the
> brightness/contrast shader.
> ...

I agree completely.  I was playing with the math back in January trying to get rid of the conditionals, but couldn't see how to do it.  I did manage to move the tan(x) part outside of the glsl, but I'm not sure if more could be done there.

Created attachment 207617 [details] [review]
Optimise the brightness/contrast shader

Here are the changes I was suggesting to the brightness/contrast
shader in patch form. I think the trick about unpremultiplying the
color before applying the effect would be a bit risky because then
when the alpha is zero you end up with a divide-by-zero and the GLSL
spec says the results are undefined if you do this. However we can
avoid this by writing the formula as if we were going to do this
unpremult/premult at each step but then rearranging it so that it
keeps the color as premultiplied and only needs to multiply the
constants by the alpha instead.

playing with the API, I think using a ClutterColor is not a great idea:

- it requires normalizing the value, and yet it doesn't allow expressing proper granularity;
- the alpha channel is discarded anyway.

plus, if you're using it in conjunction with GTK API, you'll have to move from a GdkRGBA, which uses a normalized floating point value between 0 and 1, and a ClutterColor, which uses an unsigned 8 bit integer, and then back to a floating point value when passing it to Cogl for the shader.

the best approach is to make set_brightness() and set_contrast() take a triplet of normalized floating point values between 0 and 1, e.g.:

  set_brightness (effect, float r, float g, float b)
  set_contrast (effect, float r, float g, float b)

we could even have a set_brightness()/set_contrast() taking a single value for all three channels, and a set_brightness_full()/set_contrast_full() taking three different values.

(In reply to comment #35)
Replacing the ClutterColor with floating point values is a good idea, I think.

One thing, though; in relation to their range:
> 
> the best approach is to make set_brightness() and set_contrast() take a triplet
> of normalized floating point values between 0 and 1, e.g.:

The current preferences scheme proposal (bugzilla 645665) has them ranging between -1.0 and 1.0, where negative values represent a decrease in brightness/contrast, positive values are for increases, and zero means "no change".

Of course, it's arbitrary what you call the zero point -- 0x7f in the case of uint8, 0.5 if the range is [0.0, 1.0], or 0.0 if it's [-1.0, 1.0].  It makes sense to follow the preferences schema, though.

Created attachment 207913 [details] [review]
Use floats instead of ClutterColor

Following up on Emmanuele's suggestion in comment 35.

Created attachment 208264 [details] [review]
Add brightness/contrast effect/v2.0

I squashed the attachements into a single patch that adds the BrightnessContrast effect and cleans up the coding style. I left the authorship credit to Joseph, and added a Modified-by line for me and Neil.

I'll do a second patch that adds the InvertLightness effect separately; I have not yet decided whether the invert-lightness effect is too specialized for Clutter or not; nevertheless, even if it went to the Shell, the code base would be identical, so it it could be easily merged into Clutter at a later date.

(In reply to comment #38)
> I'll do a second patch that adds the InvertLightness effect separately; I have
> not yet decided whether the invert-lightness effect is too specialized for
> Clutter or not; nevertheless, even if it went to the Shell, the code base would
> be identical, so it it could be easily merged into Clutter at a later date.

Take note that one goal is also to reuse those effects in clutter-gst to implement the color balance interface.

(In reply to comment #39)
> (In reply to comment #38)
> > I'll do a second patch that adds the InvertLightness effect separately; I have
> > not yet decided whether the invert-lightness effect is too specialized for
> > Clutter or not; nevertheless, even if it went to the Shell, the code base would
> > be identical, so it it could be easily merged into Clutter at a later date.
> 
> Take note that one goal is also to reuse those effects in clutter-gst to
> implement the color balance interface.

okay, this I didn't know. :-)

does the invert-lightness effect apply to this as well?

Isn't that another way to do Hue?

the invert-lightness effect is much more specific than just hue: it flips the lightness as defined in comment 33. if we want a hue manipulator effect then we'll need a different effect, I guess.

so, at this point, I think we should skip the InvertLightness and have a proper Hue effect.

Review of attachment 208264 [details] [review]:

Found a couple of typos.

::: clutter/clutter-brightness-contrast-effect.c
@@ +538,3 @@
+ *
+ * The range of @brightness is [-1.0, 1.0], where 0.0 designates no change;
+ * a values less that 0.0 indicates a decrease in brightness; and a value

Beginning of line should be "a value", not "a values".

@@ +628,3 @@
+ *
+ * The range for @contrast is [-1.0, 1.0], where 0.0 designates no change;
+ * avalue less than 0.0 indicates a decrease in contrast; and a value above

Beginning of line should read "a value", not "avalue"

(In reply to comment #39)

> Take note that one goal is also to reuse those effects in clutter-gst to
> implement the color balance interface.

I think it would be a shame to use these effects in something like ClutterGst where it is only rendering a single primitive anyway and the performance is critical. The trouble with using these effects is that each one redirects the rendering offscreen so the fill rate is effectively doubled. I wonder if for ClutterGst we can do something better by hooking the snippet directly into the actor's pipeline. If this is a really common use case it might be better to just make this a property of ClutterGstVideoTexture and let it handle it itself.

Created attachment 209161 [details] [review]
Add brightness/contrast effect / v3.0

Fixed the typos, and renamed the per-channel variants to _full(), and removed the _values() from the single value variants, to better fit in into the Clutter API conventions.

I'm going to push this to master.

Will keep open for the InvertLightness effect.

given that the invert lightness effect went in gnome-shell, we can probably close this one.