Last Comment Bug 724614 - 3D scaling makes text blurry
: 3D scaling makes text blurry
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Linux
: -- minor (vote)
: mozilla13
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 1098275 735373 747381
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-06 10:57 PST by :Aryeh Gregor (away until August 15)
Modified: 2014-11-13 07:59 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.03 KB, patch)
2012-02-07 08:38 PST, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch with better-named tests (3.99 KB, patch)
2012-02-07 08:43 PST, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch v3 (4.74 KB, patch)
2012-02-07 13:12 PST, :Aryeh Gregor (away until August 15)
matt.woodrow: review+
Details | Diff | Splinter Review
Patch v4, now with fails-if(Android) (4.77 KB, patch)
2012-02-08 08:36 PST, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch v5, now with fails-if(Android) for the right tests (4.85 KB, patch)
2012-02-09 07:16 PST, :Aryeh Gregor (away until August 15)
matt.woodrow: review+
Details | Diff | Splinter Review
Patch v6, now with yet more reftest fidgeting! (6.96 KB, patch)
2012-02-23 10:07 PST, :Aryeh Gregor (away until August 15)
matt.woodrow: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-02-06 10:57:42 PST
Test-case:

data:text/html,<!doctype html>
<div style="-moz-transform:scale3d(2,2,2);
-moz-transform-origin: top left">
Some text</div>

Logically, the scale3d(2,2,2) here should act the same as scale(2).  But scale(2) produces crisp text for me, while scale3d(2,2,2) produces blurry text.  This is nightly 2012-02-06, Ubuntu 11.10 with nouveau+Gallium3D.

In other words, layout/reftests/transform-3d/scale3d-1-ref.html and scale3d-2-ref.html in that directory should look the same.  Note that IE10 Developer Preview and Chrome 18 dev seem to suffer from the exact same bug, though, so it's not going to be an interop problem.
Comment 1 Boris Zbarsky [:bz] 2012-02-06 11:03:43 PST
> Logically, the scale3d(2,2,2) here should act the same as scale(2)

Should it?  What does scale3d mean, exactly?  It's not like the spec actually says....

What I see happening is that scale3d(2,2,1) acts like scale(2) (and does what looks like drawing text at a larger size, as expected) while scale3d(2,2,x) for x != 1 seems to be doing pixel-level scaling of the text rasterized at the original size...
Comment 2 :Aryeh Gregor (away until August 15) 2012-02-06 11:51:53 PST
scale3d(a,b,c) is just

matrix3d(a,0,0,0,
         0,b,0,0,
         0,0,c,0,
         0,0,0,1),

although the spec seems to not want to include any matrices except by reference to SVG, so it doesn't say this.  So it maps (x,y,z) to (ax,by,cz).  In particular, it maps (x,y,0) to (ax,by,0), which is the same as what scale(a,b) does.  So if everything lies on the x-y plane to start with, the third coordinate of scale3d() may as well be treated as 1.

I'm seeing the same thing as you.  I see the same type of effect on SVGs too:

<!doctype html>
<div style="-moz-transform:scale3d(2,2,2);
-moz-transform-origin: top left">
<svg height=100 width=100>
<circle cx=50 cy=50 r=50>
</svg>
</div>

scale3d(2,2,2) makes the edge blurry, scale3d(2,2,1) does not.  Likewise:

<!doctype html>
<div style="-moz-transform:rotate3d(0,0.000001,1,45deg);
-moz-transform-origin: top left">
Some text</div>

It looks significantly different if you change 0.000001 to 0.

I have no idea what the implementation is, so maybe optimizing for this case doesn't make sense.  It would be nice if vector stuff could remain crisp when doing arbitrary 3D transforms, but maybe that's not realistic?
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-06 17:02:10 PST
This is because our code that automatically increases the resolution of the transformed layer (ChooseScaleAndSetTransform in FrameLayerBuilder.cpp) only handles 2D transforms and leaves 3D transforms alone.

