[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