[cairo] Operator optimization based on source or dest opacity
Soeren Sandmann
sandmann at daimi.au.dk
Fri Feb 29 10:54:27 PST 2008
Hi Antoine,
> I've reworked my patches according to the comments from Soeren.
>
> Here are 3 patches attaches to this message
> Patch1: Move all composition operators to their own c/h files
This one is getting pretty close. Comments:
- pixman_image_optimize_op() is a leftover in the header file.
- The whole "FIXME" and #define INLINE inline should just go away. If
inline is accepted by all compilers we care about, then let's just
use that. An unconditional
#define INLINE inline
is certainly pointless.
None of this was introduced by your patch, but since we are moving
it around anyway, we may as well get rid of it.
- The tables pixman_fbCombineFuncC and pixman_fbCombineFuncU
should be made non-static and moved into the C file, and only
declared in the header file.
- With the tables exported like that, is it necessary to declare all
the functions in the header file? It looks to me like they could all
just be static functions within the C file.
If those declarations can be removed, can we get rid of the header
file altogether and simply declare the tables in pixman-private.h?
> Patch2: Optimize operators depending on source/dest opacity
Comments:
- The table "OptimizedOperatorInfo", and the inline functions in
pixman-compose-operators should just be moved to either
pixman-image.c or pixman-pict.c.
In general, please avoid tables and code in header files.
- It's probably not worth inlining the functions. The compiler can do
so if it thinks it's going to matter.
- I really don't think it's necessary to have a separate
pixman_transformed_image_is_opaque(). For destination images, the
transform and filter are always going to be NULL, so the additional
check is very quick.
- In pixman_image_is_opaque(),
just return FALSE instead of setting isSourceOpaque to FALSE.
- The if statement in pixman_transformed_image_is_opaque() is
difficult to follow for me. It's also broken in at least one case:
convolution filters can introduce translucency even when there
weren't any to begin with.
Also, I'm not confident that the pi/2 rotation transformation check
is correct. Doesn't it need the rotation to be around the center of
the image?
Can we just add something like this to the end of
pixman_image_is_opaque():
/* Convolution filters can always introduce translucency */
if (convolution filter)
return FALSE;
if (repeat_none && (filter || transformation))
return FALSE;
return TRUE;
then get rid of pixman_transformed_image_is_opaque()?
I *think* that would be correct, though I would appreciate an
additional review from someone else.
> Patch3: Optimize a bit the checks to select a fast code path
Let's wait with this one until the others are ready to land.
Regards,
Soren
More information about the cairo
mailing list