Closed Bug 724614 Opened 13 years ago Closed 13 years ago

3D scaling makes text blurry

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: ayg, Assigned: ayg)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

Patch v6, now with yet more reftest fidgeting!
6.96 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
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.
> 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...
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?
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.
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.
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 reckon you could fix this in five minutes from here :-)
(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).
Attached patch Patch (obsolete) — — Splinter Review
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. :)
Attachment #595052 - Flags: review?(matt.woodrow)
Attached patch Patch with better-named tests (obsolete) — — Splinter Review
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.
Assignee: nobody → ayg
Attachment #595052 - Attachment is obsolete: true
Attachment #595052 - Flags: review?(matt.woodrow)
Attachment #595056 - Flags: review?(matt.woodrow)
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?
Attached patch Patch v3 (obsolete) — — Splinter Review
(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.
Attachment #595056 - Attachment is obsolete: true
Attachment #595056 - Flags: review?(matt.woodrow)
Attachment #595144 - Flags: review?(matt.woodrow)
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 :)
Attachment #595144 - Flags: review?(matt.woodrow) → review+
Whiteboard: [autoland]
Whiteboard: [autoland] → [autoland-in-queue]
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
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.
Whiteboard: [autoland-in-queue]
The changed reftests fail on Android, so this version disables them. (Or should I deal with the failure some other way?)
Attachment #595144 - Attachment is obsolete: true
Attachment #595421 - Flags: review?(matt.woodrow)
Whiteboard: [autoland]
Whiteboard: [autoland] → [autoland-in-queue]
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
It would be nice to know why they fail, but they look like antialiasing differences.
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
Whiteboard: [autoland-in-queue]
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.
Attachment #595421 - Attachment is obsolete: true
Attachment #595421 - Flags: review?(matt.woodrow)
Attachment #595728 - Flags: review?(matt.woodrow)
Whiteboard: [autoland:-p android,android-xul]
Whiteboard: [autoland:-p android,android-xul] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
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 :)
Attachment #595728 - Flags: review?(matt.woodrow) → review+
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.
(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?
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?
Whiteboard: [autoland:-p macosx,macosx64 -u reftest]
Whiteboard: [autoland:-p macosx,macosx64 -u reftest] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
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.
Attachment #595728 - Attachment is obsolete: true
Attachment #600072 - Flags: review?(matt.woodrow)
Whiteboard: [autoland:-u reftest]
Whiteboard: [autoland:-u reftest] → [autoland-in-queue]
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
Attachment #600072 - Flags: review?(matt.woodrow) → review+
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
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/mozilla-central/rev/7dcff354aeae
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 735373
Depends on: 747381
Depends on: 1098275
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: