[qcms] Keep the output of the TRC between 0 and 1

third_party/qcms/src/transform_util.c

Index: third_party/qcms/src/transform_util.c
diff --git a/third_party/qcms/src/transform_util.c b/third_party/qcms/src/transform_util.c
index 5cb45f8419e81132b4176eb9cf076bb740c28a97..9b435f95b7995befecad416f949c3ac1a2a0d53d 100644
--- a/third_party/qcms/src/transform_util.c
+++ b/third_party/qcms/src/transform_util.c
@@ -36,16 +36,18 @@
 
 /* value must be a value between 0 and 1 */
 //XXX: is the above a good restriction to have?
-float lut_interp_linear(double value, uint16_t *table, size_t length)
+// the output range of this function is 0..1

[Matt Giuca, 2015/07/01 01:46:59]

The upstream patch says "range of this functions".

[Noel Gordon, 2015/07/01 04:12:22]

well "... the output range of this functions is 0..1", to be precise and we
prefer English.

[Matt Giuca, 2015/07/01 07:10:50]

If this were an ordinary code review, I'd tell the author to fix his grammar.
But you aren't the author, you are just checking in code, and therefore you
shouldn't change it unless you have a good reason.

(This is not our code so it is not up to us to decide what grammar standards to
uphold.)

+float lut_interp_linear(double input_value, uint16_t *table, size_t length)

[Matt Giuca, 2015/07/01 01:46:59]

Why is length size_t instead of int? (The upstream is int.) Is this a local bug
fix? Could it be upstreamed?

[Noel Gordon, 2015/07/01 04:12:22]

Local bug fix, upstream was advised iirc and is still not fixed.  We want size_t
here.

[Matt Giuca, 2015/07/01 07:10:50]

Acknowledged.

 {
 	int upper, lower;
-	value = value * (length - 1); // scale to length of the array
-	upper = ceil(value);
-	lower = floor(value);
+	float value;
+	input_value = input_value * (length - 1); // scale to length of the array
+	upper = ceil(input_value);
+	lower = floor(input_value);
 	//XXX: can we be more performant here?
-	value = table[upper]*(1. - (upper - value)) + table[lower]*(upper - value);
+	value = table[upper]*(1. - (upper - input_value)) + table[lower]*(upper - input_value);
 	/* scale the value */
-	return value * (1./65535.);
+	return value * (1.f/65535.f);
 }
 
 /* same as above but takes and returns a uint16_t value representing a range from 0..1 */
@@ -120,15 +122,17 @@ uint16_t lut_interp_linear16(uint16_t input_value, uint16_t *table, int length)
 }
 #endif
 
-void compute_curve_gamma_table_type1(float gamma_table[256], double gamma)
+void compute_curve_gamma_table_type1(float gamma_table[256], uint16_t gamma)
 {
 	unsigned int i;
+	float gamma_float = u8Fixed8Number_to_float(gamma);
 	for (i = 0; i < 256; i++) {
-		gamma_table[i] = pow(i/255., gamma);
+		// 0..1^(0..255 + 255/256) will always be between 0 and 1

[Matt Giuca, 2015/07/01 01:46:59]

The upstream patch has 16 spaces here, not 2 tabs.

:(

[Noel Gordon, 2015/07/01 04:12:22]

Acknowledged.  The qcms story re tabs and spaces is not good and it comes up in
moz reviews often [1].  So for at least some consistency, tabs win sadly.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=678324#c23

[Matt Giuca, 2015/07/01 07:10:50]

Absolutely ... if you were writing code for Mozilla you should be consistent
with the surrounding code (and use tabs).

But you aren't writing this code, you are just checking it in to Chromium, and
therefore you should not change it unless you have a good reason to do so.

+		gamma_table[i] = pow(i/255., gamma_float);
 	}
 }
 
-void compute_curve_gamma_table_type2(float gamma_table[256], uint16_t *table, int length)
+void compute_curve_gamma_table_type2(float gamma_table[256], uint16_t *table, size_t length)

[Matt Giuca, 2015/07/01 01:46:59]

The upstream patch doesn't change "int length" to "size_t length".

[Noel Gordon, 2015/07/01 04:12:22]

Refer back to the reply for line 40, we want size_t.

[Matt Giuca, 2015/07/01 07:10:50]

Acknowledged.

 {
 	unsigned int i;
 	for (i = 0; i < 256; i++) {
@@ -191,9 +195,9 @@ void compute_curve_gamma_table_type_parametric(float gamma_table[256], float par
                         // XXX The equations are not exactly as definied in the spec but are
                         //     algebraic equivilent.
                         // TODO Should division by 255 be for the whole expression.
-                        gamma_table[X] = pow(a * X / 255. + b, y) + c + e;
+                        gamma_table[X] = clamp_float(pow(a * X / 255. + b, y) + c + e);
                 } else {
-                        gamma_table[X] = c * X / 255. + f;
+                        gamma_table[X] = clamp_float(c * X / 255. + f);
                 }
         }
 }
@@ -206,15 +210,26 @@ void compute_curve_gamma_table_type0(float gamma_table[256])
 	}
 }
 
-
 float clamp_float(float a)
 {
-        if (a > 1.)
-                return 1.;
-        else if (a < 0)
-                return 0;
-        else
-                return a;
+	/* One would naturally write this function as the following:
+	if (a > 1.)
+		return 1.;
+	else if (a < 0)
+		return 0;
+	else
+		return a;
+
+	However, that version will let NaNs pass through which is undesirable
+	for most consumers.
+	*/
+
+	if (a > 1.)
+		return 1.;
+	else if (a >= 0)
+		return a;
+	else // a < 0 or a is NaN
+		return 0;
 }
 
 unsigned char clamp_u8(float v)
@@ -263,7 +278,7 @@ float *build_input_gamma_table(struct curveType *TRC)
 			if (TRC->count == 0) {
 				compute_curve_gamma_table_type0(gamma_table);
 			} else if (TRC->count == 1) {
-				compute_curve_gamma_table_type1(gamma_table, u8Fixed8Number_to_float(TRC->data[0]));
+				compute_curve_gamma_table_type1(gamma_table, TRC->data[0]);
 			} else {
 				compute_curve_gamma_table_type2(gamma_table, TRC->data, TRC->count);
 			}