Closed Bug 522859 Opened 12 years ago Closed 11 years ago

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


(Core :: Graphics, defect)

Not set





(Reporter: roc, Assigned: roc)


(Whiteboard: [needs review][approved-patches-landed][not-ready])


(4 files, 3 obsolete files)

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.
Er, that is a testcase which fails.
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.
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.
Assignee: nobody → roc
Attachment #406971 - Flags: review?(jmuizelaar)
Attached patch Part 2: fix (obsolete) — Splinter Review
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.
Attachment #406972 - Flags: review?(jmuizelaar)
Whiteboard: [needs review]
These patches have been waiting for review for nearly five months.
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?
Probably not. But what do you mean "do these as part of the cairo update"?
Review these patches, merge them, and push them upstream.
Jeff, can we get these in now?
Attached patch Part 1, v2 (obsolete) — Splinter Review
Updated to trunk.
Attachment #444589 - Flags: review?(jmuizelaar)
Attachment #406971 - Attachment is obsolete: true
Attachment #406971 - Flags: review?(jmuizelaar)
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.
The first patch doesn't seem to apply to trunk.
Attached patch Part 1, v3Splinter Review
This should apply.
Attachment #444589 - Attachment is obsolete: true
Attachment #445007 - Flags: review?(jmuizelaar)
Attachment #444589 - Flags: review?(jmuizelaar)
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;
>+        }


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


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

Attachment #445007 - Flags: review?(jmuizelaar) → review+
This patch could be tricky to keep up-to-date so I'm going to try to get it upstream as soon as possible.
Whiteboard: [needs review] → [needs review][needs landing]
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.
Checked in part 1, but backed it out due to reftest failures on Mac:
Whiteboard: [needs review][needs landing] → [needs review]
Fixed reftest failures and relanded:
(A superflous CGContextSaveState when setting up a DO_PATTERN was breaking clipping.)

Followup to make the patch file include the reftest fix:
Attached patch Part 2 updatedSplinter Review
Considerable updating to trunk here.

I want to get this fixed because people keep getting confused by our incorrect/inconsistent behavior of canvas operators.
Attachment #406972 - Attachment is obsolete: true
Attachment #458885 - Flags: review?(jmuizelaar)
Attachment #406972 - Flags: review?(jmuizelaar)
Attached patch Part 3: cleanupSplinter Review
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.
Attachment #458886 - Flags: review?(jmuizelaar)
Comment on attachment 458885 [details] [diff] [review]
Part 2 updated

Looks like nice to me.
Attachment #458885 - Flags: review?(jmuizelaar) → review+
Whiteboard: [needs review] → [needs review][needs landing]
(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.
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 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.
Attachment #458885 - Flags: approval2.0?
Whiteboard: [needs review][needs landing] → [needs review][needs approval]
Attachment #458885 - Flags: approval2.0? → approval2.0+
Whiteboard: [needs review][needs approval] → [needs review][needs landing]
Part 2 landed:

This bug is basically fixed, but I'd still like to get part 3 landed.
Whiteboard: [needs review][needs landing] → [needs review]
Flags: in-testsuite+
Part 3 has been waiting for review for two months now.
Whiteboard: [needs review] → [needs review][approved-patches-landed]
Whiteboard: [needs review][approved-patches-landed] → [needs review][approved-patches-landed][not-ready]
Marking fixed because it is.
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 458886 [details] [diff] [review]
Part 3: cleanup

We mostly use Azure now.
Attachment #458886 - Flags: review?(jmuizelaar) → review-
You need to log in before you can comment on or make changes to this bug.