Bug 84848 – [cairo] CairoGraphicsContext fillRect (with Color) overrides composite operator

• Component: Platform
• Status: RESOLVED FIXED

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

...or anyone familiar with Cairo graphics context.

[Martin Robinson]

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 ***

[Martin Robinson]

Sorry, I swapped the block here.

[Martin Robinson]

*** Bug 67404 has been marked as a duplicate of this bug. ***

[Dominik Röttsches (drott)]

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?

[Martin Robinson]

I think it's important to look at each new failure to ensure that this doesn't cause any unexpected regressions.

[Martin Robinson]

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.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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.

[Martin Robinson]

Comment on attachment 138973 [details]
Removing composite operator override, fixing baselines

Great! Thanks.

[WebKit Review Bot]

Comment on attachment 138973 [details]
Removing composite operator override, fixing baselines

Clearing flags on attachment: 138973

Committed r115321: <http://trac.webkit.org/changeset/115321>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

*** Bug 83580 has been marked as a duplicate of this bug. ***