It wouldn't be hard to fix that to handle 3D cases. We just need to come up with a way to determine what good scale factors are, which is tricky for 3D.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-06 17:04:05 PST
If someone came up with a gfx3DMatrix version of gfxMatrix::ScaleFactors (we could pass in a reference point if that's helpful, say the center of the transformed frame), the rest of the code would be easy to write.
Comment 5 Matt Woodrow (:mattwoodrow) 2012-02-06 17:27:49 PST
For this particular example (3d transforms without perpsective), we can just call ProjectTo2D on the matrix (removes any Z axis scaling) to get an equivalent 2d matrix.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-06 18:57:54 PST
I reckon you could fix this in five minutes from here :-)
Comment 7 :Aryeh Gregor (away until August 15) 2012-02-07 08:36:38 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> This is because our code that automatically increases the resolution of the
> transformed layer (ChooseScaleAndSetTransform in FrameLayerBuilder.cpp) only
> handles 2D transforms and leaves 3D transforms alone.
> 
> It wouldn't be hard to fix that to handle 3D cases. We just need to come up
> with a way to determine what good scale factors are, which is tricky for 3D.

So my first thought would be to apply the transform to (1,0,0), project the result to the x-y plane, and use the length of that, then repeat for (0,1,0).  But the problem is that perspective messes that up, right?  If there's a strong perspective effect, one scaling factor for all the text might not produce good results no matter what.

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> For this particular example (3d transforms without perpsective), we can just
> call ProjectTo2D on the matrix (removes any Z axis scaling) to get an
> equivalent 2d matrix.

I was about to observe that, yeah.  In general, correct me if I'm wrong: if 'perspective' is "none", and 'transform-style' is "flat", and the transformation matrix's last row is [0, 0, 0, 1] (no perspective() etc.), the transform should always be equivalent to a 2D transform -- right?  You should always be able to call ProjectTo2D() in this case, and thereby use the (possibly simpler/faster/better) 2D code paths across the board.

More specifically, I'd think ChooseScaleAndSetTransform should be able to use ProjectTo2D() to compute scale factors whenever there's no perspective -- i.e., CanDraw2D().  I wrote a simple patch that seems to do that fine (about to be attached).
Comment 8 :Aryeh Gregor (away until August 15) 2012-02-07 08:38:13 PST
Created attachment 595052 [details] [diff] [review]
Patch

Didn't run on tryservers, because my account hasn't been created yet (bug 724573).  I ran the tests in layout/reftests/transform-3d/.  No idea if this patch is correct, but it makes sense to me.  :)
Comment 9 :Aryeh Gregor (away until August 15) 2012-02-07 08:43:27 PST
Created attachment 595056 [details] [diff] [review]
Patch with better-named tests

I just realized my patch names the test files strangely -- "all-separate" doesn't scale in all dimensions.  So I made the ref file do only "scaleX(2) scaleY(2)" instead of "scaleX(2) scaleY(2) scaleZ(2)".  Doesn't actually change what's being tested.
Comment 10 Matt Woodrow (:mattwoodrow) 2012-02-07 12:35:18 PST
Comment on attachment 595056 [details] [diff] [review]
Patch with better-named tests

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1699,5 @@
>    }
>  
>    gfxMatrix transform2d;
> +  bool is2D = transform.Is2D();
> +  bool canDraw2D = transform.CanDraw2D(&transform2d);

I think we can just remove the Is2D() line here, and still call the variable canDraw2D. The only other usage of is2D should be fine with the result of CanDraw2D.

::: layout/reftests/transform-3d/reftest.list
@@ +20,5 @@
>  == preserve3d-3a.html preserve3d-3-ref.html
>  == preserve3d-4a.html green-rect.html
>  == scale3d-z.html scalez-1-ref.html
>  == scale3d-all.html scale3d-1-ref.html
> +== scale3d-all-separate.html scale3d-1-ref.html

This file isn't in the patch.

