[Pixman] [PATCH 0/18] Add iterators to images

Soeren Sandmann sandmann at cs.au.dk
Sun Jan 9 20:25:13 PST 2011

Andrea Canciani <ranma42 at gmail.com> writes:

> In "Move get_scanline_32/64 to the bits part of the image struct."
> (commit ffbd7a2722fff502f1ad6200ba3a009704df9d66)
> _pixman_image_get_scanline_generic_64 could easily be improved
> by pixman_contract()-ing mask8 only if it is not NULL instead of returning
> in case of an allocation failure (i.e. if it can't malloc, it fetches the whole
> row, which is slower but correct).
> (This change might belong to a different commit, because the "wrong" code
> is already present in pixman/master and your commit was only moving
> it)

It's another case of misrendering because of OOM, but I guess the fix
here is simple enough. I do think this would belong in a separate
commit - and isn't really related to this particular patch series.

> In "Skip fetching pixels when possible" (commit
> b2842d6d2b9ab7b1380c3cb95956dc886fcd1ee7), would it be possible to
> use get_scanline_null() in _pixman_implementation_src_iter_init()
> when the flags are NOOP or, which is the same, to replace both src
> and mask with NULL when their flags are NOOP?
> This won't give a performance boost (the noop and null iterators should
> have very similar speed), but it should immediately point out if a compositing
> function incorrectly tries to access data which it should be
> ignoring.

Generally, bugs in pixman should not crash the X server, if it can be
at all avoided. Asserts are disabled for that reasons. Instead it is
better to call _pixman_log_error() to print spew to stderr. This will
show up in the X log file, I believe.

In this case, using the NULL fetcher wouldn't actually be
correct. Code for a CLEAR combiner might do something like this:

        f1 = 0;
        f2 = 0;

        s = *src;
        d = *dst;

        *dst = s * f1 + d * f2;

which would crash unless the compiler optimized the memory reads away.

> In ""Skip fetching pixels when possible" it would also be nice to
> have the "NOOP" macro defined once and used everywhere
> (pixman-bits-image.c, pixman-general.c, pixman-implementation.c)

I actually think it's better to just expand the macro in
pixman-implementation.c, rather than using one in
pixman-bits-image.c. The name "NOOP" was mainly used as a shorthand,
not as the definition of an interesting concept.

> The punctuation is slightly inconsistent between different commit
> messages and in "Linear: Optimize for horizontal gradients."
> (46a0115bf62fb61da357c3ece654b57c9947202c) the commit message ends
> with "... _pixman_linear_gradient_iter_init().;"

I have updated the iterators4 branch with a fix for the "().;" issue
and a couple of other things in the commit messages.

While I know the kernel is obsessive-compulsive about punctuation in
their commit messages, for pixman I'm not convinced we need to very
rigid rules or require people to write perfect English.


More information about the Pixman mailing list