813124 - Cairo fills don't handle multiple clip paths

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Joe Drew (not getting mail)]

This is a clone of bug 803910 that focuses on the bad background drawing. That bug is about overinvalidation.

If you load the switches index HTML I have put up in the URL, it has a mostly-minimal testcase that reproduces on Mac OMTC when you scroll the circle into view.

This is the basecamp blocker based on bug 803910 comment 4.

Comment 1

[Joe Drew (not getting mail)]

Attachment: bad drawing example

I suspect this has to do with bad clipping, because the rounded background as shown here (dumped to a PNG file directly from the nsDisplayItem paint) is painted in a spot it's not supposed to (that is, on top of "Switch, commonly used in settings"). In this case, that rounded background shouldn't be painted at all - that part of the image is outside the bounds of where we're supposed to be drawing.

Comment 2

[Joe Drew (not getting mail)]

The content that's being drawn without clips here is a TRANSFORM DisplayItem assigned to the inactive layer manager.

Comment 3

[Andreas Gal :gal]

Not according to my gdb session. From what I can tell we are painting a single row of pixels that scrolled into view. That row intersects with the rounded edge clipped background image and we paint one row of it and recycle the clipping mask layer. However, we don't set a clipping region on the mask layer so we composite the whole mask with the partial to be clipped content, which explains why the whole checkbox flashes as a result of painting with h=1. I ran out of battery on the plane, and I won't have time to try to patch this before tomorrow. We should pull in roc and see whether this description makes sense. He might have a fix in mind.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds plausible, but I don't see anything in our BasicLayers mask compositing code (which I assume is what's being used here) that would cause it to draw outside the current clip on the context the BasicLayerManager draws into.

Comment 5

[Joe Drew (not getting mail)]

I don't see any mask layers being created here, only rounded rectangle clips. (In the common case, that is, when things are going right.)

Tomorrow morning I'll finaly be able to look in to what happens in the uncommon case when things go wrong.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Drawing a solid color Paint() right after we setup the rounded rect clipping shows us drawing outside of the aRegionToDraw, and even across the buffer rotation boundary (such that the color appears at the bottom of the screen).

For some reason the clip to aRegionToDraw is being ignored. I'm vaguely suspecting a bug somewhere in cairo/pixman but I haven't been able to find anything concrete yet.

I might try azure/skia content rendering to see if that fixes the issue, should help confirm what the problem is.

Comment 7

[Andreas Gal :gal]

Sorry for joining the party late. A bit jetlagged off the plane and have to run to the airport for another trip again.

comment 4: yes, BasicLayers via PaintWithMask. I agree the code looks correct. However, just for completeness, can you check FillWithMask for the aOpacity == 1.0 case? We do ->Clip() there. Is that right? Note: we are NOT using this path. We use PaintWithMask only in this case.

comment 6: this sounds promising. I think we are really close.

Comment 8

[Andreas Gal :gal]

As stated in the other bug this has nothing to do with OMTC, and even less so with paint ordering. Resetting title to latest bug description.

Comment 9

[Alex Keybl [:akeybl]]

Marking for C2, given this meets the criteria of known P1/P2 bugs at the end of C1.

Comment 10

[Joe Drew (not getting mail)]

Attachment: Before-and-after dumps from within the FrameLayerBuilder::DrawThebesLayer paint loop

