[Pixman] [PATCH] Don't discriminate PAD and REFLECT repeat in standard fast paths

[Siarhei Siamashka]

On Wednesday 22 September 2010 17:56:20 Soeren Sandmann wrote:
> Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:
> > But right now setting PAD repeat for everying will cause serious
> > performance problems for pixman, because it will stop using simple
> > non-transformed fast paths.
> > 
> > This patch tries to address the problem. It passes current pixman tests
> > and I think that it is most likely fine. But I'm still not totally sure
> > if it is really safe in all possible cases.
> 
> I can't think of a case where it would be broken, but I don't think
> it's really *right* either. The COVERS_CLIP flag is supposed to
> indicate that the image doesn't have any undefined parts, which is
> certainly the case when you have PAD or REFLECT set.

I don't think the patch is more broken than the current code. Both NONE
and PAD repeats are very similar, as they both define the values of pixels
outside image boundaries according to some predefined rules. Both can't
be used with simple non-transformed fast paths if the operation involves
accessing any pixels outside image boundaries. So in my opinion treating
them equally in this part of code is the right thing. Even though this
treatment itself is not perfect and can be improved as you explained below.

Would it be a bad idea to apply this patch now, and then proceed with
a larger scale refactoring next?

> The fundamental problem is that the PIXMAN_STD_FAST_PATH macro is
> mixing up several things. There are two kinds of images involved in
> the standard fast paths: solids and bits. The problem is that the
> required flags for those images are quite different.
> 
> A solid image has basically no requirements on the flags other than
> the usual NO_ALPHAMAP, NO_CONVOLUTION etc. Generally, as long as it's
> a solid color, the fast path will work.
> 
> The bits images have basically the requirement that we now call
> SAMPLES_COVER_CLIP. That allows the biltters to simply walk the image
> without worrying about reading out of bounds.
> 
> Then there is the 'simple repeat' stuff, which is an optimization that
> notices that in some cases you can do a repeat operation by repeating
> the compositing instead of sampling with wrap-around addressing. This
> code has survived since the dawn of time, when pixman was called fb,
> didn't have transformations, and NORMAL was the only repeat mode
> available. Back then this code was the way to do repeating.
> 
> On IRC you pointed out that for 1xn repeated images, this simple
> repeat stuff is not an optimization at all because it results in
> terrible memory access patterns. It has historically had other
> pathological cases, such as one it would end up calling a fast path a
> zillion times with a 1x1 rectangle.
> 
> In fact, it was never written as an optimization, it was only left in
> because it was assumed to be one. I don't think anyone has ever
> measured the impact compared to the general path.
> 
> Considering that it is really at the wrong level in the pipeline too
> (repeating conceptually happens during sampling, not after
> compositing), maybe it's time to retire this code. There are cases
> where it is probably still useful, but I'd rather avoid the headache
> of thinking about special cases where it can't or shouldn't be used.
> 
> So, basically, what I think could be done is:
> 
> - Change the PIXMAN_STD_FAST_PATH to put in different flags when an
>   image is solid vs. when it is bits. The flags in the solid case
>   would be just the usual ones, in the bits case, it would be the
>   usual ones plus SAMPLES_COVER_CLIP.
> 
> - Delete the COVERS_CLIP flag, since it would not be used in either
>   case and isn't used elsewhere.
> 
> - Delete the simple_repeat stuff (along with the SIMPLE_REPEAT flags)
> 
> If there are specific benchmarks where the simple_repeat stuff made a
> noticable difference, then it's probably better to just do a regular
> fast path for them. A scanline approach would likely be faster than a
> tiled approach anyway in those cases.

All of this looks like a good plan to me.

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20100923/9694f6b0/attachment-0001.pgp>