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

[Soeren Sandmann]

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.

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.

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. 

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. But
once the immediate symptom is solved, people inevitably end up not
fixing the real problem.

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.


Soren