[Pixman] sRGB processing for pixman
Antti S. Lankila
alankila at bel.fi
Sat Jun 16 14:09:19 PDT 2012
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
> 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
> - 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
> - 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
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 42077 bytes
Desc: not available
More information about the Pixman