This is a complete dump of all drawing from within DrawThebesLayer, before and after. The bug clearly happens on the draw of item 2, the TRANSFORM item. Unfortunately I have yet to actually isolate that draw in a debugger, because it somehow (?!?!) doesn't call PaintThebesLayer when I scroll. Since I'm using OMTC (and can't reproduce this bug without OMTC) I presume that the bad drawing actually happened just *before* this, and the pre-drawn layer is only presented at that time.

Comment 11

[Joe Drew (not getting mail)]

There is insanity in Cairo's image surface that tries to see if our clip is made up of a single path, and if so, to use fill instead. Naturally it is written in an insane way and doesn't work at all. I have no idea how this works in general.

Comment 12

[Joe Drew (not getting mail)]

Attachment: correctly detect having a single clip path

Neither Jeff nor I have any idea what this function was trying to do, but by the way it's used, this seems like the correct implementation.

Cairo 1.12 not only removes this code but rewrites all the surrounding code to apparently erase every trace of it.

Try: https://tbpl.mozilla.org/?tree=Try&rev=bfdda20faf58

Comment 14

[Andreas Gal :gal]

Was comment 10 captured on the device? Can you post the test case for that. Thanks.

Comment 15

[Andreas Gal :gal]

Excellent, comment 12/the attached patch fix the rendering issue on device, and fixes the overpainting. Thanks!

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It looks like the intent of the existing code was: if there's exactly one path that's not an axis-aligned rectangle, return it, otherwise return NULL. That could be useful, in theory. But it does look like _cairo_image_surface_paint and _cairo_image_surface_fill are buggy where they call _clip_get_single_path, just ignoring the rectangular paths in the clip.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 684186 [details] [diff] [review]
correctly detect having a single clip path

Review of attachment 684186 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with the approach of fixing the fast path to only be used with a single path. However, I have not reviewed the implications of this patching the behaviour of this function.

Comment 18

[Andreas Gal :gal]

Whats the next step here?

Comment 19

[Joe Drew (not getting mail)]

The current patch broke a reftest, and I'm investigating that.

Comment 20

[Joe Drew (not getting mail)]

Attachment: broken reftest

The reftest my patch breaks is this one. It creates a path as follows:

 * a counterclockwise loop 10 px outside around the canvas; then
 * a clockwise loop directly around the canvas.

It then clips the canvas to this path, and tries filling with a red rectangle. Before my patch, this worked correctly (i.e., the red was clipped out according to the winding rule); now it doesn't.

It looks like one of _cairo_clip_to_boxes or _clip_and_composite_boxes isn't handling this strange clip correctly.

Comment 21

[Joe Drew (not getting mail)]

Ah, cairo_clip_to_boxes doesn't handle us because we're not rectilinear, so it just falls back and gives us the extents.

Comment 22

[Joe Drew (not getting mail)]

Ugh ugh ugh. We can't even use _cairo_clip_get_region because this clip path has the REGION_IS_UNSUPPORTED flag set.

At this point I don't know if Cairo 1.10 can support this clip path. OTOH it looks like the complete reworking in 1.12 will make this work.

Roc, what do you think of me disabling this test (which works on all other browsers, fwiw) and just moving forward with this patch? If we upgrade Cairo or switch to Skia or any of a number of other options, we'll magically stop breaking this.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Cairo 1.10 should be able to support any combination of clip paths. It certainly used to.

It seems to me that _clip_and_composite_boxes should handle these paths. _clip_and_composite should set need_clip_surface to true and then call _clip_and_composite_with_mask, calling _create_composite_mask_pattern, calling _cairo_clip_combine_with_surface. Or something like that.

Comment 24

[Joe Drew (not getting mail)]

Oh, I think I was just misunderstanding what the boxes sent to _clip_and_composite_boxes were for. I thought they were used instead of the clip, but that seems not to be the case - it's the boxes of content to be rendered.

Comment 25

[Joe Drew (not getting mail)]

Attachment: correctly detect having a single clip path and handle being all clipped

I copied some code from _clip_and_composite to _composite_boxes that handles _cairo_clip_get_region detecting that we're completely clipped and giving up. I guess this never came up before.

Roc, flagging you for review because Jeff's out for a few days.

Comment 26

[Joe Drew (not getting mail)]

Attachment: correctly detect having a single clip path and handle being all clipped

foiled by qref

Going through try: https://tbpl.mozilla.org/?tree=Try&rev=59a3e0a75434

Comment 27

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8209de5489e

What approval do I need to push this to aurora & beta?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

None for now since it's bb+. Which means that if you do nothing, I'll get it after it hits m-c as one of my usually daily uplifts.

Comment 29

[Andreas Gal :gal]

Nice patch. Thanks for the quick patching/review.

Comment 30

[Joe Drew (not getting mail)]

(In reply to Ryan VanderMeulen from comment #28)
> None for now since it's bb+. Which means that if you do nothing, I'll get it
> after it hits m-c as one of my usually daily uplifts.

I have no idea how we lived without you before.

Comment 31

[Joe Drew (not getting mail)]

Attachment: convert content/canvas/test/reftest/reftest.list to unix line endings

Comment 32

[Joe Drew (not getting mail)]

Attachment: make sure that multiple clips are handled on a canvas

This is a test of this exact bug, using canvas. Since this bug only exists in the Cairo image surface, as we use Cairo it could only ever have failed on Android and B2G, but at least it makes sure we'll never fail it again.

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 684761 [details] [diff] [review]
convert content/canvas/test/reftest/reftest.list to unix line endings

Review of attachment 684761 [details] [diff] [review]:
-----------------------------------------------------------------

I've been landing such line-endings-changes without review.

From a quick glance it does seem to only change line endings.

Comment 34

[Joe Drew (not getting mail)]

Testing the test on try: https://tbpl.mozilla.org/?tree=Try&rev=93b389c31738

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8209de5489e

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e0a8f234a6d
https://hg.mozilla.org/releases/mozilla-beta/rev/f5b325a7d5c6

Comment 37

[Joe Drew (not getting mail)]

Try results for the test look good. Once Jeff reviews it I'll push it to central.

Comment 38

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b43311784b7f

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b43311784b7f