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 759155 - Review the ranges of the Colors and Enhance sliders
Review the ranges of the Colors and Enhance sliders
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
3.19.x
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-12-08 07:30 UTC by Debarshi Ray
Modified: 2016-02-15 14:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gnome-photos: restrict the range of the brightness slider (1.29 KB, patch)
2016-01-15 14:08 UTC, Neha Yadav
needs-work Details | Review
tool-colors: Restrict the range of the brightness slider (1.03 KB, patch)
2016-01-15 17:47 UTC, Neha Yadav
none Details | Review
tool-colors: Restrict the range of the brightness slider (1.08 KB, patch)
2016-01-15 18:15 UTC, Neha Yadav
none Details | Review
tool-colors: Restrict the range of the brightness slider (10.15 KB, patch)
2016-01-15 19:00 UTC, Neha Yadav
none Details | Review
tool-colors: Restrict the range of the brightness slider (1.08 KB, patch)
2016-01-15 19:30 UTC, Neha Yadav
none Details | Review
tool-colors: Restrict the range of the brightness slider (1.08 KB, patch)
2016-01-16 06:10 UTC, Neha Yadav
none Details | Review
tool-colors: Restrict the range of the brightness slider (1.08 KB, patch)
2016-01-16 06:21 UTC, Neha Yadav
none Details | Review
tool-colors: Restrict the range of the brightness slider (1.08 KB, patch)
2016-01-16 09:11 UTC, Neha Yadav
committed Details | Review
Adjust Contrast Slider (1.86 KB, patch)
2016-01-25 07:52 UTC, Umang Jain
none Details | Review
Adjust Contrast Slider (3.18 KB, patch)
2016-01-26 13:02 UTC, Umang Jain
committed Details | Review
tool-colors: Restrict the lower bound of the contrast slider (3.29 KB, patch)
2016-01-26 17:37 UTC, Debarshi Ray
committed Details | Review
tool-enhance, denoise: Increase the upper bound of denoise slider (1.29 KB, patch)
2016-02-12 22:36 UTC, Umang Jain
none Details | Review
tool-enhance, denoise: Increase the gegl:noise-reduction step from 0.5 to 1.0 (1.28 KB, patch)
2016-02-13 12:09 UTC, Umang Jain
committed Details | Review
tool-enhance, denoise: Increase the upper bound of denoise slider (1.14 KB, patch)
2016-02-13 12:13 UTC, Umang Jain
committed Details | Review

Description Debarshi Ray 2015-12-08 07:30:35 UTC
Some of the sliders in the Colors and Enhance tools have extremely large ranges, while others are too limited. We should sanitize the ranges and fix the recommendations in GEGL wherever applicable.
Comment 1 Debarshi Ray 2016-01-14 09:04:21 UTC
Let's start with brightness.

After some discussion with pippin in #gegl on GIMPNet we decided to restrict the range of the brightness slider to [-0.5, 0.5] instead of [-1.0, 1.0]. The step should also be adjusted to ensure that we continue of have 10 levels.
Comment 2 Neha Yadav 2016-01-15 14:08:59 UTC
Created attachment 319106 [details] [review]
gnome-photos: restrict the range of the brightness slider

restrict the range of brightness slider

The problem is that slider in the colour have extremely large range that is [-1.0,1.0]

To fix this issue we restrict the range of the brightness slider to [-0.5, 0.5] instead of [-1.0, 1.0]. The step should also be adjusted to ensure that we continue of have 10 levels.
Comment 3 Alessandro Bono 2016-01-15 15:16:05 UTC
Review of attachment 319106 [details] [review]:

Hey, thanks for the patch.
At this time we have 20 levels (from -1.0 to 1.0 with a 0.1 step), since we want to continue to have the same number of levels as before you should also adjust the BRIGHTNESS_STEP.
Comment 4 Neha Yadav 2016-01-15 17:47:58 UTC
Created attachment 319133 [details] [review]
tool-colors: Restrict the range of the brightness slider

tool-colors: Restrict the range of the brightness slider
Comment 5 Debarshi Ray 2016-01-15 18:08:34 UTC
Review of attachment 319133 [details] [review]:

This looks OK, but can you please squash the two patches into one? Also, the subject of the commit message seems to have gotten duplicated in the body.
Comment 6 Neha Yadav 2016-01-15 18:15:27 UTC
Created attachment 319140 [details] [review]
tool-colors: Restrict the range of the brightness slider
Comment 7 Debarshi Ray 2016-01-15 18:34:09 UTC
Review of attachment 319140 [details] [review]:

Thanks. This looks much better.

One small thing, though. Did you set your name correctly in git as shown in https://wiki.gnome.org/Newcomers/CodeContributionWorkflow ? We usually prefer to have a proper full name in the commit message.

Thanks for your work so far.
Comment 8 Neha Yadav 2016-01-15 19:00:21 UTC
Created attachment 319142 [details] [review]
tool-colors: Restrict the range of the brightness slider
Comment 9 Neha Yadav 2016-01-15 19:30:01 UTC
Created attachment 319145 [details] [review]
tool-colors: Restrict the range of the brightness slider
Comment 10 Debarshi Ray 2016-01-15 20:44:39 UTC
Review of attachment 319145 [details] [review]:

Your name isn't properly capitalized. I am insisting on this because your name shows up in the UI, specifically the about dialog.
Comment 11 Neha Yadav 2016-01-16 06:10:06 UTC
Created attachment 319167 [details] [review]
tool-colors: Restrict the range of the brightness slider
Comment 12 Neha Yadav 2016-01-16 06:21:17 UTC
Created attachment 319168 [details] [review]
tool-colors: Restrict the range of the brightness slider
Comment 13 Neha Yadav 2016-01-16 09:11:20 UTC
Created attachment 319170 [details] [review]
tool-colors: Restrict the range of the brightness slider
Comment 14 Debarshi Ray 2016-01-19 08:20:52 UTC
I would say that the saturation slider is quite OK at this moment. It increases the saturation until a certain point and at its lowest extreme it turns the image into a greyscale monochrome, which is what I expect when there is zero saturation.

The contrast slider, on the other hand, needs some adjustment. I don't like that it turns the image into a flat mid-tone grey when the slider is at a minimum. The higher end, however, looks OK.

For reference, this is how the gegl:brightness-contrast operation works:
    out_pixel[i] = (in_pixel[i] - 0.5f) * contrast + brightness + 0.5;
where, i is a particular colour component of a RGB pixel, and
each colour component is a single precision floating point value in [0.0, 1.0].

The problem with restricting the constrast slider to something like [0.5, 2.0] is that the default value of 1.0, which is the no-op, is no longer at the centre of the scale. That is a nice visual aid and it would be awkward to lose it. ie., if the scale is linear, as it is now.

So, I think, that we should come up with a non-linear scale, or split it into two linear ranges. eg., one from 0.5 to 1.0 with 10 steps increasing by 0.05, and another from 1.0 to 2.0 with 10 steps increasing by 0.1.

Or does anyone have a better idea?
Comment 15 Umang Jain 2016-01-25 07:52:33 UTC
Created attachment 319661 [details] [review]
Adjust Contrast Slider

Implementation changed to non-linear scale, the contrast slider now, has the range from -1 to 1 with contrast=2^x , where x is the value in the slider. So that when the slider is at 0 (in the middle) we have contrast=1, min and max will be 0.5 and 2.
Comment 16 Debarshi Ray 2016-01-25 08:23:42 UTC
Review of attachment 319661 [details] [review]:

Thanks for the patch. Using a base-2 logarithmic scale is a nice idea. Some comments:

The commit message shouldn't exceed more than 72 characters per line. See: https://wiki.gnome.org/Git/CommitMessages

::: src/photos-tool-colors.c
@@ +87,1 @@
+  contrast = pow(2, gtk_range_get_value (GTK_RANGE (self->contrast_scale))) ;

(a) We need to #include <math.h> for the prototype/signature of pow.
(b) It would be better to use 2.0 instead of 2 because it expects a double, not an integer.
(c) The use of spaces is a bit off.
(d) I prefer to avoid nested function calls because it helps with debugging. So, can you use a separate variable to hold the value of gtk_range_get_value? You can call it something like contrast_log.
(e) We need to do the inverse of this operation (ie. log2) in photos_tool_colors_activate
Comment 17 Umang Jain 2016-01-26 13:02:27 UTC
Created attachment 319747 [details] [review]
Adjust Contrast Slider