@@ +21,5 @@
>  == preserve3d-4a.html green-rect.html
>  == scale3d-z.html scalez-1-ref.html
>  == scale3d-all.html scale3d-1-ref.html
> +== scale3d-all-separate.html scale3d-1-ref.html
> +== scale3d-xz.html scale3d-1-ref.html

Why did this change the reference file?
Comment 11 :Aryeh Gregor (away until August 15) 2012-02-07 13:12:08 PST
Created attachment 595144 [details] [diff] [review]
Patch v3

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I think we can just remove the Is2D() line here, and still call the variable
> canDraw2D. The only other usage of is2D should be fine with the result of
> CanDraw2D.

Okay.  I erred on the side of safety because I didn't understand the second usage.

> > +== scale3d-all-separate.html scale3d-1-ref.html
> 
> This file isn't in the patch.

Isn't it?  It was renamed.

  rename from layout/reftests/transform-3d/scale3d-2-ref.html
  rename to layout/reftests/transform-3d/scale3d-all-separate.html

> > +== scale3d-all-separate.html scale3d-1-ref.html
> > +== scale3d-xz.html scale3d-1-ref.html
> 
> Why did this change the reference file?

Because this change means scale3d-1-ref.html and scale3d-2-ref.html should look identical, so they shouldn't both be refs -- having one ref is enough.  I moved the second ref to be a test file.
Comment 12 Matt Woodrow (:mattwoodrow) 2012-02-07 13:54:09 PST
Comment on attachment 595144 [details] [diff] [review]
Patch v3

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

Oh sorry, it is too. Splinter apparently doesn't show renames at all. How useful :)
Comment 13 Mozilla RelEng Bot 2012-02-07 18:25:07 PST
Autoland Patchset:
	Patches: 595144
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/772dc5196079
Try run started, revision 772dc5196079. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=772dc5196079
Comment 14 Mozilla RelEng Bot 2012-02-08 04:00:28 PST
Try run for 772dc5196079 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=772dc5196079
Results (out of 212 total builds):
    success: 191
    warnings: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-772dc5196079
 Timed out after 06 hours without completing.
Comment 15 :Aryeh Gregor (away until August 15) 2012-02-08 08:36:40 PST
Created attachment 595421 [details] [diff] [review]
Patch v4, now with fails-if(Android)

The changed reftests fail on Android, so this version disables them.  (Or should I deal with the failure some other way?)
Comment 16 Mozilla RelEng Bot 2012-02-08 09:58:37 PST
Autoland Patchset:
	Patches: 595421
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/0e5aed55407c
Try run started, revision 0e5aed55407c. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=0e5aed55407c
Comment 17 Matt Woodrow (:mattwoodrow) 2012-02-08 12:43:56 PST
It would be nice to know why they fail, but they look like antialiasing differences.
Comment 18 Mozilla RelEng Bot 2012-02-08 14:45:22 PST
Try run for 0e5aed55407c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0e5aed55407c
Results (out of 210 total builds):
    exception: 1
    success: 176
    warnings: 19
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-0e5aed55407c
Comment 19 :Aryeh Gregor (away until August 15) 2012-02-09 07:16:36 PST
Created attachment 595728 [details] [diff] [review]
Patch v5, now with fails-if(Android) for the right tests

Okay, so I misread which tests fail and added the annotations to the wrong lines.  Let me see if I got it right this time.  Same as the last patch, except I moved the fails-if(Android) from scale3d-xz.html (unexpected pass with last patch) to scale3d-all.html (unexpected fail with last patch).  I'll retry on just Android this time.
Comment 20 Mozilla RelEng Bot 2012-02-09 07:20:42 PST
Autoland Patchset:
	Patches: 595728
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/0aca14479a9e
Try run started, revision 0aca14479a9e. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=0aca14479a9e
Comment 21 Mozilla RelEng Bot 2012-02-09 14:30:23 PST
Try run for 0aca14479a9e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0aca14479a9e
Results (out of 41 total builds):
    exception: 1
    success: 36
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-0aca14479a9e
Comment 22 Matt Woodrow (:mattwoodrow) 2012-02-19 00:46:29 PST
Comment on attachment 595728 [details] [diff] [review]
Patch v5, now with fails-if(Android) for the right tests

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

