[cairo] Operator optimization based on source or dest opacity

Antoine Azar cairo at antoineazar.com
Wed Feb 27 13:37:16 PST 2008


Hi Soeren,
thanks for the good code review.

>- pixman_image_is_opaque() needs to handle alpha-maps.

I'm not sure I understand what you mean. How do you check if an alpha 
map is opaque without checking every pixel of it?


>- In general, it looks to me like the code in pixman-pict.c that 
>checks whether the optimization is applicable, is really checking 
>whether the image is opaque. If so, it should be moved into 
>pixman_image_is_opaque().

Yes and no. I originally didn't include the other checks in the 
function because I wanted to keep it generic enough for other uses. 
It basically just checks if a source contains alpha information or 
not which I guess could be useful at other places. The other checks 
take into account *how* the source is being used and if this 
introduces any transparency. So I can see a valid point either way, 
let me know if you would still rather have the checks included in 
pixman_image_is_opaque.


>- The remaining code in pixman-pict.c that deals with the 
>optimization should be moved to its own function, like this:
>
>         op = optimize_operator (op, src, mask, dst);
>
>   The pixman_optimize_operator() from your patch can then be 
> integrated into this.

Will do.


>- Coding style:
>
>+    case BITS:
>+        {
>+            if(PIXMAN_FORMAT_A(image->bits.format))
>+                isSourceOpaque = FALSE;
>+            break;
>+        }
>
>    I'm not very draconian about coding style, but please eliminate 
> those braces. Also, when such braces are necessary, please align 
> them with the 'case' keyword.

Done.

>- I'd like to see the unrelated changes to the fast path search 
>dropped. We can look at them later if they do make a performance difference.

I'll separate them to illustrate the strange performance differences 
in that section of the code. I'll send back 2 patches.


>I like this; in fact, I'd like to see even the inlined functions 
>moved into the C file instead of the header file.

Will do.

Thanks,
Antoine 



More information about the cairo mailing list