[Pixman] [PATCH 1/4] bits: Implement PAD support in the simple fetcher

[Søren Sandmann]

Chris Wilson <chris at chris-wilson.co.uk> writes:

>> Some coding style issues in this one: no braces around single lines,
>> spaces around -.
>
> "image->width-1" right, just checking I haven't misinterpreted.

Yeah.

>> But I'm curious what particular operation hits this. Usually PAD
>> repeating is most useful with scaling, but this is an untransformed
>> fetcher.
>
> Not much I can add to this one based on the inspection of the call
> stack, it is a regular pixel aligned cairo_fill hitting the
> pixman_image_composite32() fast path. However, I can add that firefox
> likes to default to PAD for its images. (Note that this benchmark
> appears to have been written to demonstrate a few bugs in firefox, so I
> would not rule out that the composite() should have ideally been clipped
> to the source image.)

It's simply that the speedup you got:

    SNB i5-2500s: firefox-chalkboard  25.9s -> 19.6s: 1.32x speedup

is rather large for a change that doesn't even introduce a fast path or
use SIMD. That suggests either that something dumb is going on in pixman
or in the benchmark, or that we want SIMD variants of this operation.

> #1  0xb7e2c755 in general_composite_rect (imp=0x8058008, info=0xbfffe234)
>     at pixman-general.c:241
...
>         width = 2560
>         height = 1439

So this is a very large operation which explains some of the
speedup. How big is the source image here? If it's the same size or
bigger than the destination, something may be going wrong in
analyze_extents() in pixman-composite.c, which should detect that we
never access the image outside the sample grid and therefore hit the
standard fast paths.

On the other hand, if the source is smaller than the destination, then
hitting the general case is expected, but then you have to wonder why
PAD is being used, since that will just cause a bunch of stripes to be
composited. And if this operation really is the desired one, then we'd
ideally want a SIMD fast path for (or at least an iterator so that the
scanlines outside the images could be generated from the same buffer.

So anyway, I'm not opposed to this patch; the numbers just suggest that
some deeper investigation is warranted.


Søren