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

[Siarhei Siamashka]

On Friday 24 September 2010 19:39:10 Soeren Sandmann wrote:
> Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:
> > 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.
> 
> You can make exactly the same argument about NORMAL: it defines values
> of pixels outside image boundaries according to some predefined
> rules.

Exactly. This is one of the possible interpretations. A bit more details below.

> The reason you can't remove COVERS_CLIP in that case is that solid
> images created by NORMAL repeating a 1x1 image is still a common way
> to generate solid images. But PAD and REFLECT *could* be used that way
> too, even if in practice they aren't.
>
> I guess that's an example of something that is in principle more
> broken with the patch that without: with the patch you can't use PAD
> and REFLECT with a 1x1 image to make solid images.

Well, you still can and it will even work. But admittedly it will be just
ridiculously slow. Flipping these flag bits just changes where we are going to
take serious performance hit: for nonscaled standard fast paths when the source
image has PAD/REFLECT repeat or for the solid sources which are represented by
1x1 images with PAD/REFLECT repeat

Anyway, that's a good point and a proof that this stuff really needs a better
fix. Having performance regressions for any use cases is not good.

> But my main objection is still that it uses the flags in a meaningless
> way that just happen to give a desired result. With the patch you
> can't assign any real meaning to the COVERS_CLIP flag.

Looking from the end of the pipeline, the COVERS_CLIP flag is set when there is 
no risk accessing pixels outside image boundaries from the fast path functions 
(which implies some knowledge about what fast path functions are actually
doing). And it is one of the requirements for selecting standard fast paths.

Standard fast paths can be used in the following two cases:
a) pixels outside image boundaries are not accessed
b) solid source (special cases of 1x1 image with repeat set)
And can't be when:
c) pixels outside image boundaries are accessed (excluding solid case)

The problem is that there is only a single flag bit to differentiate all
these three cases at the moment. It means that for each particular repeat type,
either (a) case is accelerated, or it is (b). But not both.

Before the patch it was:
NONE:    a)
PAD:     b)
REFLECT: b)
NORMAL:  b)

After the patch it is going to be:
NONE:    a)
PAD:     a)
REFLECT: a)
NORMAL:  b)

Either way is not optimal.

One of the possible solutions is probably to explicitly set COVERS_CLIP flag
when the image is known to be solid, but not just based on the repeat type.
Like in the attached patch variant.

But surely the problem can be solved better, additionally accelerating solid
case even when transformation is set.

> Ie., it's a hack to get around some admittedly not-too-well-though-out
> existing code, instead of fixing it.
> 
> > 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?
> 
> If I thought that a larger scale refactoring was likely to actually
> happen, then I'd be more willing to consider applying this patch.

The most difficult part is that any problem can be typically solved in many
different ways, with pretty much equal end result. So some misunderstandings
can be always possible unless a really unambiguous explanation is provided
about how exactly you would like to see it fixed. I understand that providing
such clear explanation may take even more time than fixing the problem
yourself and that's unfortunate.

I would actually prefer to see you taking care of all the general
infrastructure for the fast path code selection in pixman. Yes, maybe I'm
asking too much. But anybody else seems to be much less effective in this area
(other than providing test cases or prototype patches).

> But once the immediate symptom is solved, people inevitably end up not
> fixing the real problem.

Sometimes the problems need to be fixed fast. And if there are no other ill
symptoms, then it may be not so bad in the short run.

> Actually, I don't even think a large scale refactoring is necessary -
> most of the real fix is just deleting code. The main piece of new
> logic is the extending of PIXMAN_STD_FAST_PATH to generate different
> flags for solid and bits images.

Yes, agreed. I was more worried about removing the special case for the
NORMAL repeat because it may (or may not) cause some unpredictable performance
regressions in the short run. In the long run, it indeed will be possible to
identify these potential slowdowns and provide special fast paths. But in order
to make this happen, some kind of advanced performance reporting may be useful:
http://lists.freedesktop.org/archives/pixman/2010-September/000608.html

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Don-t-discriminate-PAD-and-REFLECT-repeat-in-standar.patch
Type: text/x-patch
Size: 2457 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20100927/fd5e845c/attachment.bin>
-------------- 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/20100927/fd5e845c/attachment.pgp>