The implementation uses a GLSL shader -- see the following patch.  The shader first applies any contrast change, then any brightness change, and finally, inverts brightness.  I'd prefer using three separate shaders for this, but (1) the outcome of these operations is different depending on the order in which they are applied, and (2) I don't see that any order is enforced by ClutterActor.add_effect().  If there is a way to separate these out into three different ClutterEffects that are applied in the correct order, then let me know.

Created attachment 178629 [details] [review]
Magnifier:  added brightness and contrast enhancements.

Magnifier:  added brightness and contrast enhancements.

Using a GLSL shader, a ZoomRegion can perform brightness inversion ("inverse
video"),  increases/decreases in overall brightness, and increases/decreases
in overall contrast.

Created attachment 180109 [details] [review]
[ To squash with adding brightness and contrast enhancements ]

[ To squash with adding brightness and contrast enhancements ]

Fixed calculation that mapped contrast change from [0, 255] to [-0.5, 0.5].
It was incorrectly mapping to [-1.0, 1.0].

Review of attachment 178629 [details] [review]:

Not really a full review, just some comments on the effect the issues / suggestions (about the effect / shader handling) from 639460 pretty much all apply here.
(Probably expect the "reuse shader between instances" bit, as it does not really make much sense here).

::: src/shell-mag-color-effects.c
@@ +142,3 @@
+    return FALSE;
+
+  self->actor = clutter_actor_meta_get_actor (CLUTTER_ACTOR_META (effect));

This shouldn't be needed.

@@ +153,3 @@
+      g_warning ("Unable to use the ShaderEffect: the graphics hardware "
+                 "or the current GL driver does not implement support "
+                 "for the GLSL shading language.");

Get rid of this warning, see https://bugzilla.gnome.org/show_bug.cgi?id=639460#c16 and in case where we don't have GLSL support we should quit much earlier than that.

::: src/shell-mag-color-effects.h
@@ +22,3 @@
+
+void        shell_mag_color_effects_set_invert_brightness (ShellMagColorEffects *effect,
+                                                       const gboolean       invert_brightness);

Not lined up.

(In reply to comment #4)
> Review of attachment 178629 [details] [review]:
> 
> Not really a full review, just some comments on the effect the issues /
> suggestions (about the effect / shader handling) from 639460 pretty much all
> apply here.

Thanks for the suggestions!

Looking over the way things were done for 639460, I've reworked the effect in a comparable manner.  A patch is forthcoming.

Created attachment 180618 [details] [review]
Magnifier: added brightness and contrast enhancements.

Using a GLSL shader, a ZoomRegion can perform brightness inversion ("inverse
video"),  increases/decreases in overall brightness, and increases/decreases
in overall contrast.

Created attachment 184228 [details] [review]
Adds inverse video, and brightness and contrast level functionality

The preferences schema has moved to the gsettings-desktop-schemas project, since I uploaded the last patch.  I've opened a new bugzilla on the schemas project (645665) to add the preferences there, and modified the code here to take that into account.

I've also refactored the shader into three separate clutter effects; one for inverting the brightness, one for changing brightness, and one for changing contrast.

(In reply to comment #7)
> I've also refactored the shader into three separate clutter effects; one for
> inverting the brightness, one for changing brightness, and one for changing
> contrast.

Just curious, but I think it might be nice to have these effects upstream in Clutter.

Additionally, I'm not sure your brightness code is absolutely correct, given that luminance doesn't scale linearly.

(In reply to comment #8)
>
> Just curious, but I think it might be nice to have these effects upstream in
> Clutter.
> 

If these effects were in clutter, then apps other than GNOME Shell would have access to them.  That's a good thing(tm).

> Additionally, I'm not sure your brightness code is absolutely correct, given
> that luminance doesn't scale linearly.

There are a number of ways of calculating luminance, and, yes, I used one of the simpler ones.  Are you referring the calculation:

L = 0.30R + 0.59G + 0.11B

?

I'm willing to change to that formulation.

(In reply to comment #9)
> If these effects were in clutter, then apps other than GNOME Shell would have
> access to them.  That's a good thing(tm).

Indeed, and all these effects and more can be represented by a color matrix, so it's probably best to add a generic color matrix effect with some subclasses for common effects (desaturation, reverse video, brightness, contrast)

> There are a number of ways of calculating luminance, and, yes, I used one of
> the simpler ones.  Are you referring the calculation:
> 
> L = 0.30R + 0.59G + 0.11B
> 
> ?

I can't find any color space with those primaries. For sRGB, it would be:

  Y = 0.2126R + 0.7152G + 0.0722B

See the values for the Y coordinate (luminance) at the end of http://www.w3.org/Graphics/Color/sRGB

(In reply to comment #10)
> 
> > There are a number of ways of calculating luminance, and, yes, I used one of
> > the simpler ones.  Are you referring the calculation:
> > 
> > L = 0.30R + 0.59G + 0.11B
> > 
> > ?
> 
> I can't find any color space with those primaries.

It's for HSL.  That is, given an rgb colour, the above is the formula for calculating the corresponding L in HSL space.  Wikipedia has a nice summary of these various ways of calculating "lightness" - http://en.wikipedia.org/wiki/HSL_and_HSV#Lightness

Note that the ClutterColorizeEffect uses the above in its glsl shader; quoting:

...
float gray = dot (color.rgb, vec3 (0.299, 0.587, 0.114));
...

> For sRGB, it would be:
> 
>   Y = 0.2126R + 0.7152G + 0.0722B
> 
> See the values for the Y coordinate (luminance) at the end of
> http://www.w3.org/Graphics/Color/sRGB

Okay, I'll have a look.  Thanks for the tip!

"for the Rec. 601 NTSC primaries, Y′601 = 0.30R + 0.59G + 0.11B; for other primaries different coefficients should be used"

and

"the magic gray vec3 has been taken from the NTSC conversion weights as defined by:"

I forgot to check NTSC!

Anyway, are you going to make a ClutterColorMatrixEffect, or should I start on one (I already started)

OK, before scope creep happens, I'm just going to ask that you create a bug for Clutter to get the three effects (reverse video, brightness, contrast) as-is into Clutter, and we can worry about the color matrix stuff later.

So, uh... let's push forward with this patch.

(In reply to comment #14)
> So, uh... let's push forward with this patch.

So, what's the step?  Are you going to do a full review Jasper?

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

Created attachment 197277 [details] [review]
Adds inverse video, and brightness and contrast level functionality

Since the clutter effects are likely moving to the Clutter project itself (656156), I've made this dependant on that bug number, and updated the code dependencies.  Also, modified the code in light of the latest proposed structure of the relevant gsettings-desktop-schemas (645665).

(In reply to comment #15)
> (In reply to comment #14)
> > So, uh... let's push forward with this patch.
> 
> So, what's the step?  Are you going to do a full review Jasper?

Argh ... I completely forgot about this one due to the stupid "doing a review does not add to CC list" issue.


Seems like the clutter patch got reverted again.

Straight color negative inversion makes sense too.

The clutter part has (partially) landed.

Created attachment 209639 [details] [review]
Adds brightness and contrast functionality

This integrates the ClutterBrightnessContrast effect (https://bugzilla.gnome.org/show_bug.cgi?id=656156#c45) into the magnifier, as well as the corresponding gsettings-desktop-schemas GSettings (https://bugzilla.gnome.org/show_bug.cgi?id=645665).

Unforturnately it doesn't quite work.  While it does apply the brightness and/or contrast changes to the relevant actor (a clone of the uiGroup), it also makes that actor partially transparent.  I've tried a number of things to see what might be going on and get a clue as to how to fix it, but I'm stumped.

- if the mouse hovers over the bar at the top of the screen, the effect is applied correctly, although as the mouse moves within the bar, the transparency comes and goes.

- bring up the switcher (alt-tab): the effect is properly applied to the task switcher view and its icons.

- it's not the brightness/contrast shader only:  apply the default ClutterColorizeEffect and the view becomes sepia-toned (as expected), but also transparent.

- apply the effect to the uiGroupClone's parent (a ClutterGroup), and the effect works.  But, in this scenario, it is also applied to actors within the zoomer's clutter hierarchy that it shouldn't be applied to (e.g., the crosshairs).

- the effect worked in Clutter 1.8.

Digging around, I discovered this Clutter bug "ClutterOffscreenEffect doesn't work for cloned actors" (https://bugzilla.gnome.org/show_bug.cgi?id=659523).  That might explain it, since the actor in question is a clone.

I'm putting the code here in case anyone wants to see for themselves.  I'm going to keep looking for a solution.

I wrote:
> Unforturnately it doesn't quite work.  While it does apply the brightness
> and/or contrast changes to the relevant actor (a clone of the uiGroup), it also
> makes that actor partially transparent.  I've tried a number of things to see
> what might be going on and get a clue as to how to fix it, but I'm stumped.
> 

I just rebuilt cogl and clutter, and now the effects work properly within GNOME Shell.

Can someone review?

(In reply to comment #22)
> Can someone review?

Are you planning on requesting a freeze break for this?

(In reply to comment #23)
> (In reply to comment #22)
> > Can someone review?
> 
> Are you planning on requesting a freeze break for this?

Short answer:  no.  After discussing with other a11y developers, the decision was that a freeze break is unwarranted.

<API_afk> I don't think that those changes are applicable for a code freeze break
<clown>   fair enough.
<API_afk> your change is somewhat big	
<API_afk> and usually code freeze break
<API_afk> well, in fact
<API_afk> hard code freeze break
<API_afk> are just used for crashes, problematic regressions and so on
<API_afk> in summary, severe bugs
<API_afk> not new functionality
<clown>   makes sense to me.
<API_afk> clown, https://live.gnome.org/ReleasePlanning/Freezes#Hard_Code_Freeze

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Can someone review?
> > 
> > Are you planning on requesting a freeze break for this?
> 
> Short answer:  no.

OK - so no hurry then to get this reviewed before branching (which should be shortly after.3.4.1)  :-)

Created attachment 211309 [details] [review]
Adds lightness inversion functionality

Adds a ShellInvertLightnessEffect clutter effects class to the shell itself, and modifies the magnifier to use it.  The patch is accumulative with the previous one for brightness/contrast.

Note that, as with the contrast/brightness effects, this depends on the lightness, brightness, and contrast preferences (645665#6).

Can we get this reviewed ?

I'll try to review it today. The main thing was that was there before was that I wanted some effects to go upstream in Clutter. Did that land, or get rejected?

(In reply to comment #28)
> I'll try to review it today. The main thing was that was there before was that
> I wanted some effects to go upstream in Clutter. Did that land, or get
> rejected?

See bug 656156 ... seems like brightness/contrast has been pushed while InvertLightness has been rejected/delayed hence why Joseph implemented it at the shell side.

Review of attachment 211309 [details] [review]:

Commit message seems wrong. This doesn't add brightness/contrast functionality at all!

Code still looks good.

Review of attachment 209639 [details] [review]:

Not sure I like the setting-for-each-color-thing (I'd rather a '(ddd)' setting or something), but if that's the interface we're going with, I guess that's what we're going with.

(In reply to comment #31)
> Review of attachment 209639 [details] [review]:
> 
> Not sure I like the setting-for-each-color-thing (I'd rather a '(ddd)' setting
> or something), but if that's the interface we're going with, I guess that's
> what we're going with.

Thanks for the reviews Jasper!

Note the dependency on a gsettings-desktop-schemas bug and that has to commit first.  I've asked for a final review there; hopefully this will resolve soon.

See:  https://bugzilla.gnome.org/show_bug.cgi?id=645665#c7

The following fixes have been pushed:
61a17d7 Magnifier: Add brightness and contrast functionality
865cfa5 Magnifier: Add brightness and contrast functionality

Created attachment 214219 [details] [review]
Magnifier: Add brightness and contrast functionality

Added clutter effects class that does lightness inversion.
Modified the magnifier to use this inversion effect.

Created attachment 214220 [details] [review]
Magnifier: Add brightness and contrast functionality

Modified the magnifier to use the CluttterBrightnessContrastEffect
for changing overall brightness and/or contrast of a ZoomRegion.