[Pixman] sRGB processing for pixman

[Søren Sandmann]

"Antti S. Lankila" <alankila at bel.fi> writes:

> Can do. However, there will be a few places then which need to be
> taught about the new ARGB_SRGB pixel type, unless we also modify the
> PIXMAN_FORMAT_TYPE to filter out the color space bits. I did this to
> keep changes minimal. I added the PIXMAN_FORMAT_SPACE macro,
> however. I adopted your implicit suggestion of splitting the 8-bit
> type value to 3+5 bits.

I don't see the point of this macro. If we ever add more color spaces,
this macro will likely become meaningless. Is there anything wrong with
simply testing PIXMAN_FORMAT_TYPE (format) == PIXMAN_TYPE_ARGB_SRGB?

>> - There are formatting issues. Please see COPYING.
>
> I'll try to hone the whitespace arrangement to match rest of the
> code. I think this version attached should be acceptable. Please tell
> me what you think.

Some bug in my brain caused me to write "COPYING" where I obviously
meant "CODING_STYLE". It looks like in the new patch, you consistently
put braces on the same line as if/while/for. As CODING_STYLE points out,
and as all the surrounding code shows, pixman puts braces on their own
line. There may be other coding style issues that I missed.

> +# 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 srgb2lin table, and keeps the actual
> +# table lookup in the other direction as simple as possible.
> +for my $x (0 .. $#srgb2lin) {
> +	my $add = 0;
> +	while (1) {
> +		my $lin = $srgb2lin[$x];
> +		my $x2 = $lin2srgb[$lin >> 4];
> +		if ($x == $x2) {
> +			last;
> +		}
> +		# Add slight bias to this component until it rounds correctly
> +		$srgb2lin[$x] ++;
> +		$add ++;
> +	}
> +	die "Too many adds at $x" if $add > 5;
> +}
> +
> +print <<"PROLOG";
> +/* WARNING: This file is generated by $0.
> +   Please edit that file instead of this one. */
> +
> +#include <stdint.h>
> +#include "pixman-srgb.h"
> +
> +PROLOG
> +print "const uint8_t lin2srgb[" . @lin2srgb . "] = {\n";

I think the name here should be spelled out: linear_to_srgb[] instead of
lin2srgb.

> +print "const uint16_t srgb2lin[" . @srgb2lin . "] = {\n";

and similarly here.

>  /*
>   * XXX: The transformed fetch path only works at 32-bpp so far.  When all
>   * paths have wide versions, this can be removed.
> @@ -1079,6 +1153,13 @@ static const format_info_t accessors[] =
>      FORMAT_INFO (r8g8b8x8),
>      FORMAT_INFO (x14r6g6b6),
>  
> +/* sRGB formats */
> +  { PIXMAN_a8r8g8b8_sRGB,
> +    fetch_scanline_a8r8g8b8,
> +    fetch_scanline_generic_64_sRGB,
> +    fetch_pixel_a8r8g8b8, fetch_pixel_generic_64_sRGB,
> +    store_scanline_a8r8g8b8, store_scanline_generic_64_sRGB },

I don't think this is correct. 

Basically, what these fetchers do is convert from an external format to
pixman's internal format. The two internal formats that pixman supports
currently are 8 bpc linear and 16 bpc linear. That means all the
fetchers for this format must convert from sRGB to linear, and all the
storers must convert the other way.

For the 8 bpc fetcher converting from 8 bit srgb to 8 bit linear loses a
lott of information, which is why the format is considered wide. But if
it somehow ends up being used with the 8 bit pipe (and currently it
will, because at the moment everything with a transformation ignores the
wide path), the pixels still need to be linear. We may need a 256->256
table doing 8-bit sRGB to 8-bit linear.

Finally, note that these functions should not access pixels directly;
instead they need to go through the READ and WRITE macros. Otherwise,
pixman_set_accessors() won't work. To test this, you can add the new
format to the table in stress-test.c.

> --- /dev/null
> +++ b/pixman/pixman-srgb.c
> --- /dev/null
> +++ b/pixman/pixman-srgb.h

Generated code should not be checked into the repository, and I don't
think we need a separate pixman-srgb.h file. The table can just be
declared in pixman-private.h


Thanks,
Søren