Implement setTransform for CanvasPattern

RESOLVED FIXED in mozilla33

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: tschneider, Assigned: milan)

Tracking

(Blocks 1 bug, {dev-doc-complete, feature})

Trunk
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Shumway:P1], )

Attachments

(1 attachment, 3 obsolete attachments)

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.
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.
Severity: enhancement → critical
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Shumway:P1]
Version: 30 Branch → Trunk
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?
Posted image Is this the "correct" result? (obsolete) —
For example, if you were to apply 45-ish degree rotation in the setTransform, would you get the result on the left?
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.
Attachment #8433608 - Flags: feedback?(till)
Attachment #8433608 - Flags: feedback?(schneider)
Assignee: nobody → milan
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 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.
Attachment #8433608 - Flags: feedback?(till)
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.
Attachment #8433588 - Attachment is obsolete: true
Attachment #8433608 - Attachment is obsolete: true
Attachment #8433608 - Flags: feedback?(schneider)
Attachment #8435213 - Flags: review?(gwright)
: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 :)
Review ping
Flags: needinfo?(gwright)
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.
Attachment #8435213 - Flags: review?(gwright) → review+
(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?
(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. :)
Flags: needinfo?(gwright)
Remove checkin-needed, I should get a DOM peer review first.
Keywords: checkin-needed
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.
Attachment #8439236 - Flags: review+ → review?(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
Attachment #8439236 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/1fbb41bd1d7a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Is there a need for manual testing on this feature?
Flags: in-testsuite?
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
Keywords: dev-doc-needed
Setting in-testsuite+ since an automated test landed for this bug. Please change back if you think other tests are needed.
Flags: in-testsuite? → in-testsuite+
Depends on: 1486318
You need to log in before you can comment on or make changes to this bug.