1019257 - Implement setTransform for CanvasPattern

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Tobias Schneider [:tobytailor]]

Having setTransform for CanvasPattern is a quite an useful new addition to the Canvas spec. When using the traditional Canvas API, the same effect could be achieved by changing the current transform right before calling fill(). This is not longer possible when using Shape2D objects since setting a transformation before calling fill(path) transforms the entire path drawn.

Comment 1

[Till Schneidereit [:till]]

This blocks landing of the Shumway "nat" development branch with its huge amount of correctness and performance fixes, so it's very high priority for us.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Can you attach a (simple) example and what the result would look like?  Also, we have this issue with repeat-* and fill (though we're ok with repeat-* and fillRect), like in bug 468358, would that need to be fixed as well, or is repeat-* not an issue?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Is this the "correct" result?

For example, if you were to apply 45-ish degree rotation in the setTransform, would you get the result on the left?

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: WIP: No error handling, etc., but is this what you/standard had in mind?

Let me know if this is the right direction for what the result would look like.  The setTransform is like the canvas setTransform, taking six doubles.

Comment 5

[Tobias Schneider [:tobytailor]]

Yeah, that's what the result using the tranform-before-fill workaround would look like and therefore exactly what we need. Don't think the standard had anything else in mind.

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 8433608 [details] [diff] [review]
WIP: No error handling, etc., but is this what you/standard had in mind?

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

Result-wise, this looks very good - and I'm delighted to see how simple it is to implement.

Looking at the spec more in-depth, however, I have a couple of additional questions:

- should setTransform really take an SVGMatrix? I don't think that makes sense, and your implementation is much better. (See also inline)
- regarding error handling, the spec only says "If a radial gradient or repeated pattern is used when the transformation matrix is singular, the resulting style must be transparent black". I know too little about WebIDL to know if everything else is implicitly covered by specifying the argument types. The spec, however, probably doesn't do a more thorough job here because it's not required if the matrix is supplied as an SVGMatrix instance

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4051,5 @@
>                                                  bool hasDirtyRect, int32_t dirtyX, int32_t dirtyY,
>                                                  int32_t dirtyWidth, int32_t dirtyHeight)
>  {
>    if (w == 0 || h == 0) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

This and the change below are unrelated cleanup, right?

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +286,5 @@
>  
>  interface CanvasPattern {
>    // opaque object
> +  [Throws, LenientFloat]
> +  void setTransform(double a, double b, double c, double d, double e, double f);

Per spec, setTransform takes an SVGMatrix. Given that CanvasRenderingContext2D#setTransform takes a 6-tuple of floats, and that SVGMatrix is supposed to be replaced by DOMMatrix, I think it makes a lot of sense to use the 6-tuple here, too. That should probably be brought up with spec people, though.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks for the quick feedback!

(In reply to Till Schneidereit [:till] from comment #6)
> Comment on attachment 8433608 [details] [diff] [review]
> ...
> 
> - should setTransform really take an SVGMatrix? I don't think that makes
> sense, and your implementation is much better. (See also inline)

I think the SVGMatrix is defined as a vector of six floats, but I'll definitely check that with people that know better.

> - regarding error handling, the spec only says "If a radial gradient or
> repeated pattern is used when the transformation matrix is singular, the
> resulting style must be transparent black".

Right, I'll cover those, and I was also thinking of the "here's 1.0e347, let's see if you start overwriting memory and cause a security issue" type of things.  There are also the tests that need to be added.


> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> > +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> 
> This and the change below are unrelated cleanup, right?

Yes, sorry about that, another bug.

Anyway, I'll keep this moving.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=e10f39e18e44

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

:gw280 - I still have the NS_ERROR_DOM_INVALID_STATE_ERR changes in the patch I attached, but that change wouldn't go in - I'm just anticipating seeing bug 1011574 land before this :)

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Review ping

Comment 12

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8435213 [details] [diff] [review]
Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL.

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

Looks reasonable to me.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4053,5 @@
>                                                  bool hasDirtyRect, int32_t dirtyX, int32_t dirtyY,
>                                                  int32_t dirtyWidth, int32_t dirtyHeight)
>  {
>    if (w == 0 || h == 0) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

Looks unrelated?

@@ +4107,5 @@
>    uint32_t dataLen = aArray->Length();
>  
>    uint32_t len = w * h * 4;
>    if (dataLen != len) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

Same - unrelated?

::: content/canvas/test/test_canvas.html
@@ +21551,5 @@
>    isPixel(ctx, 15, 5, 128, 0, 0, 255, 0);
>  }
>  </script>
>  
> +<p>Canvas test: 2d.composite.canvaspattern.setTransform</p>

Where did this test come from? I think test_canvas.html is supposed to be mostly taken from the canvas testsuite (http://philip.html5.org/tests/canvas/suite/tests/). I could be wrong about that though.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to George Wright (:gw280) from comment #12)
> ...
> > +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> 
> Same - unrelated?

Right - see comment 10 - part of bug 1011574 - I'll produce a patch without it.

> 
> ::: content/canvas/test/test_canvas.html
> @@ +21551,5 @@
> >    isPixel(ctx, 15, 5, 128, 0, 0, 255, 0);
> >  }
> >  </script>
> >  
> > +<p>Canvas test: 2d.composite.canvaspattern.setTransform</p>
> 
> Where did this test come from? I think test_canvas.html is supposed to be
> mostly taken from the canvas testsuite
> (http://philip.html5.org/tests/canvas/suite/tests/). I could be wrong about
> that though.

I just wrote the test (it at least passes the sanity check in that it fails if setTransform is a stub), I don't think it's part of the canvas testsuite yet.  Should I take it out and forget about it, move it somewhere else or just leave it in and start some process to get the "real test" in the canvas testsuite somehow created?

Comment 14

[George Wright (:gw280) (needinfo me!)]

(In reply to Milan Sreckovic [:milan] from comment #13)

> I just wrote the test (it at least passes the sanity check in that it fails
> if setTransform is a stub), I don't think it's part of the canvas testsuite
> yet.  Should I take it out and forget about it, move it somewhere else or
> just leave it in and start some process to get the "real test" in the canvas
> testsuite somehow created?

Let's move it into test_2d.composite.canvaspattern.setTransform.html in content/canvas/test. Not a fan of lumping everything into test_canvas.html anyway. :)

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=f1d8dd4df1cd

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL. Test in a new file, rather than test_canvas.html. Carry r=gw280.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Remove checkin-needed, I should get a DOM peer review first.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8439236 [details] [diff] [review]
Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL.  Test in a new file, rather than test_canvas.html.  Carry r=gw280.

Forgot about the DOM peer review (already have the canvas one.)  Boris won, because he knows the context of bug 1020975 mentioned in the webidl file.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8439236 [details] [diff] [review]
Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL.  Test in a new file, rather than test_canvas.html.  Carry r=gw280.

r=me

Comment 20

[Till Schneidereit [:till]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbb41bd1d7a

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1fbb41bd1d7a

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is there a need for manual testing on this feature?

Comment 23

[Till Schneidereit [:till]]

I changed the availability information in [1], but this could probably do with some explanation/examples.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/CanvasPattern

Comment 24

[Mihaela Velimiroviciu (:mihaelav)]

Setting in-testsuite+ since an automated test landed for this bug. Please change back if you think other tests are needed.

Comment 25

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/CanvasPattern.setTransform
https://developer.mozilla.org/en-US/Firefox/Releases/33#Interfaces.2FAPIs.2FDOM