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.

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.

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.

Created attachment 319133 [details] [review]
tool-colors: Restrict the range of the brightness slider

tool-colors: Restrict the range of the brightness slider

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.

Created attachment 319140 [details] [review]
tool-colors: Restrict the range of the brightness slider

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.

Created attachment 319142 [details] [review]
tool-colors: Restrict the range of the brightness slider

Created attachment 319145 [details] [review]
tool-colors: Restrict the range of the brightness slider

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.

Created attachment 319167 [details] [review]
tool-colors: Restrict the range of the brightness slider

Created attachment 319168 [details] [review]
tool-colors: Restrict the range of the brightness slider

Created attachment 319170 [details] [review]
tool-colors: Restrict the range of the brightness slider

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?

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.

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

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.

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.

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.

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

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.

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.

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.

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.

Review of attachment 321068 [details] [review]:

Looks good. Pushed. Thanks.

Review of attachment 321069 [details] [review]:

Looks good. Pushed.

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 :)

Let's close this bug, then. We can always re-open later.

Thanks for all the patches and feedback!