Bug 84848 - [cairo] CairoGraphicsContext fillRect (with Color) overrides composite operator
Summary: [cairo] CairoGraphicsContext fillRect (with Color) overrides composite operator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominik Röttsches (drott)
URL:
Keywords:
: 67404 83580 (view as bug list)
Depends on:
Blocks: 83580
  Show dependency treegraph
 
Reported: 2012-04-25 04:51 PDT by Dominik Röttsches (drott)
Modified: 2012-04-26 09:59 PDT (History)
7 users (show)

See Also:


Attachments
Removing composite operator override (1.59 KB, patch)
2012-04-25 08:27 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Removing composite operator override (8.08 KB, patch)
2012-04-26 01:49 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Removing composite operator override, fixing baselines (172.09 KB, patch)
2012-04-26 04:15 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-04-25 04:51:18 PDT
Cairo's
void GraphicsContext::fillRect(const FloatRect& rect, const Color& color, ColorSpace)
calls the 
fillRectWithColor(platformContext()->cr(), rect, color);
helper which overrides the composite operator.

This makes for example
svg/filters/feDropShadow.svg
fail, since computing the dropShadow combines the CompositeSourceIn operator with fillRect.
Comment 1 Dominik Röttsches (drott) 2012-04-25 08:27:48 PDT
Created attachment 138816 [details]
Removing composite operator override

For "historic" reasons there's a composite operator override in this function which IMHO is very wrong. It's the root cause for breaking svg/filters/feDropShadow.svg. Instead of the green drop shadow, a green rectangle gets painted due to the operator override.

Fixing this one might be a bit tricky because of the rebaselining consequences. If this gets landed, I think a number of cases need rebaselining. On EFL, I got about 400 new failures with this one - I am now running the GTK tests.
So, first I would like to get an opinion from Martin Robinson.
Comment 2 Dominik Röttsches (drott) 2012-04-25 08:28:09 PDT
...or anyone familiar with Cairo graphics context.
Comment 3 Martin Robinson 2012-04-25 08:32:37 PDT
It would be nice to include rebaselines with this patch. Please include them for EFL and I'll fix GTK+ when this is landed.

*** This bug has been marked as a duplicate of bug 67404 ***
Comment 4 Martin Robinson 2012-04-25 08:37:16 PDT
Sorry, I swapped the block here.
Comment 5 Martin Robinson 2012-04-25 08:37:48 PDT
*** Bug 67404 has been marked as a duplicate of this bug. ***
Comment 6 Dominik Röttsches (drott) 2012-04-25 08:42:06 PDT
Martin, thanks for the quick feedback - how would you handle the rebaselines in this case? Would you blindly rebaselines or actually go and inspect the new failures one by one?
Comment 7 Martin Robinson 2012-04-25 08:44:58 PDT
I think it's important to look at each new failure to ensure that this doesn't cause any unexpected regressions.
Comment 8 Martin Robinson 2012-04-25 09:20:41 PDT
The patch on the duplicated bug seems to be a bit more correct, I think. You should try re-running the tests with those changes, if they look correct to you as well.
Comment 9 Dominik Röttsches (drott) 2012-04-25 09:49:30 PDT
I actually tried similar changes as in the other the patch before uploading (i.e. switching the composite operator before the remaining uses of fillRectWithColor. I came to the preliminary conclusion that keeping the composite operator overrides for drawing "corner line endings" and in drawRect might keep the numbers for rebaselining low but keeps the error alive, and is actually less correct.

For example, why would drawLineOnCairoContext need to switch to a different composition operator? If we wouldn't do the intentional "corner stamping"/fillRect corners there, there would only be cairo_stroke, and no change of composite operator. 
In drawRect, I think the side effect of switching the composite operator is actually worse, since the rectangle stroke is always drawn with CompositeOver operator after fillRectWithColor. I don't think that's intended for drawRect.

FWIW, I compared CG and Qt graphics contexts - they don't do any changing of the composite operator in these cases.
Comment 10 Dominik Röttsches (drott) 2012-04-26 01:49:25 PDT
Created attachment 138953 [details]
Removing composite operator override

For an odd reason, today I don't get a long list of new failures today. - There was an issue with disk space on my machine yesterday - this might have interfered.
Comment 11 Dominik Röttsches (drott) 2012-04-26 04:15:22 PDT
Created attachment 138973 [details]
Removing composite operator override, fixing baselines

I doublechecked and the failure rates in fact do not seem to shoot up with this change, sorry for the confusion. Merged the new baselines for drop shadow into this patch. I am confident the fix is correct and ready for landing now.
Comment 12 Martin Robinson 2012-04-26 08:22:38 PDT
Comment on attachment 138973 [details]
Removing composite operator override, fixing baselines

Great! Thanks.
Comment 13 WebKit Review Bot 2012-04-26 09:12:59 PDT
Comment on attachment 138973 [details]
Removing composite operator override, fixing baselines

Clearing flags on attachment: 138973

Committed r115321: <http://trac.webkit.org/changeset/115321>
Comment 14 WebKit Review Bot 2012-04-26 09:13:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Dominik Röttsches (drott) 2012-04-26 09:59:07 PDT
*** Bug 83580 has been marked as a duplicate of this bug. ***