366283 - globalCompositeOperation "copy" incorrect

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P4)

Description

[Takeshi Kurosawa :takenspc]

In the trunk builds globalCompositeOperation = copy doesn't render correctly.
According to the spec, this should render the source image instead of the destination image.

But currently this renders the source image top of the destination image.

Comment 1

[Takeshi Kurosawa :takenspc]

Attachment: Testcase

A test case.

Comment 2

[Takeshi Kurosawa :takenspc]

Attachment: Patch rv.1.0

clear the canvas before draw paths, rects, or images.

Comment 3

[Philip Taylor]

I have a collection of tests at http://canvex.lazyilluminati.com/tests/tests/index.2d.composite.uncovered.html - 'copy' is the only one that is broken for fill, but all the other modes are broken for drawImage. (This is with Gecko/20070328, not with the above patch). Opera 9.1 does these all correctly [except its atop blending is wrong], Safari 2.0.4 does none correctly. (I agree with Opera's interpretation of the spec here.)

Comment 4

[Philip Taylor]

More specific tests at http://philip.html5.org/tests/canvas/misc/uncovered-compositing.html

This problem is more general than in comment 0 (so the patch doesn't fix it properly, but I guess it's best to stay inside this bug):

When a source image is painted onto a destination image, the source image is theoretically part of an infinite transparent-black layer. Under some globalCompositeOperations (copy, source-in, source-out, etc), compositing transparent black onto the destination results in transparent black (assuming globalAlpha=1). Therefore, it's not adequate to only consider the source area - the transparent black surroundings matter too.

FF trunk is incorrect for drawImage, since it clips to the image rectangle. It's incorrect for 'copy' fills, since it effectively clips to the source shape. It's incorrect in all modes when the source shape is outside the clip region, since it doesn't draw anything at all.

Comment 5

[Stuart Parmenter]

vlad, take a look at this please and decide blocking

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So I'm inclined to not fix this, but need to figure out the correct wording.  Both us and Safari implement copy in the same way; that is, that it's implicitly bounded by the mask/shape.  I think that intuitively, this is more useful; otherwise, to get the same result, you'd have to first clear the region and use OVER, or set a clip and use SOURCE, both of which would be more expensive.  But this needs to be brought up in whatwg to come to an agreement on what should be done here.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 250815 [details] [diff] [review]
Patch rv.1.0

Clearing review; still needs to be discussed on whatwg.

Comment 8

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

(In reply to comment #6)
> So I'm inclined to not fix this, but need to figure out the correct wording. 
> Both us and Safari implement copy in the same way; that is, that it's
> implicitly bounded by the mask/shape.

Then the spec would have to define the extents of all shapes, which seems like unnecessary complexity in the spec just for this one case.

Comment 9

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

It seems to me that the problem here is not in our canvas implementation but in cairo itself. NeedToUseIntermediateSurface should not be needed, the cairo backends should be implementing OPERATOR_SOURCE as an unbounded operator, but don't, presumably because of some optimization that's not correct for situations where the source image extents don't cover the destination. Fixing it in cairo would also be preferable because cairo backends already have access to the image extents.

Comment 10

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

OK never mind, OPERATOR_SOURCE and OPERATOR_CLEAR are explicitly bounded by the mask in cairo.
http://cairographics.org/operators/

Comment 11

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

So basically there are two bugs here:
1) cairo-quartz doesn't handle unbounded operators correctly when drawing images. This is simply a cairo bug that we should fix.
2) we implement the canvas "copy" operator using cairo's bounded OPERATOR_SOURCE but the canvas spec is pretty clear that no operators are bounded.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#drawing-model

Arguably the canvas spec should be changed so that all operators are bounded. That may not be very hard --- ignore my comment #8. But our current behaviour, where SOURCE is bounded but none of the others are, definitely doesn't make sense IMHO. It's just an artifact of the way cairo works.

Comment 12

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

Filed bug 522859 for the issue of cairo-quartz not doing unbounded operators correctly when drawing non-tiled images.

Comment 13

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

Attachment: fix

Two related changes here:

-- NeedToDrawShadow just follows the spec text at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#when-shadows-are-drawn and does not consider the operator. This revealed that a couple of the tests in test_canvas.html are bogus, since they rely on shadows being drawn when the spec says not to. This is probably because the spec text changed.

