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

[Andrea Canciani]

On Sat, Jan 8, 2011 at 12:15 PM, Soeren Sandmann <sandmann at cs.au.dk> wrote:
>> Iterators gets initialized with a buffer which can always contain at
>> least "width" pixels. This is sufficient for current image types,
>> but future image types might need additional space.
>>
>> Would it be possible to have the general_composite_rect ask to the
>> iterator about how much memory it will need (and provide a buffer
>> with at least this much memory)?
>
> If future images need something like this, the iterators can be
> extended to support it. It's always difficult to predict what
> infrastructure might be needed in the future.
>
>> Or would it be better to have the iterator allocate that memory internally
>> upon initialization?
>> In this case, we should probably have the initializer return success/error
>> and also have some kind of iter_fini()
>
> Right, that's another possibility, although if memory allocation
> fails, the image might also simply install a noop() fetcher. If we get
> out-of-memory errors during rendering, we generally don't guarantee
> the results, except that it obeys they clipping rules. This is
> something pixman has inherited from the X server.
>
>> Apart from this, I skimmed through the patches and noticed that
>> sometimes a change is fixed or improved by a following patch
>> (example: patch 15/18 is immediately generalized by patch 16/18). Is
>> this intentional?
>
> Yeah, generally, I'm trying to avoid patches being too
> "clairvoyant". In principle, new infrastructure shouldn't be
> introduced until something else makes it necessary. It isn't always
> possible or convenient to do it that way, though.
>
> Wrt. localized alpha:
>
>> I thought I understood the definition after the first two lines, then
>> the OVER/ADD explanation confused me. After reading again everything,
>> I was convinced again that I had understood it correctly the first
>> time.
>>
>> I don't know if this is actually better (I still think that it needs a
>> better wording), but I'd suggest something like:
>>
>> +    /* "Separable alpha" is when the alpha channel is used only to compute the
>> +     * alpha value of the destination. This means that the computation of the
>> +     * RGB values of the result is independent of the alpha value.
>> +     *
>> +     * For example, the OVER operator has separable alpha for the destination,
>> +     * because the RGB values of the result can be computed without knowing
>> +     * the destination alpha. Similarly, ADD has separable alpha for both source
>> +     * and destination because the RGB values of the result can be
>> +     * computed without knowing the alpha value of source or destination.
>>
>> I'm proposing the "separable" term because it is consistent with
>> "separable blend modes" (compositing operators in which each channel
>> can be computed individually), whereas "localized" didn't tell me
>> anything until I read the explanation.
>
> I think your new description is better, but I don't like using the
> word "separable" for two things that are not really the same. A
> "separable" operator is one that can be applied separately for each R,
> G, and B channel, whereas the "localized alpha" means that you can
> compute the R, G, and B channels independent from the alpha channel in
> question.
>
> I have pushed a new branch here:
>
>    http://cgit.freedesktop.org/~sandmann/pixman/log/?h=iterators4
>
> that shold take care of all your other comments. In addition, I also
> squashed together moving the gradient initializers; there didn't seem
> to be much gained from having them in separate commits.

Thank you! I find this branch much easier to read.

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)

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.

<nitpicking>

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)

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().;"

</nitpicking>

This time I have read the whole patchset as carefully as possible, so I hope
I won't notice anything else. Sorry for not having noticed these in iterators3,
but I had only skimmed through it.

Andrea