Last Comment Bug 735373 - 3D transforms of images seems to have been broken
: 3D transforms of images seems to have been broken
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 747381 (view as bug list)
Depends on:
Blocks: 724614 745523
  Show dependency treegraph
 
Reported: 2012-03-13 12:08 PDT by :Aryeh Gregor (working until September 2)
Modified: 2016-03-05 01:12 PST (History)
8 users (show)
ayg: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed


Attachments
Make ChooseScaleAndSetTransform correctly detect active layers (7.69 KB, patch)
2012-03-14 21:49 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review

Description :Aryeh Gregor (working until September 2) 2012-03-13 12:08:56 PDT
data:text/html,<!DOCTYPE html>
<img style="-moz-transform:rotatex(30deg)"
 src='http://www.mozilla.org/img/download-logo-sub.png'>
<img style="-moz-transform:scale3d(2,2,2)"
 src='http://www.mozilla.org/img/download-logo-sub.png'>

The first image is translated down, not squished vertically as one would expect.  The second disappears instead of being scaled.  Something seems to be badly wrong here.
Comment 1 :Aryeh Gregor (working until September 2) 2012-03-13 14:07:57 PDT
Better test-case for the latter transform:

data:text/html,<!DOCTYPE html>
<img style="-moz-transform:scale3d(-2,1,2)"
 src='http://www.mozilla.org/img/download-logo-sub.png'>

This translates the image toward the right.  It seems like bitmap images affected by 3D transforms are being translated for some reason, instead of being transformed in the manner they should.  Another example:

data:text/html,<!DOCTYPE html>
<img style="-moz-transform:perspective(100px) scale(-2,1)"
 src='http://www.mozilla.org/img/download-logo-sub.png'>

Was this really always here?  It must be a regression, it's too crazy for no one to have noticed it until now.
Comment 2 Alice0775 White 2012-03-13 18:41:34 PDT
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6799a5e6912f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120312 Firefox/13.0a1 ID:20120312224507
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4bd80687cb32
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120312 Firefox/13.0a1 ID:20120312225607
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6799a5e6912f&tochange=4bd80687cb32
Comment 3 Alice0775 White 2012-03-13 18:43:49 PDT
Sorry,

Please Ignore comment #2
Comment 4 Alice0775 White 2012-03-13 18:57:18 PDT
Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/cd4853b0b94a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120226 Firefox/13.0a1 ID:20120226031015
Fails:
http://hg.mozilla.org/mozilla-central/rev/b98fc24ac54b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120226 Firefox/13.0a1 ID:20120226151249
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cd4853b0b94a&tochange=b98fc24ac54b

Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c928dc7c9dff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120225 Firefox/13.0a1 ID:20120225011949
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7dcff354aeae
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120225 Firefox/13.0a1 ID:20120225014049
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c928dc7c9dff&tochange=7dcff354aeae

Suspected:
7dcff354aeae	Aryeh Gregor — Bug 724614 - 3D scaling makes text blurry. r=mattw
Comment 5 :Aryeh Gregor (working until September 2) 2012-03-14 11:02:14 PDT
Um, oops.  Matt, any idea on what the problem is here?  I more or less just put something in my patch that looked like it might work, and it seemed to, so I figured it was okay.  I don't know what would have caused this regression.  If you don't have any ideas offhand on how to fix it properly, I can write a patch to just back out the change.
Comment 6 Matt Woodrow (:mattwoodrow) 2012-03-14 21:49:08 PDT
Created attachment 606110 [details] [diff] [review]
Make ChooseScaleAndSetTransform correctly detect active layers

Aryeh: Looks like your patch just exposed a different bug.

ChooseScaleAndSetTransform was using AreLayerMarkedActive to check for an active transform layer, which doesn't accurately reflect the result of GetLayerState().

What this meant was that we were removing the 2x2 scale from the transform, with the intent of applying it to the content and then never doing the latter.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-17 21:04:39 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> What this meant was that we were removing the 2x2 scale from the transform,
> with the intent of applying it to the content and then never doing the
> latter.