Sorry for the slow review, forgot about this bug. Feel free to remind me in these situations :)
Comment 25 :Aryeh Gregor (away until August 15) 2012-02-21 06:25:29 PST
The reftests all passed on OS X in the try runs with results posted in comment 14 and comment 18.  The failure in comment 24 is quite noticeable in the reftest analyzer if you switch back and forth between the images, though.  Could it be that a parallel change introduced a discrepancy between how 2D and 3D transforms are rendered?

The failing reftest is not one I touched in this commit -- it's preserve3d-1a.html, which tests that four rotateX(45deg) with preserve-3d is the same as one rotateX(180deg).  With this change, I guess rotateX(180deg) gets text scaled as a 2D transform (since it's really the same as scaleY(-1)), while the four rotateX(45deg) don't get special treatment.  But rotateX(180deg) should really result in no text scaling anyway, right?  If it does, that's a bug.

What should I do here?  Perhaps I could modify the patch so that there's no scaling for things like rotateX(180deg) that don't change the text size?  Indeed, there shouldn't be.  I could add more tests for this.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-22 10:55:33 PST
(In reply to Aryeh Gregor from comment #25)
> The failing reftest is not one I touched in this commit -- it's
> preserve3d-1a.html, which tests that four rotateX(45deg) with preserve-3d is
> the same as one rotateX(180deg).

Can you just change this test so that instead of testing text, it tests a plain rectangle sized so that zooming doesn't matter?
Comment 27 :Aryeh Gregor (away until August 15) 2012-02-22 11:19:29 PST
Sure.  That masks the discrepancy, though.  Would it be better in that case to mark the existing test as random and add a new test, so that at least there's some record that the behavior is possibly undesirable?  Or does it not matter, since random tests' results are ignored by the test runners anyway and we don't care if this test is pixel-perfect anyway?

I'm not sure why this would be random anyway.  Let me try again.  If it fails again on OS X, maybe fails-if() would be better.  Or do we actively not want to test for this, because we don't care if it's precisely the same?
Comment 28 Mozilla RelEng Bot 2012-02-22 11:23:13 PST
Autoland Patchset:
	Patches: 595728
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=6cb7d94d4b47
Try run started, revision 6cb7d94d4b47. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=6cb7d94d4b47
Comment 29 Mozilla RelEng Bot 2012-02-22 16:30:59 PST
Try run for 6cb7d94d4b47 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6cb7d94d4b47
Results (out of 9 total builds):
    success: 3
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-6cb7d94d4b47
Comment 30 :Aryeh Gregor (away until August 15) 2012-02-23 10:07:04 PST
Created attachment 600072 [details] [diff] [review]
Patch v6, now with yet more reftest fidgeting!

Okay, so it seems like the OS X failures are consistent.  That test actually was originally disabled across the board, but I enabled it in bug 723680 except left it disabled for Windows 7.  This patch makes it not work on OS X either.  Maybe we should ditch the test entirely, but if it still passes on Windows < 7 and Linux and Android, I guess it might still be useful.  I added another test that uses a simple box as roc suggested in comment 26, which should test the basic idea but hopefully pass on all platforms.
Comment 31 Mozilla RelEng Bot 2012-02-23 10:11:08 PST
Autoland Patchset:
	Patches: 600072
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=2d9a5a015673
Try run started, revision 2d9a5a015673. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=2d9a5a015673
Comment 32 Mozilla RelEng Bot 2012-02-23 15:02:42 PST
Try run for 2d9a5a015673 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2d9a5a015673
Results (out of 37 total builds):
    success: 33
    warnings: 3
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-2d9a5a015673
Comment 34 Phil Ringnalda (:philor, back in August) 2012-02-26 16:26:19 PST
https://hg.mozilla.org/mozilla-central/rev/7dcff354aeae

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