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

[Soeren Sandmann]

Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:

> >  15 files changed, 743 insertions(+), 470 deletions(-)
> 
> On the other hand, I'm not so happy about the implementation. The number of
> lines of code as well as the complexity has increased, and the performance
> seems to have dropped a little bit in some of the cases. 

The performance numbers you posted seem very close to noise to me; in
some cases the new code is faster. Considering that this was measured
with the CPU specific fast paths turned off, most users won't actually
see even this small slowdown.

> I really want to see some practical benefits which depend on these
> added iterators in the near future.

It isn't clear to me what specifically you are suggesting.

One practical benefit of the patch set is that it fixes the bug where
destination properties aren't ignored as they should be. This bug
could probably be fixed some other way, but it doesn't seem to be
possible with separating source- and destination accessors in some
way. This separation is where most of the additional new lines are
coming from.

In fact, as far as I can tell, the iterators make the code _less_
complex. Compare the new loop in pixman-general.c:

    for (i = 0; i < height; ++i)
    {
        uint32_t *s, *m, *d;

        m = mask_iter.get_scanline (&mask_iter, NULL);
        s = src_iter.get_scanline (&src_iter, m);
        d = dest_iter.get_scanline (&dest_iter, NULL);

        compose (imp->toplevel, op, d, s, m, width);

        dest_iter.write_back (&dest_iter);
    }

with the old one:

    for (i = 0; i < height; ++i)
    {
        /* fill first half of scanline with source */
        if (fetch_src)
        {
            if (fetch_mask)
            {
                /* fetch mask before source so that fetching of
                   source can be optimized */
                fetch_mask (mask, mask_x, mask_y + i,
                            width, (void *)mask_buffer, 0);

                if (mask_class == SOURCE_IMAGE_CLASS_HORIZONTAL)
                    fetch_mask = NULL;
            }

            if (src_class == SOURCE_IMAGE_CLASS_HORIZONTAL)
            {
                fetch_src (src, src_x, src_y + i,
                           width, (void *)src_buffer, 0);
                fetch_src = NULL;
            }
            else
            {
                fetch_src (src, src_x, src_y + i,
                           width, (void *)src_buffer, (void
            *)mask_buffer);
            }
        }
        else if (fetch_mask)
        {
            fetch_mask (mask, mask_x, mask_y + i,
                        width, (void *)mask_buffer, 0);
        }

        if (store)
        {
            /* fill dest into second half of scanline */
            if (fetch_dest)
            {
                fetch_dest (dest, dest_x, dest_y + i,
                            width, (void *)dest_buffer, 0);
            }

            /* blend */
            compose (imp->toplevel, op,
                     (void *)dest_buffer,
                     (void *)src_buffer,
                     (void *)mask_buffer,
                     width);

            /* write back */
            store (&(dest->bits), dest_x, dest_y + i, width,
                   (void *)dest_buffer);
        }
        else
        {
            /* blend */
            compose (imp->toplevel, op,
                     bits + (dest_y + i) * stride + dest_x,
                     (void *)src_buffer, (void *)mask_buffer, width);
        }
    }



Soren