[cairo] [PATCH] pixman: fast path for nearest neighbour scaled compositing operations.

[Jeff Muizelaar]

On Fri, May 29, 2009 at 11:00:31PM +0300, Siarhei Siamashka wrote:
> On Thursday 28 May 2009 21:28:31 ext Jeff Muizelaar wrote:
> > I think these fuctions would be easier to understand if the variables
> > were renamed:
> > vv0 -> vx;
> > vv1 -> vy;
> > uv0 -> unit_x;
> > uv1 -> unit_y;
> >
> > I think x and y a are clearer then 0 and 1
> 
> done

Great.

> The other variables are consistent with the rest of file (the same naming 
> conventions exist in other functions). They are mostly temporary variables
> which are really hard to misunderstand. If you have some specific suggestions
> about how to better name these variables, it can be done easily.

I can't think of any right now.

> > > +            if (a1 == 0xff)
> > > +                WRITE(pDst, dst, cvt8888to0565(s1));
> > > +            else if (s1) {
> > > +                d = cvt0565to0888(READ(pDst, dst));
> > > +                a1 ^= 0xff;
> >
> > things like this could also maybe use a comment or a macro:
> > e.g. //equivalent to 255 - a1
> > in fact is a1 ^= 0xff actually faster than a1 = 255 - a1?
> 
> It's just a single instruction on most (or even all?) cpu architectures, while
> for 'a1 = 255 - a1' it might not be true (though ARM has RSB instruction).

How about a comment then? Or perhaps have a little preface section to
this set of code that explains some of the conventions and tricks.
That will avoid having to have a comment each usage.

[snip]

> Overall, I don't quite like code duplication (many functions sharing the same
> code), but alternatives may look a bit more obfuscated. Alternatives could use
> either C preprocessor or perl script for emitting scaler functions from a
> common template. In any case, the most important thing is that the code has
> an extensive correctless test, so it is easy to transform code to any other
> representation without too much risk of breaking it.

I agree. It's definitely a tough balance between duplication and one's
ability to quickly understand a single function. However, since all of
the functions are farely small I think it's easier to justify the
duplication. I also whole heartedly agree that having the test code
makes the maintenance burden of the duplication much more manageable.
Thanks for that!

> +/*
> + * Check if we actually have source clipping. Just comparing pointers
> + * does not work, so full comparison of regions is needed. Source
> + * clipping is enforced in X server since the fix of
> + * http://bugs.freedesktop.org/show_bug.cgi?id=11620

Why isn't comparing pointers sufficent for checking source clipping?

pixman_image_set_source_clipping() does
	common->src_clip = &common->full_region;
when source_clipping is false.

-Jeff