[Pixman] sRGB processing for pixman

[Antti Lankila]

Søren Sandmann kirjoitti 20.6.2012 kello 23.48:

> "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?

Okay. I have reworked the code to not define new macros, and simply defined it as a type.

> 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.

Um, right. Somehow I was entirely blind to the brace positions. They have now been corrected. I also adjusted shift width to 4 spaces for the perl program.

>> +print "const uint8_t lin2srgb[" . @lin2srgb . "] = {\n";
> 
> I think the name here should be spelled out: linear_to_srgb[] instead of
> lin2srgb.

Name of the table has been expanded, definition moved to pixman-private.h and the pixman-srgb.c is not included in version control anymore.

>> +  { 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.

Understood. I assumed the wide path is always used if format is declared that way, and tried to get by through piggybacking on the existing a8r8g8b8 storage handlers to keep the amount of added code minimal. I have now written all the code paths separately, including the narrow path accessors.

In truth the compose test should be expanded to exercise also the narrow path. I have not done so. Consequently, these narrow accessors are untested, even if they look pretty straightforward.

> 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.

Added & make check passed.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-support-for-sRGB-surfaces.patch
Type: application/octet-stream
Size: 22352 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20120621/519f734d/attachment-0001.obj>
-------------- next part --------------


-- 
Antti