Bug 5777

Summary:	fbcompose.c broken for some composite operations post r1.5
Product:	cairo	Reporter:	tor
Component:	general	Assignee:	Carl Worth <cworth>
Status:	RESOLVED FIXED	QA Contact:	cairo-bugs mailing list <cairo-bugs>
Severity:	blocker
Priority:	high	CC:	gavin.sharp, jwatt, reveman, vladimir
Version:	1.1.1
Hardware:	x86 (IA32)
OS:	Windows (All)
Whiteboard:
i915 platform:		i915 features:
Attachments:	testcase possible bandaid fix Fix

Description tor 2006-02-01 08:00:30 UTC

Revision 1.6 of fbcompose.c added an optimization to fbCompositeRect where the
src pixels would only be fetched once if this was possible (solid fills and the
like).    However this doesn't work for some operations which overwrite the src
buffer, for example fbCombineAddC (via fbCombineMaskValueC).  Such operations
need to be checked for when deciding when the src fetch-once optimization can occur.

Comment 1 David Reveman 2006-02-02 10:29:31 UTC

I'm pretty sure that all my patches have not landed in the cairo tree yet? If I
remeber correctly the second iteration which contained mostly performance
improvments also had some minor fixes that could correct this.

Is there a test program that expose this? rendercheck doesn't complain about the
same code in the server (that's code with all my patches applied though).

Comment 2 tor 2006-02-02 10:42:38 UTC

I don't have a test program - doing text with a clip path set on win32 triggered
this composite operation.

Which server are you talking about?  xc head fbcompose.c doesn't have this
optimization, and the xserver one looks to have the same problem at first glance.

Comment 3 tor 2006-02-03 04:55:44 UTC

Created attachment 4546 [details]
testcase

Here's a cairo test that shows the problem.

You'll need to disable the mmx versions of the composite functions (hack
fbHaveMMX), as the mmx implementation of CombineAddC doesn't have this problem.

Comment 4 David Reveman 2006-02-03 12:07:59 UTC

Thanks for the test case, I'll look at this asap. Not being exposed when using
the mmx code explaines why I didn't notice it.

Comment 5 tor 2006-02-08 11:20:37 UTC

Created attachment 4577 [details] [review]
possible bandaid fix

Comment 6 Jonathan Watt 2006-09-21 10:25:41 UTC

David, have you managed to make any progress on a fix for this bug?

Comment 7 David Reveman 2007-04-03 13:44:06 UTC

Created attachment 9469 [details] [review]
Fix

The previous fix is fine too but here's another one which I think is a bit more appropriate. It changes so fbCombineMaskC, fbCombineMaskValueC and fbCombineMaskAlphaC operate on pixels instead of scanlines and can be used without modifying src and mask scanlines.

Comment 8 Owen Taylor 2007-04-08 16:36:41 UTC

What's the effect on performance of the patch? Function-call-per-pixel might 
be noticeable and I don't think gcc will inline at -O2 unless you add 'inline'

Comment 9 David Reveman 2007-04-09 07:44:21 UTC

I don't know if the current patch got any effect on performance. We should add 'inline' to make sure gcc will inline those functions.

Comment 10 Behdad Esfahbod 2007-04-12 11:45:32 UTC

commit d3c7942fb271fe8d1df7ca3205b41601abdcb5c8
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Thu Apr 12 14:22:02 2007 -0400

    [pixman] Mark some small functions that are called per-pixel inline
    
    These uses were introduced in the previous commit.

commit e3b3d22999a130f7017e8e20a432a0d8a7f48f3b
Author: David Reveman <davidr@novell.com>
Date:   Thu Apr 12 14:14:12 2007 -0400

    [pixman] Fix fbcompose.c that was broken for some composite operations (#5777)

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.