[Pixman] sRGB processing for pixman

[Antti S. Lankila]

On 15.06.2012 22:40, Søren Sandmann wrote:
> Comments on the patch:
>
> - I think the code that generated the tables should be included, and the
>    tables generated as part of the build process.

Agree. I have reworked the build to do this.

> - We can't break source level compatibility. Instead of adding a new
>    field in the formats, how about simply adding
>
>         PIXMAN_TYPE_ARGB_SRGB
>
>    which is #defined to (PIXMAN_TYPE_ARGB | (1<<  5))? That still makes
>    it possible to determine quickly whether the format is srgb encoded,
>    but doesn't break source compatibility.

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.

> - expand/contract seems like the wrong place to do the conversion. It
>    makes more sense to me to do it directly in pixman-access.c for the
>    format in question. Conceptually, everything pixman does internally is
>    with linearly encoded pixels, and so expanding the output of an 8-bit
>    fetcher is not subject to gamma conversions.
>
>    Instead, gamma conversions belong as the first thing after fetching
>    and the last thing before storing.
>
>    That also means the 8 bit fetcher for this new format would be done by
>    (a) fetching wide, gamma-converted pixels, and then (b) contracting
>    them to 8 bit without gamma conversion. This loses a lot of
>    information of course, but that's why the format is considered "wide".

I have defined the relevant 64-bit fetchers now that do the conversion 
while loading, and keep my hands off pixman_expand/contract.

> - I'm not convinced that we need that pixman_image_get_space()
>    accessor. There is already a pixman_image_get_format() that can be
>    used to get the information.

Agreed, especially if Adrian Johnson is working on an alternative 
approach which may prove to be more flexible in the long run. The 
sRGB-based pixel fetcher and storer is nevertheless an isolated feature 
with fairly minimal impact to the rest of the code, but if Adrian's work 
goes in and we stop using sRGB as the primary cairo surface format for 
applications, instead opting for some 64-bit linear light color space 
instead, then this work should considered to be superseded by that 
optimal design.

> - The format should be added to the test suite, at the very least the
>    "composite" test.

Agree. I did this, and was able to see all composite tests pass. Not 
entirely without issues, though: because the test constructs all the 
colors from pixman fills, they will by construction always be linearly 
specified colors, because fills can not be specified in sRGB presently.

When placed on sRGB surface, the numeric pixel values of course change 
to reflect the source color's linear light nature. This however changes 
the quantization errors, and I had to simulate round_color() in sRGB 
space to get every subtest to pass. Pretty tricky, and I don't think I 
like the end code too much, but I will nevertheless submit it as it is now.

I believe that there should be ways to construct both fills and 
gradients in sRGB color space. Fills are simple to do, of course, but 
gradients are interpolated and this interpolation is presumably linear 
(I have not read the code). So a gradient constructed from sRGB points 
would appear to interpolate its colors in linear light when composed to 
sRGB destination, if we simply transform the gradient's input colors 
from sRGB to linear light on gradient construction time. This is 
probably how it should work, anyway.

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

-- 
Antti
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-support-for-sRGB-surfaces.patch
Type: text/x-patch
Size: 42077 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20120617/e961e848/attachment-0001.bin>