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 639851 - Magnifier: Add brightness and contrast functionality.
Magnifier: Add brightness and contrast functionality.
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: magnifier
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 658409 (view as bug list)
Depends on: 645665 656156
Blocks: 596386 641553 653170
 
 
Reported: 2011-01-18 14:55 UTC by Joseph Scheuhammer
Modified: 2013-12-04 18:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Magnifier: added brightness and contrast enhancements. (32.58 KB, patch)
2011-01-18 15:02 UTC, Joseph Scheuhammer
needs-work Details | Review
[ To squash with adding brightness and contrast enhancements ] (1.23 KB, patch)
2011-02-04 20:33 UTC, Joseph Scheuhammer
none Details | Review
Magnifier: added brightness and contrast enhancements. (32.87 KB, patch)
2011-02-10 21:54 UTC, Joseph Scheuhammer
none Details | Review
Adds inverse video, and brightness and contrast level functionality (48.49 KB, patch)
2011-03-25 20:55 UTC, Joseph Scheuhammer
none Details | Review
Adds inverse video, and brightness and contrast level functionality (14.76 KB, patch)
2011-09-22 18:36 UTC, Joseph Scheuhammer
none Details | Review
Adds brightness and contrast functionality (12.61 KB, patch)
2012-03-13 19:47 UTC, Joseph Scheuhammer
accepted-commit_now Details | Review
Adds lightness inversion functionality (15.84 KB, patch)
2012-04-04 15:54 UTC, Joseph Scheuhammer
accepted-commit_now Details | Review
Magnifier: Add brightness and contrast functionality (15.82 KB, patch)
2012-05-16 22:58 UTC, Matthias Clasen
committed Details | Review
Magnifier: Add brightness and contrast functionality (12.60 KB, patch)
2012-05-16 22:58 UTC, Matthias Clasen
committed Details | Review

Description Joseph Scheuhammer 2011-01-18 14:55:49 UTC
As per the magnifier's features-in-development list (http://live.gnome.org/GnomeShell/Magnification#Features_in_Development), other display enhancements for low vision users are inverse video, contrast, and brightness controls.

Inverse video is realized as brightness inversion.  That is, using an HSL colour model, the hue and saturation of a given colour are untouched, but its lightness is inverted.  For example, dark red becomes light red, light red, dark, while a pure red (H = 0 deg, S = 100%, L = 50%) remains the same.  Also, white becomes black, and vice versa.

Contrast and brightness are alterations to the overall contrast and/or brightness of the magnified view.

In terms of magnifier objects, these effects should be implemented as aspects of a ZoomRegion.
Comment 1 Joseph Scheuhammer 2011-01-18 14:57:13 UTC
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.
Comment 2 Joseph Scheuhammer 2011-01-18 15:02:01 UTC
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.
Comment 3 Joseph Scheuhammer 2011-02-04 20:33:34 UTC
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].
Comment 4 drago01 2011-02-04 22:10:52 UTC
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.
Comment 5 Joseph Scheuhammer 2011-02-10 21:41:07 UTC
(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.
Comment 6 Joseph Scheuhammer 2011-02-10 21:54:00 UTC
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.
Comment 7 Joseph Scheuhammer 2011-03-25 20:55:50 UTC
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.
Comment 8 Jasper St. Pierre (not reading bugmail) 2011-08-03 18:32:32 UTC
(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.
Comment 9 Joseph Scheuhammer 2011-08-05 19:38:28 UTC
(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.
Comment 10 Jasper St. Pierre (not reading bugmail) 2011-08-05 20:02:35 UTC
(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
Comment 11 Joseph Scheuhammer 2011-08-05 20:41:03 UTC
(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!
Comment 12 Jasper St. Pierre (not reading bugmail) 2011-08-05 21:11:23 UTC
"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)
Comment 13 Jasper St. Pierre (not reading bugmail) 2011-08-05 22:22:09 UTC
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.
Comment 14 Jasper St. Pierre (not reading bugmail) 2011-08-17 19:46:13 UTC
So, uh... let's push forward with this patch.
Comment 15 Joseph Scheuhammer 2011-08-30 14:24:38 UTC
(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?
Comment 16 Florian Müllner 2011-09-06 22:00:30 UTC
*** Bug 658409 has been marked as a duplicate of this bug. ***
Comment 17 Joseph Scheuhammer 2011-09-22 18:36:44 UTC
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).
Comment 18 drago01 2011-09-29 19:45:22 UTC
(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.
Comment 19 Tobias Wolf 2011-10-19 22:21:17 UTC
Straight color negative inversion makes sense too.
Comment 20 Matthias Clasen 2012-03-07 21:55:00 UTC
The clutter part has (partially) landed.
Comment 21 Joseph Scheuhammer 2012-03-13 19:47:04 UTC
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.
Comment 22 Joseph Scheuhammer 2012-03-23 14:39:36 UTC
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?
Comment 23 Florian Müllner 2012-03-23 14:45:06 UTC
(In reply to comment #22)
> Can someone review?

Are you planning on requesting a freeze break for this?
Comment 24 Joseph Scheuhammer 2012-03-23 16:31:48 UTC
(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
Comment 25 Florian Müllner 2012-03-23 17:47:48 UTC
(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)  :-)
Comment 26 Joseph Scheuhammer 2012-04-04 15:54:08 UTC
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).
Comment 27 Matthias Clasen 2012-05-02 10:40:42 UTC
Can we get this reviewed ?
Comment 28 Jasper St. Pierre (not reading bugmail) 2012-05-02 14:29:09 UTC
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?
Comment 29 drago01 2012-05-02 14:32:30 UTC
(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.
Comment 30 Jasper St. Pierre (not reading bugmail) 2012-05-02 15:10:12 UTC
Review of attachment 211309 [details] [review]:

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

Code still looks good.
Comment 31 Jasper St. Pierre (not reading bugmail) 2012-05-02 15:12:04 UTC
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.
Comment 32 Joseph Scheuhammer 2012-05-03 15:59:29 UTC
(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
Comment 33 Matthias Clasen 2012-05-16 22:58:48 UTC
The following fixes have been pushed:
61a17d7 Magnifier: Add brightness and contrast functionality
865cfa5 Magnifier: Add brightness and contrast functionality
Comment 34 Matthias Clasen 2012-05-16 22:58:56 UTC
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.
Comment 35 Matthias Clasen 2012-05-16 22:58:59 UTC
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.