Bug 497363. qcms: Use a minimum of 256 entries when calling invert_lut. r=joedrew, a=beltzner
authorJeff Muizelaar <jmuizelaar@mozilla.com>
Fri, 17 Jul 2009 17:30:56 -0400
changeset 26078 cb58df6e256c72cf77e332454e6c3699c31f1564
parent 26077 bd36744074882f1e3f98be47906dd5fc0e4e3778
child 26079 d1c4c407468cb7770f5d5e43517fa8e445ae0878
push id1781
push userjmuizelaar@mozilla.com
push dateMon, 20 Jul 2009 15:02:35 +0000
reviewersjoedrew, beltzner
bugs497363
milestone1.9.1.1pre
Bug 497363. qcms: Use a minimum of 256 entries when calling invert_lut. r=joedrew, a=beltzner Also includes: - Add reference to the rationale for not correcting alpha - Fix a memory leak when fread() fails
gfx/qcms/iccread.c
gfx/qcms/transform.c
--- a/gfx/qcms/iccread.c
+++ b/gfx/qcms/iccread.c
@@ -760,18 +760,20 @@ qcms_profile* qcms_profile_from_file(FIL
 		return NO_MEM_PROFILE;
 
 	/* copy in length to the front so that the buffer will contain the entire profile */
 	*((__be32*)data) = length_be;
 	remaining_length = length - sizeof(length_be);
 
 	/* read the rest profile */
 	read_length = fread((unsigned char*)data + sizeof(length_be), 1, remaining_length, file);
-	if (read_length != remaining_length)
+	if (read_length != remaining_length) {
+		free(data);
 		return INVALID_PROFILE;
+	}
 
 	profile = qcms_profile_from_memory(data, length);
 	free(data);
 	return profile;
 }
 
 qcms_profile* qcms_profile_from_path(const char *path)
 {
--- a/gfx/qcms/transform.c
+++ b/gfx/qcms/transform.c
@@ -542,27 +542,40 @@ qcms_bool set_rgb_colorants(qcms_profile
 
 	profile->blueColorant.X = double_to_s15Fixed16Number(colorants.m[0][2]);
 	profile->blueColorant.Y = double_to_s15Fixed16Number(colorants.m[1][2]);
 	profile->blueColorant.Z = double_to_s15Fixed16Number(colorants.m[2][2]);
 
 	return true;
 }
 
-static uint16_t *invert_lut(uint16_t *table, int length)
+/*
+ The number of entries needed to invert a lookup table should not
+ necessarily be the same as the original number of entries.  This is
+ especially true of lookup tables that have a small number of entries.
+
+ For example:
+ Using a table like:
+    {0, 3104, 14263, 34802, 65535}
+ invert_lut will produce an inverse of:
+    {3, 34459, 47529, 56801, 65535}
+ which has an maximum error of about 9855 (pixel difference of ~38.346)
+
+ For now, we punt the decision of output size to the caller. */
+static uint16_t *invert_lut(uint16_t *table, int length, int out_length)
 {
 	int i;
-	/* for now we invert the lut by creating a lut of the same size
+	/* for now we invert the lut by creating a lut of size out_length
 	 * and attempting to lookup a value for each entry using lut_inverse_interp16 */
-	uint16_t *output = malloc(sizeof(uint16_t)*length);
+	uint16_t *output = malloc(sizeof(uint16_t)*out_length);
 	if (!output)
 		return NULL;
 
-	for (i = 0; i < length; i++) {
-		double x = ((double) i * 65535.) / (double) (length - 1);
+	for (i = 0; i < out_length; i++) {
+		double x = ((double) i * 65535.) / (double) (out_length - 1);
 		uint16_fract_t input = floor(x + .5);
 		output[i] = lut_inverse_interp16(input, table, length);
 	}
 	return output;
 }
 
 static uint16_t *build_linear_table(int length)
 {
@@ -650,16 +663,22 @@ static void qcms_transform_data_gray_out
 		out_device_b = lut_interp_linear(linear, transform->output_gamma_lut_b, transform->output_gamma_lut_b_length);
 
 		*dest++ = clamp_u8(out_device_r*255);
 		*dest++ = clamp_u8(out_device_g*255);
 		*dest++ = clamp_u8(out_device_b*255);
 	}
 }
 
+/* Alpha is not corrected.
+   A rationale for this is found in Alvy Ray's "Should Alpha Be Nonlinear If
+   RGB Is?" Tech Memo 17 (December 14, 1998).
+	See: ftp://ftp.alvyray.com/Acrobat/17_Nonln.pdf
+*/
+
 static void qcms_transform_data_graya_out_lut(qcms_transform *transform, unsigned char *src, unsigned char *dest, size_t length)
 {
 	int i;
 	for (i = 0; i < length; i++) {
 		float out_device_r, out_device_g, out_device_b;
 		unsigned char device = *src++;
 		unsigned char alpha = *src++;
 
@@ -1281,20 +1300,27 @@ void compute_precache_linear(uint8_t *ou
 
 qcms_bool compute_precache(struct curveType *trc, uint8_t *output)
 {
 	if (trc->count == 0) {
 		compute_precache_linear(output);
 	} else if (trc->count == 1) {
 		compute_precache_pow(output, 1./u8Fixed8Number_to_float(trc->data[0]));
 	} else {
-		uint16_t *inverted = invert_lut(trc->data, trc->count);
+		uint16_t *inverted;
+		int inverted_size = trc->count;
+		//XXX: the choice of a minimum of 256 here is not backed by any theory, measurement or data, however it is what lcms uses.
+		// the maximum number we would need is 65535 because that's the accuracy used for computing the precache table
+		if (inverted_size < 256)
+			inverted_size = 256;
+
+		inverted = invert_lut(trc->data, trc->count, inverted_size);
 		if (!inverted)
 			return false;
-		compute_precache_lut(output, inverted, trc->count);
+		compute_precache_lut(output, inverted, inverted_size);
 		free(inverted);
 	}
 	return true;
 }
 
 
 // Determine if we can build with SSE2 (this was partly copied from jmorecfg.h in
 // mozilla/jpeg)
@@ -1357,30 +1383,33 @@ static qcms_bool sse2_available(void)
                      has_sse2 = 0;
        }
 
        return has_sse2;
 #endif
        return false;
 }
 
-
 void build_output_lut(struct curveType *trc,
 		uint16_t **output_gamma_lut, size_t *output_gamma_lut_length)
 {
 	if (trc->count == 0) {
 		*output_gamma_lut = build_linear_table(4096);
 		*output_gamma_lut_length = 4096;
 	} else if (trc->count == 1) {
 		float gamma = 1./u8Fixed8Number_to_float(trc->data[0]);
 		*output_gamma_lut = build_pow_table(gamma, 4096);
 		*output_gamma_lut_length = 4096;
 	} else {
-		*output_gamma_lut = invert_lut(trc->data, trc->count);
+		//XXX: the choice of a minimum of 256 here is not backed by any theory, measurement or data, however it is what lcms uses.
 		*output_gamma_lut_length = trc->count;
+		if (*output_gamma_lut_length < 256)
+			*output_gamma_lut_length = 256;
+
+		*output_gamma_lut = invert_lut(trc->data, trc->count, *output_gamma_lut_length);
 	}
 
 }
 
 void qcms_profile_precache_output_transform(qcms_profile *profile)
 {
 	/* we only support precaching on rgb profiles */
 	if (profile->color_space != RGB_SIGNATURE)