522859 - Quartz backend handling of unbounded operators doesn't handle bounded-size sources

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

The attached testcase should end with the canvas entirely clear. It doesn't, on Mac. The reason is that _cairo_quartz_fixup_unbounded_operation is set up to handle bounded masks, but not bounded sources. In particular when we use CGContextDrawImage to draw a (non-tiled) image that doesn't completely cover the mask, the area that's outside the image and inside the mask has not been composited to by Quartz, and _cairo_quartz_fixup_unbounded_operation doesn't fix it up either.

Comment 1

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

Attachment: testcase which works

Comment 2

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

Er, that is a testcase which fails.

Comment 3

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

I think we should just reimplement  _cairo_quartz_fixup_unbounded_operation so that instead of trying to apply the operator "outside" the area that we already composited into, which is fairly complicated and fragile (and I suspect has seam bugs where the edges of the area aren't pixel-aligned), we just render into a temporary surface which is the size of the clipping area and then composite that into the target with the right operator.

Comment 4

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

Attachment: Part 1: refactor _cairo_quartz_setup_source

This patch doesn't change any behaviour. The per-drawing-operation state is pulled out of cairo_quartz_surface_t and moved into a new struct cairo_quartz_drawing_state_t, because I want to add new state when I fix this bug and having per-drawing-operation state in the surface is icky.

The action is also stored in cairo_quartz_drawing_state_t, and I rename _cairo_quartz_setup_source to _cairo_quartz_setup_state, because we're going to add more then just source-related state.

I move responsibility for saving and restoring the context into _cairo_quartz_setup_state/_cairo_quartz_teardown_state, and also responsibility for calling CGContextSetCompositeOperation. This adds an unnecessary Save/RestoreGState pair to _cairo_quartz_surface_paint with a solid color or pattern (cases which we never use, I think), but removes some unnecessary duplicate Save/Restore pairs, e.g. when filling or stroking with a surface pattern. I think it's a net code simplification without affecting performance. We could easily suppress the save/restore in more cases if we cared.

Comment 5

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

Attachment: Part 2: fix

Moves handling of unbounded operators into _cairo_quartz_setup_state/teardown_state by creating a temporary surface the size of the current clipbox. Also reworks _cairo_quartz_surface_mask_with_surface to use _cairo_quartz_setup_state/teardown_state so it can take advantage of this machinery.

Also fixes the empty path test in _cairo_quartz_surface_fill so we don't bail out early if the operator is unbounded, because that's incorrect.

This simplifies the code a lot as well as fixing the bug.

Comment 6

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

These patches have been waiting for review for nearly five months.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Sorry about this, my plan has always been to do these as part of the cairo update, which is unfortunately taking much longer than I had planed. Should these be higher priority?

Comment 8

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

Probably not. But what do you mean "do these as part of the cairo update"?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Review these patches, merge them, and push them upstream.

Comment 10

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

Jeff, can we get these in now?

Comment 11

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

Attachment: Part 1, v2

Updated to trunk.

Comment 12

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

I haven't updated the second patch yet, since I don't need it right now. Let me know if/when you need it refreshed.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

The first patch doesn't seem to apply to trunk.

Comment 14

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

Attachment: Part 1, v3

This should apply.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 445007 [details] [diff] [review]
Part 1, v3

I really like the idea of moving this stuff out of the surface.

>-
>-    return DO_IMAGE;
>+    if (status) {
>+        state->action = DO_UNSUPPORTED;
>+	return;

A little whitespace error here.

>+    }
>+    if (img == NULL) {
>+        state->action = DO_NOTHING;
>+	return;
>+    }

and here.

>-	    return DO_UNSUPPORTED;
>-	if (img == NULL)
>-	    return DO_NOTHING;
>+        if (status) {
>+            state.action = DO_UNSUPPORTED;
>+	    return state;
>+        }
>+        if (img == NULL) {
>+            state.action = DO_NOTHING;
>+	    return state;
>+        }

Whitespace.

>-	    surface->sourceImageRect = srcRect;
>-
>-	    return DO_TILED_IMAGE;
>+	    state.imageRect = srcRect;
>+            state.action = DO_TILED_IMAGE;
>+            return state;
> 	}

Whitespace.


>-	CGContextSetPatternPhase (surface->cgContext, CGSizeMake(0,0));
>-
>-	surface->sourcePattern = pattern;
>-
>-	return DO_PATTERN;
>+	CGContextSetPatternPhase (context, CGSizeMake(0,0));
>+
>+	state.pattern = pattern;
>+        state.action = DO_PATTERN;
>+        return state;

Whitespace.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

This patch could be tricky to keep up-to-date so I'm going to try to get it upstream as soon as possible.

Comment 17

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

Those whitespace errors aren't real, they're because some lines have tabs and some have spaces, and the diff has one-char indent everywhere so the tabs look less wide than they are.

Comment 18

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

Checked in part 1, but backed it out due to reftest failures on Mac:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274240648.1274247824.24616.gz

Comment 19

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

Fixed reftest failures and relanded:
http://hg.mozilla.org/mozilla-central/rev/da64615a2fac
(A superflous CGContextSaveState when setting up a DO_PATTERN was breaking clipping.)

Followup to make the patch file include the reftest fix:
http://hg.mozilla.org/mozilla-central/rev/7066468619b5

Comment 20

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

Attachment: Part 2 updated

Considerable updating to trunk here.

I want to get this fixed because people keep getting confused by our incorrect/inconsistent behavior of canvas operators.

Comment 21

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

Attachment: Part 3: cleanup

Additional cleanup, primarily moving IS_EMPTY(surface) optimization and _cairo_surface_clipper_set_clip call into _cairo_quartz_setup_state, reducing code duplication.

It also happens to fix a bug; _cairo_quartz_surface_mask_with_surface was ignoring its clip.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 458885 [details] [diff] [review]
Part 2 updated

Looks like nice to me.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #20)
> Created attachment 458885 [details] [diff] [review]
> Part 2 updated
> 
> Considerable updating to trunk here.
> 
> I want to get this fixed because people keep getting confused by our
> incorrect/inconsistent behavior of canvas operators.

Have you done any perf testing of this? It seems like the different approach to unbounded could change things.

Comment 24

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

I haven't. However, the only unbounded operators in cairo are CAIRO_OPERATOR_OUT, CAIRO_OPERATOR_IN, CAIRO_OPERATOR_DEST_IN, and CAIRO_OPERATOR_DEST_ATOP, which are only used if someone specifically requests them through <canvas> (OUT and IN can be requested with SVG <feComposite> also). So I'm not expecting much performance impact in the wild.

Comment 25

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

Comment on attachment 458885 [details] [diff] [review]
Part 2 updated

Makes us compliant with the canvas spec as it currently is. Makes the Quartz backend consistent with other cairo backends.

Comment 26

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

Part 2 landed:
http://hg.mozilla.org/mozilla-central/rev/19c6914ca8cd

This bug is basically fixed, but I'd still like to get part 3 landed.

Comment 27

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

Part 3 has been waiting for review for two months now.

Comment 28

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

Marking fixed because it is.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 458886 [details] [diff] [review]
Part 3: cleanup

We mostly use Azure now.