664884 - Implement getters/setters for CTM and inverse CTM

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Currently setTransform() allows replacing the CTM, but there's no way to get the CTM.  In pdf.js, our use case is that we need to be able to fill the current clip region, but there's no canvas API that does that (i.e., no analogue of cairo_paint()).  paint() can be emulated by filling the canvas bounds, but do this properly, we need to fill the bounds transformed by the CTM inverse.  It's possible for us to track the CTM in JS, implementing our own little matrix math library, but since the gfx lib behind canvas must be doing this already, it's wasteful and potentially harmful for all JS code that needs the CTM to track it themselves.

So we discussed adding
  attribute currentTransform;
  attribute currentTransformInverse;

Upcoming patches implement those as mozCurrentTransform(Inverse).

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement mozCurrentTransform/mozCurrentTransformInverse attributes for manipulating the canvas CTM

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test mozCurrentTransform/mozCurrentTransformInverse

Comment 3

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

I don't think throwing on a singular matrix is the right thing to do. In general canvas APIs don't throw on exceptional values. Setting the inverse to a singular matrix should just set the transform to all zeroes or something like that.

GetMozCurrentTransformInverse also seems wrong to assert that the gfxContext matrix is not singular. The gfxContext matrix can be singular. You should define the behavior when the matrix is singular (I suggest returning all zeroes).

Comment 4

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

(I didn't look at patches, just reacting to Roc's comment)

(In reply to comment #3)
> I don't think throwing on a singular matrix is the right thing to do.

Indeed!!

Look at FP arithmetic: there's a reason why FP exceptions such as divide-by-zero (as opposed to integer divide-by-zero) are not raising signals by default: it's too painful to have to always catch them.

The same goes here: throwing on singular matrices would force a lot of user code to put try { ... } catch statements everytime.

> In
> general canvas APIs don't throw on exceptional values. Setting the inverse
> to a singular matrix should just set the transform to all zeroes or
> something like that.

Whatever you want; alternatively you could even not do anything about it and just let inf/nan values happen.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

cairo doesn't allow setting a singular transform.

If silent no-op on obvious error is the general m.o. in canvas-land, ok.  I don't think it's helpful to authors in the long run, but better to adhere to convention.

Comment 6

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

The problem with disallowing singular transforms is that that's trying to map a continuum (of condition numbers) to a boolean (singular or invertible). At some point you have to introduce an arbitrary threshold. Granted, for very small matrices like in graphics, a more reasonable condition is to check whether the determinant is exactly 0, at least that's a boolean already, and I can believe that that's useful in practice (I don't know) but that won't catch slightly more subtle cases such as: make a matrix with condition number 1e+20, multiply it by itself (i.e. apply twice the same transformation), now you have condition number 1e+40 == inf (in single precision) with means that whatever inverse matrix you compute will be meaningless.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 539954 [details] [diff] [review]
Implement mozCurrentTransform/mozCurrentTransformInverse attributes for manipulating the canvas CTM

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

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +1441,5 @@
>  
> +// Make a double out of |v|, treating undefined values as 0.0 (for the
> +// sake of sparse arrays).  Return true iff coercion succeeded.
> +static bool
> +CoerceDouble(jsval v, double* d)

This is duplicated with bug 662038 to, I presume, avoid dependencies?

@@ +1456,5 @@
> +    return true;
> +}
> +
> +// Return true iff the conversion succeeded, false otherwise.  *rv is
> +// the value to return to script if this returns false.

Why not just return an nsresult and wrap calls in NS_SUCCEEDED?

@@ +1462,5 @@
> +JSValToMatrix(JSContext* cx, const jsval& val, gfxMatrix* matrix,
> +              nsresult* rv)
> +{
> +    double* elts[] = { &matrix->xx, &matrix->yx, &matrix->xy, &matrix->yy,
> +                       &matrix->x0, &matrix->y0 };

We should definitely specify *somewhere* what order (column-major or row-major) the matrices we're returning are in.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #7)
> Comment on attachment 539954 [details] [diff] [review] [review]
> Implement mozCurrentTransform/mozCurrentTransformInverse attributes for
> manipulating the canvas CTM
> 
> Review of attachment 539954 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2D.cpp
> @@ +1441,5 @@
> >  
> > +// Make a double out of |v|, treating undefined values as 0.0 (for the
> > +// sake of sparse arrays).  Return true iff coercion succeeded.
> > +static bool
> > +CoerceDouble(jsval v, double* d)
> 
> This is duplicated with bug 662038 to, I presume, avoid dependencies?

Not duplicated sire, but moved.

> @@ +1456,5 @@
> > +    return true;
> > +}
> > +
> > +// Return true iff the conversion succeeded, false otherwise.  *rv is
> > +// the value to return to script if this returns false.
> 
> Why not just return an nsresult and wrap calls in NS_SUCCEEDED?