Implementation changed to non-linear scale, the contrast slider now,
has the range from -1 to 1 with contrast=2^x , where x is the value
in the slider. So that when the slider is at 0 (in the middle) we
have contrast=1, min and max will be 0.5 and 2.
Comment 18 Debarshi Ray 2016-01-26 17:36:48 UTC
Review of attachment 319747 [details] [review]:

Thanks for the patch, Umang. This looks much better now.

::: src/photos-tool-colors.c
@@ +161,3 @@
     }
+  else
+      contrast = log2 (contrast_real);

Nitpick. The alignment is a bit off, and now that we have an else branch it is slightly more readable to re-arrange the if/else branches to avoid the negation operator.
Comment 19 Debarshi Ray 2016-01-26 17:37:48 UTC
Created attachment 319785 [details] [review]
tool-colors: Restrict the lower bound of the contrast slider

I made the above adjustments, tweaked the commit message a bit and pushed it.
Comment 20 Umang Jain 2016-02-12 20:37:10 UTC
Coming to "Enhance" tool, some reviewing had been done regarding "Denoise".

By applying "Denoise" both in gnome-photos and GIMP, on the test images**, following observations are made:

1) Range of GIMP's "Denoise Strength" is [0,32]*. For Photos, its [0,8] till this point.

2) After testing the images with GIMP for the range [0,16], the results are promising. Therefore, it is decided to push the "Denoise" upper limit for gnome-photos to [0,16].

*[0,32] 
From #photos: 
<rishi> GIMP uses [0, 8] on the slider but it can be forced to go up to 32.
<rishi> It is intentional. You can force it beyond the visual range.

**Test images:
1) https://people.sc.fsu.edu/~jburkardt/m_src/image_denoise/balloons_noisy.png
2) http://videoclarity.com/wp-content/uploads/2013/03/im_sp1.png
3) http://blog.neocamera.com/images/ng_bwgirl_crop.jpg
Comment 21 Umang Jain 2016-02-12 22:36:30 UTC
Created attachment 321047 [details] [review]
tool-enhance, denoise: Increase the upper bound of denoise slider

While testing images with GIMP, it is realized
that increasing the upper bound of denoise from [0,8]
to [0,16] can provide more enhancement in noise-reduction
of the image in gnome-photos.
Comment 22 Debarshi Ray 2016-02-13 00:31:14 UTC
Review of attachment 321047 [details] [review]:

::: src/photos-tool-enhance.c
@@ +63,2 @@
 static const gdouble DENOISE_ITERATIONS_MINIMUM = 0.0;
+static const gdouble DENOISE_ITERATIONS_STEP = 1.0;

The fact that gegl:noise-reduction cannot accept non-integer values and yet we were using non-integer increments is a bug in itself. Please use a separate patch. I would first fix the step and then increase the maximum.
Comment 23 Umang Jain 2016-02-13 12:09:09 UTC
Created attachment 321068 [details] [review]
tool-enhance, denoise: Increase the gegl:noise-reduction step from 0.5 to 1.0

gegl:noise-reduction step does not expect non-integer values yet
we were using non-integer increments. In order to change the
noise-reduction range from [0,8] to [0,16], the increment in step
was recommended in a separate patch.
Comment 24 Umang Jain 2016-02-13 12:13:21 UTC
Created attachment 321069 [details] [review]
tool-enhance, denoise: Increase the upper bound of denoise slider

While testing images with GIMP, it is realized that increasing
the upper bound of denoise from [0,8] to [0,16] can provide more
enhancement in noise-reduction of the image in gnome-photos.
Comment 25 Debarshi Ray 2016-02-14 19:27:38 UTC
Review of attachment 321068 [details] [review]:

Looks good. Pushed. Thanks.
Comment 26 Debarshi Ray 2016-02-14 19:53:58 UTC
Review of attachment 321069 [details] [review]:

Looks good. Pushed.
Comment 27 Jakub Steiner 2016-02-15 14:17:24 UTC
brightness, contrast & saturation ranges feel good now. So does sharpen on the samples I've tried. Denosify I need to test on some poor photos. Will have to shoot some poor pictures for a change :)
Comment 28 Debarshi Ray 2016-02-15 14:33:31 UTC
Let's close this bug, then. We can always re-open later.

Thanks for all the patches and feedback!