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

[Siarhei Siamashka]

On Wednesday 12 January 2011 12:46:39 Soeren Sandmann wrote:
> 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.

Performance numbers for some of the tests look like they are more or less
statistically relevant. It's not a big slowdown, but if such changes keep
accumulating, then we got a problem.

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

More like I'm trying to figure out the purpose of all these changes and their
place in the long term plans. If this code is only related to general
implementation which just needs to be simple, clean and maintainable, then it
is perfectly fine. If the iterators are also going to be used in the fast path
functions eventually, then they are better to be also really fast.

> 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);
>         }
>     }

The old code had more lines of code for this part, but on the other hand all
this code was in a single place as opposed to being spread around. In some
ways, the new code is a bit more limiting. For example, fetching directly to
destination without intermediate buffers for SRC operator might be more tricky
to implement.

Overall, the patch set looks good. It's just not everything seems to be rosy
to me, I can see (and accept) some of the compromises it introduces.

-- 
Best regards,
Siarhei Siamashka