[Pixman] sRGB processing for pixman

[Søren Sandmann]

Antti Lankila <alankila at bel.fi> writes:

> From 693af3995b46d531342d68ac83b794113d0a7a90 Mon Sep 17 00:00:00 2001
> From: "Antti S. Lankila" <alankila at bel.fi>
> Date: Sun, 10 Jun 2012 22:31:38 +0300
> Subject: [PATCH] Add support for sRGB surfaces

This mostly looks good to me. The main highlevel comment I have is that
I'd like the changes to composite.c and the addition of srgb-test.c to
happen in separate commits.

I also have a few more specific comments below.

> +# Ensure that we have a lossless sRGB and back conversion loop.
> +# some of the darkest shades need a little bias -- maximum is just
> +# 5 increments out of 16. This gives us useful property with
> +# least amount of error in the sRGB-to-linear table, and keeps the actual
> +# table lookup in the other direction as simple as possible.
> +for my $srgb (0 .. $#srgb_to_linear)
> +{
> +    my $add = 0;
> +    while (1)
> +    {
> +	my $linear = $srgb_to_linear[$srgb];
> +	my $srgb_lossy = $linear_to_srgb[$linear >> 4];
> +	last if $srgb == $srgb_lossy;
> +
> +	# Add slight bias to this component until it rounds correctly
> +	$srgb_to_linear[$srgb] ++;
> +	$add ++;

I can't say I'm thrilled about this biasing going on here, but it is
important to support losssless roundtrips, and I can't think of a better
way to do it.

> diff --git a/test/composite.c b/test/composite.c
> index bdecd75..2900151 100644
> --- a/test/composite.c
> +++ b/test/composite.c
> @@ -50,6 +50,32 @@ static const color_t colors[] =
>      { 0.5, 0.0, 0.0, 0.5 },
>  };
>  
> +static double
> +convert_srgb_to_linear (double c)
> +{
> +    if (c <= 0.04045)
> +    {
> +	return c / 12.92;
> +    }
> +    else
> +    {
> +	return powf ((c + 0.055) / 1.055, 2.4);
> +    }
> +}
> +
> +static double
> +convert_linear_to_srgb (double c)
> +{
> +    if (c <= 0.0031308)
> +    {
> +	return c * 12.92;
> +    }
> +    else
> +    {
> +	return 1.055 * powf (c, 1.0/2.4) - 0.055;
> +    }
> +}

No braces when both branches are single lines.

>  static uint16_t
>  _color_double_to_short (double d)
>  {
> @@ -99,6 +125,9 @@ static const format_t formats[] =
>      P(x2b10g10r10),
>      P(a2r10g10b10),
>      P(a2b10g10r10),
> +    
> +    /* sRGB formats */
> +    P(a8r8g8b8_sRGB),
>  
>      /* 24 bpp formats */
>      P(r8g8b8),
> @@ -524,17 +553,8 @@ composite_test (image_t *dst,
>  		pixman_bool_t component_alpha,
>  		int testno)
>  {
> -    pixman_color_t fill;
>      color_t expected, tdst, tsrc, tmsk;
>      pixel_checker_t checker;
> -    pixman_image_t *solid;
> -
> -    /* Initialize dst */
> -    compute_pixman_color (dst->color, &fill);
> -    solid = pixman_image_create_solid_fill (&fill);
> -    pixman_image_composite32 (PIXMAN_OP_SRC, solid, NULL, dst->image,
> -			      0, 0, 0, 0, 0, 0, dst->size, dst->size);
> -    pixman_image_unref (solid);

Where did this initialization go? If this is an unrelated bugfix, it
should go in its own commit.

> @@ -580,6 +654,13 @@ composite_test (image_t *dst,
>  		  &expected,
>  		  component_alpha);
>  
> +    if (PIXMAN_FORMAT_TYPE (dst->format->format) == PIXMAN_TYPE_ARGB_SRGB)
> +    {
> +	expected.r = convert_linear_to_srgb (expected.r);
> +	expected.g = convert_linear_to_srgb (expected.g);
> +	expected.b = convert_linear_to_srgb (expected.b);
> +    }
> +
>      pixel_checker_init (&checker, dst->format->format);

It seems to me that it would make more sense to add support for sRGB
(including the gamma conversion) directly to pixel_checker_t instead of
doing the gamma conversion here. That way, it can be used in other tests
involving sRGB destinations too.


Soren