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

[Soeren Sandmann]

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

Patches to come in follow-up emails.


Thanks for the review,
Soren