[cairo] [PATCH] pixman: fast path for nearest neighbour scaled compositing operations.
Jeff Muizelaar
jeff at infidigm.net
Fri May 29 14:03:33 PDT 2009
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
More information about the cairo
mailing list