1071774 - using drawImage with SVG source resets the context's current transformation

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Simon Sarris]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2159.4 Safari/537.36

Steps to reproduce:

Scale the canvas, draw an SVG image, then inspect the context's transformation matrix. It gets reset! That's way bad.

Example:

ctx.scale(5,5);
ctx.fillStyle = 'rgba(0,0,0,.5)'

var img = new Image();
img.onload = function() {
  // comment this out to see what happens in Firefox Nightly:
  console.log(ctx.mozCurrentTransform) // 5,0,0,5,0,0
  ctx.drawImage(img, 0, 0, 29, 20, 0, 0, 29, 20);
  console.log(ctx.mozCurrentTransform) // 1,0,0,1,0,0 <- THATS BAD.
  
  ctx.fillRect(10,10,10,10);
}
img.src = "http://gojs.net/temp/draft-vm.svg";


Live example: http://codepen.io/simonsarris/pen/mKqye


Actual results:

The context's transformation matrix gets reset.


Expected results:

ctx.drawImage() should not have side effects like this.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Regression

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3488976ecf0f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131153
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d51132a0099
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131352
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3488976ecf0f&tochange=3d51132a0099
Regressed by:
3d51132a0099	Seth Fowler — Bug 1043560 - Refactor the imgIContainer::Draw API. r=tn,dholbert,jwatt,mwu,mattwoodrow,roc sr=jrmuizel

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Actually, I had to move the imgFrame.cpp changes further up the stack. It seems that we need the transform to still be changed when we call gfxUtils::DrawPixelSnapped. The following tests fail if it isn't:

image/test/reftest/gif/small-background-size.gif
image/test/reftest/gif/small-background-size-2.gif
layout/reftests/bugs/456330-1.gif
layout/reftests/bugs/488685-1.html

We should redesign this code as per the comment I left in the code, but I don't have time to think about that right now.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8496417 [details] [diff] [review]
patch

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

Looks good!

I thought I'd summarize our discussion on IRC about this so it's recorded somewhere: The fact that changing the matrix of a temporary gfxContext wrapping an underlying DrawTarget causes a permanent change in the underlying DrawTarget's matrix is profoundly unintuitive to me. You mentioned that we aren't supposed to wrap an existing DrawTarget in a temporary gfxContext. We concluded that it would be very useful if we could figure out how to assert about that, since it's not obvious from the API.

Comment 6

[Simon Sarris]

You guys are the best.

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abf96b7fa963

Thanks for reporting this, Simon.

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 8496417 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1043560
[User impact if declined]: HTML <canvas> transform broken if image draw to it
[Describe test coverage new/current, TBPL]: passed full Try run, plus new reftest, and now on m-i
[Risks and why]: should be low risk, since it only saves and restores a transform
[String/UUID change made/needed]: none

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Jonathan, 33 is marked as unaffected and you request an uplift to beta. Which one is right?

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 8496417 [details] [diff] [review]
patch

Sorry, getting my version numbers confused.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abf96b7fa963

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8496417 [details] [diff] [review]
patch

Aurora+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3baf1d41b83