Because if we see a non-finite float we're supposed to abort the call but return NS_OK.  (Existing code does that, following convention.)

> 
> @@ +1462,5 @@
> > +JSValToMatrix(JSContext* cx, const jsval& val, gfxMatrix* matrix,
> > +              nsresult* rv)
> > +{
> > +    double* elts[] = { &matrix->xx, &matrix->yx, &matrix->xy, &matrix->yy,
> > +                       &matrix->x0, &matrix->y0 };
> 
> We should definitely specify *somewhere* what order (column-major or
> row-major) the matrices we're returning are in.

Yeah.  It's the same order as args to setTransform().

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Rob/Benoit: to my eyes, the discussion about singular matrices kind of trailed off.  Do you guys have specific suggestions?

I guess we could make setting the transform (or inverse) be a no-op NS_OK on singular matrices.  Note that canvas doesn't check that currently for setTransform(), so setting a singular transform will put the cairo_t into an error state.  I don't like either option as explained above, but this isn't my front yard.

Comment 10

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

Setting a singular transform in either the transform or its inverse should definitely not throw. I don't think such a setting should be ignored; if I scale by (0,0) then logically, drawing should do nothing. So I think the logical thing to do is to simply draw nothing while there is a singular matrix. As for reading the transform or its inverse, I think it would be fine to return all-zeroes but there are other reasonable options. So I think we should detect singularity and set a flag that means "draw nothing".

Hmm, I think it's also observable where points get mapped to by a singular matrix, if you switch to a singular matrix during path emmission. So we should define that as well. Maybe this has been discussed on the WHATWG or canvas lists?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

You're talking about a lot of things that would need to be implemented on top of cairo.  (What about azure?)  cairo treats singular matrices as an error state.  I don't know whether that's good/bad/indifferent/implementation-detail or not.  The current canvas spec says nothing about the transform in this regard.

Since the behavior of canvas with a singular transform is literally undefined by the current canvas spec, can we pick some inoffensive behavior for currentTransform/currentTransformInverse and worry about the more general issue of singular matrices separately?  I think the benefit of the new interfaces outweighs the spec-work slog of defining behavior under singular transforms.

In the patches here, I could remove the singularity checks and the new interfaces would behave as setTransform([singular matrix]) now: namely, script would never be able to observe a singular transform through currentTransform/currentTransformInverse (since the setTransform([singular matrix]) would fail), and all subsequent canvas calls would be ignored (since the cairo_t would enter an error state).

Adding a "transform-is-singular" flag and no-oping while set would require checking the flag on basically every canvas entry point, I think.  Is the flexibility enabled by that worth the pervasiveness of the change?

Comment 12

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

(In reply to comment #11)
> You're talking about a lot of things that would need to be implemented on
> top of cairo.  (What about azure?)  cairo treats singular matrices as an
> error state.  I don't know whether that's
> good/bad/indifferent/implementation-detail or not.  The current canvas spec
> says nothing about the transform in this regard.

The spec doesn't specifically mention singular transforms. In the absence of any special mention, I think we have to assume they don't get special treatment, so an exception should not be thrown. The spec talks about applying the matrix via "transforming points" and you can certainly transform points via a singular matrix --- everything just gets transformed to a single point or line --- so I think that's what should happen.

(Obviously I think cairo made the wrong choice.)

This is basically the issue in bug 612033. Webkit apparently does not give singular transforms special treatment. See also bug 659201.

> In the patches here, I could remove the singularity checks and the new
> interfaces would behave as setTransform([singular matrix]) now: namely,
> script would never be able to observe a singular transform through
> currentTransform/currentTransformInverse (since the setTransform([singular
> matrix]) would fail), and all subsequent canvas calls would be ignored
> (since the cairo_t would enter an error state).

I'm OK with not fixing singular transforms now since bug 612033 already exists, but I would like to define how currentTransform(Inverse) *should* work with singular transforms.

How about: currentTransform is always a valid matrix (singular or not), getting currentTransformInverse when currentTransform is singular returns an array of NaNs, and setting currentTransformInverse to a singular matrix is ignored?

> Adding a "transform-is-singular" flag and no-oping while set would require
> checking the flag on basically every canvas entry point, I think.  Is the
> flexibility enabled by that worth the pervasiveness of the change?

There aren't that many canvas entry points.

However I think for performance we should delegate this problem to Azure. Let's make Azure handle singular transforms the way we expect for canvas. So if an Azure backend (like the cairo backend) doesn't do what we want, we fix it there.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm curious why cairo made the choice they did.

All the rest sounds good to me.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1.1: Don't special case singular transforms.

Implements later discussion.  I'll fold this into the earlier patches.

BTW, Jeff sez

[12:39]	cjones	jrmuizel, why does cairo avoid nonsingular transforms?
[12:39]	cjones	er
[12:39]	cjones	avoid singular ones
[12:40]	jrmuizel	because we often end up needing the inverse and having to check if it's singular is a pain
[12:41]	cjones	so would it be fair to say that's an implementation detail?
[12:41]	jrmuizel	drawing with a singular matrix tends to be a special case vs. something that just works
[12:41]	cjones	instead of a semantic problem?
[12:41]	cjones	i believe that
[12:41]	jrmuizel	cjones: maybe

Comment 15

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

Sounds like an implementation detail to me.

Comment 16

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

Comment on attachment 541168 [details] [diff] [review]
part 1.1: Don't special case singular transforms.

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

This looks fine except I think this reftests (and others in this directory) should actually be mochitests.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #16)
> Comment on attachment 541168 [details] [diff] [review] [review]
> part 1.1: Don't special case singular transforms.
> 
> Review of attachment 541168 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> This looks fine except I think this reftests (and others in this directory)
> should actually be mochitests.

Just to be clear, you mean the *-sanity tests, right?

Comment 18

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

Comment on attachment 539954 [details] [diff] [review]
Implement mozCurrentTransform/mozCurrentTransformInverse attributes for manipulating the canvas CTM

So I like the functionality; and the API isn't bad, but I wonder if we should be trying to reuse things like the SVG matrix object here (or CSS matrix object or whatever).  Feel free to call bikeshed on that; sr+ regardless.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't think that's bikeshedding.  I'm not against using SVGMatrix, but array-like thing is more flexible and less verbose.  I sort of lean towards the lower-level thing, because it's easy for authors to add wrappers for currentTransform/Inverse that understand SVGMatrix (or whatever else), but baking SVGMatrix into currentTransform/Inverse means we're stuck with it forever and can't cooperate with the next WebOohShinyMatrix without more translation overhead.

Comment 20

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

(In reply to comment #17)
> Just to be clear, you mean the *-sanity tests, right?

Yes.

Comment 21

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

I could go either way on the Matrix object question. Probably go with the array for now and deal with the Matrix object bikeshedding when we come to spec it.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Share some common code and fix some build warnings

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Add JSVal<->Matrix helpers

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Implement canvas.mozCurrentTransform(Inverse) for Azure

Comment 25

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

Comment on attachment 542674 [details] [diff] [review]
part 2: Share some common code and fix some build warnings

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

::: content/canvas/src/CanvasUtils.cpp
@@ +66,2 @@
>                                        nsIPrincipal *aPrincipal,
>                                        PRBool forceWriteOnly)

Fix indent

::: content/canvas/src/CanvasUtils.h
@@ +52,5 @@
>  
> +inline PRBool CheckSaneSubrectSize(PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h,
> +                            PRInt32 realWidth, PRInt32 realHeight) {
> +    CheckedInt32 checked_x_plus_w  = CheckedInt32(x) + w;
> +    CheckedInt32 checked_y_plus_h  = CheckedInt32(y) + h;

checked_xmost and checked_ymost to be consistent with other usage

Comment 26

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

Comment on attachment 542677 [details] [diff] [review]
part 4: Implement canvas.mozCurrentTransform(Inverse) for Azure

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

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #25)
> Comment on attachment 542674 [details] [diff] [review] [review]
> part 2: Share some common code and fix some build warnings
> 
> Review of attachment 542674 [details] [diff] [review] [review]:

Done.

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 542675 [details] [diff] [review]
part 3: Add JSVal<->Matrix helpers

Too bad we couldn't use JSValToMatrixElts from within JSValToMatrix(Matrix).

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ddce22d83b0d
http://hg.mozilla.org/integration/mozilla-inbound/rev/1382428eb6ce

Comment 30

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/0949d3090bba
http://hg.mozilla.org/mozilla-central/rev/5166d82391ee

looks like comment 29 has the wrong changesets, those are from Bug 662038 (that landed with this one).

Comment 31

[AndreiD[QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a2) Gecko/20110706 Firefox/7.0a2
Verified fixed; detailed by the changesets (comment 30)