-- The only other thing that cares about the boundedness of the operator is NeedToUseIntermediateSurface. There we only need to use a temporary surface for operators that cairo treats as bounded and where the bounded version gives different results to unbounded (the canvas spec requires all operators to be unbounded). These two operators are SOURCE and CLEAR (although canvas doesn't currently support the equivalent of CLEAR).

We could optimize here to avoid the temporary surface in some common cases using "copy". For example, in DrawImage() we could check whether the image covers the entire clip extents, and if it does, we don't need the temporary surface because the entire destination is contained in the mask. This would catch the case where someone uses "copy" to clone a canvas. Is this worth doing, though?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 406983 [details] [diff] [review]
fix

It looks like this still uses a temporary surface. I think we should be able to implement unbounded semantics by just discarding the mask.

Comment 15

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

Attachment: Part 1: fix shadow enabling

Comment 16

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

Attachment: Part 2: make 'copy' unbounded

We can't just ignore the mask. We need to use it to render the source. However, we can get the right effect without a temporary surface, by clearing the entire destination using Paint() before we render the source into it. I can't think of a more efficient way to do this other than adding an unbounded-SOURCE operator to cairo.

Comment 17

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

Attachment: Part 3: fix tests

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Did this get fixed in Safari/WebKit?  I'm hesitant to change our implementation here; copy is pretty frequently used as an optimization, and the unbounded behaviour is usually not expected.

Comment 19

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

No, it's still incorrect in Safari. However, Safari makes *all* the operators bounded (because it's just using Quartz). Opera follows the spec. How about we fix it on trunk and see what happens?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 417245 [details] [diff] [review]
Part 2: make 'copy' unbounded

Use: mThebes->Paint()

instead of: mThebes->Paint(1.0);

Other than that the patch itself is fine.

I too am not terribly comfortable with changing the behavior here, I'd rather we change the spec to match what everyone else is doing. But, I think I'm ok with experimenting on trunk

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/20f8d9eb6742
http://hg.mozilla.org/mozilla-central/rev/6035f8ed96b4
http://hg.mozilla.org/mozilla-central/rev/06aaa35724b1

Comment 22

[Martin Braun]

I think the spec should be changed - and all the hesitation to change this was justified.

I'm speaking from an application standpoint and I'm using the copy operation extensively in an application. Copy becomes pretty useless in many cases if it's replacing the whole content on the destination canvas instead of replacing only the bounded area/shape in the destination image. 

One use case is e.g. image masking:
The task is to make some area/shape in an image transparent. 
Since composite mode clear has been removed lately (as its not in the spec), the next best option is to draw the shape using mode copy and rgb(x,x,x,0.0) - pretty straightforward way of doing this. Not possible anymore with the current spec behaviour.

I'm quite sure this can be done some other way (which i'll probably have to figure that out now) but I think the most obvious and quickest way is how it has been done before.

Comment 23

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

(In reply to comment #22)
> One use case is e.g. image masking:
> The task is to make some area/shape in an image transparent. 
> Since composite mode clear has been removed lately (as its not in the spec),
> the next best option is to draw the shape using mode copy and rgb(x,x,x,0.0) -
> pretty straightforward way of doing this. Not possible anymore with the current
> spec behaviour.

Isn't it just as easy, or even easier, to do

ctx.rect(...);
ctx.clip();
ctx.clearRect(0, 0, canvas.width, canvas.height);
?

Comment 24

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

Well, not rect(), but whatever shape you need to clear.

Comment 25

[Martin Braun]

You're probably right that its just as easy. At least if you know about clipping paths. However not as intuitive in my opinion.

ctx.rect() composition mode "clear" was intuitive.
ctx.rect() composition mode "copy" and rgb(x,x,x,0.0) requires you to think a bit.
the above requires you to think a bit more - about clipping paths.

just my opinion and probably not an argument.

Comment 26

[bbbenbbbugs]

I am also coming from an application standpoint and I agree with both of Martin's points.

I agree that the spec is not crystal clear as to whether a mask should be used.

The unbounded copy functionality can be recreated by clearing the entire canvas before drawing whatever you need.  I do not think that such an easily duplicated operation would be worth adding a composite mode.

Bounded copy functionality can be used to ignore alpha settings and, e.g., draw a path to be 50% transparent black or 100% transparent black, etc. regardless of what is behind it.  Why might this be useful? If you want to clear a stroke of a complex path using a clip region, you would have to figure out a path that exactly encompasses the stroke's mask.  A stroke's mask depends on the points in the path, the lineWidth, lineCap, lineJoin, and sometimes the miterLimit.

Wanting to erase a stroked path: reasonable (in my mind).
Making client code calculate a clipping path that exactly recreates the mask of a stroke: insane (in my opinion).

Comment 27

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

It's crystal clear that what we currently implement is what the spec currently requires.

If you want the spec changed, I recommend taking that up on the WHATWG mailing list. Be prepared to specify exactly bounded copy should work (namely, exactly which pixels should be affected in all situations, and how, including when shadows are drawn and when global opacity is used). No-one has been able to come up with a good definition for that yet.

Thanks.