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

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 3 obsolete attachments)

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.
Created attachment 406971 [details] [diff] [review]
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.
Assignee: nobody → roc
Attachment #406971 - Flags: review?(jmuizelaar)
Created attachment 406972 [details] [diff] [review]
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.
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?
Created attachment 444589 [details] [diff] [review]
Part 1, v2

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.
Created attachment 445007 [details] [diff] [review]
Part 1, v3

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

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.
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:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274240648.1274247824.24616.gz
Whiteboard: [needs review][needs landing] → [needs review]
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
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.
Attachment #406972 - Attachment is obsolete: true
Attachment #458885 - Flags: review?(jmuizelaar)
Attachment #406972 - Flags: review?(jmuizelaar)
Created attachment 458886 [details] [diff] [review]
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.
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:
http://hg.mozilla.org/mozilla-central/rev/19c6914ca8cd

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.
Status: NEW → RESOLVED
Last Resolved: 7 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.