This confuses me. Why were we never applying it? The scale factors in the ContainerParameters returned by ChooseScaleAndSetTransform must *always* be applied. If they're not being applied in some cases, we should fix that.
Comment 8 Matt Woodrow (:mattwoodrow) 2012-03-18 14:47:26 PDT
Why only apply it if mInActiveTransformedSubtree is set to tree, which it wasn't previously. This patch fixes that.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-19 00:58:53 PDT
Seems to me that that code in PopThebesLayerData should be unconditional instead of testing mInActiveTransformedSubtree, no? If there are scale factors, they should always be applied. Arguably ConfigureLayer should take mParameters as a parameter and do the adjustment itself.
Comment 10 Matt Woodrow (:mattwoodrow) 2012-03-21 18:13:23 PDT
I'm not actually sure why, but being conditional on active transforms is correct. I guess because inactive layers are being drawn into a context with the scale already applied?

This bug only affects 3d transforms (since they are always active), there's no bug with 2d equivalents.

My patch just fixes the active layer detection code to be correct, but I can look into other solutions if you'd like.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-21 18:52:04 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I'm not actually sure why, but being conditional on active transforms is
> correct. I guess because inactive layers are being drawn into a context with
> the scale already applied?

Probably.

I believe my proposal in comment #9 is correct and will fix this bug. It leads to simpler code since the scale factors are always applied instead of only being applied in some cases. So we should do that. Am I wrong?
Comment 12 :Aryeh Gregor (working until September 2) 2012-06-04 10:41:44 PDT
So this is a standing regression from Firefox 13.  If a proper fix isn't going to land soon, shouldn't we just back out bug 724614?
Comment 13 Alice0775 White 2012-06-04 11:33:11 PDT
Fixed window(m-c):
Bad:
http://hg.mozilla.org/mozilla-central/rev/c00a9c1940c5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120515185430
Fixed:
http://hg.mozilla.org/mozilla-central/rev/bfc259be3fa2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516020729
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c00a9c1940c5&tochange=bfc259be3fa2

Fixed window(m-c):
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b69d000b726a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120514231326
Fixed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd3c873674fc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120515030522
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b69d000b726a&tochange=dd3c873674fc

It seems fixed in Firefox14 and Firefox15 by Bug 750334
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-04 18:03:30 PDT
(In reply to Aryeh Gregor from comment #12)
> So this is a standing regression from Firefox 13.  If a proper fix isn't
> going to land soon, shouldn't we just back out bug 724614?

Yes. This bug should have been tracked for FF13 :-(
Comment 15 :Aryeh Gregor (working until September 2) 2012-06-05 01:15:52 PDT
(In reply to Alice0775 White from comment #13)
> Fixed window(m-c):
> Bad:
> http://hg.mozilla.org/mozilla-central/rev/c00a9c1940c5
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
> ID:20120515185430
> Fixed:
> http://hg.mozilla.org/mozilla-central/rev/bfc259be3fa2
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
> ID:20120516020729
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=c00a9c1940c5&tochange=bfc259be3fa2
> 
> Fixed window(m-c):
> Bad:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/b69d000b726a
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
> ID:20120514231326
> Fixed:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/dd3c873674fc
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
> ID:20120515030522
> Pushlog:
> http://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=b69d000b726a&tochange=dd3c873674fc
> 
> It seems fixed in Firefox14 and Firefox15 by Bug 750334

Thanks, Alice!  You're awesome.  :)  Confirmed works for me now.  We should add a test, though.  (I have some in the CSS Transforms tests I'm submitting, so if those are ever imported that should suffice.)
Comment 16 :Aryeh Gregor (working until September 2) 2012-06-26 02:31:51 PDT
*** Bug 747381 has been marked as a duplicate of this bug. ***
Comment 17 Paul Silaghi, QA [:pauly] 2012-06-26 06:44:43 PDT
http://jsbin.com/3/ugipen/5/edit?html,css,live - no difference if removing the background color

data:text/html,<!DOCTYPE html>
<img style="-moz-transform:rotatex(30deg)"
 src='http://www.mozilla.org/img/download-logo-sub.png'>
<img style="-moz-transform:scale3d(2,2,2)"
 src='http://www.mozilla.org/img/download-logo-sub.png'>

The first image is squashed vertically as expect and the second one doesn't disappear.


data:text/html,<!DOCTYPE html>
<img style="-moz-transform:scale3d(-2,1,2)"
 src='http://www.mozilla.org/img/download-logo-sub.png'>
The image is squashed vertically.

Verified fixed on FF 14b9 on Win 7, Ubuntu 12.04 and Mac OS X 10.6.

Note You need to log in before you can comment on or make changes to this bug.