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

[Soeren Sandmann]

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.


Thanks,
Soren