Last Comment Bug 719173 - Specifying multiple transforms loses precision in the translate part
: Specifying multiple transforms loses precision in the translate part
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Linux
: -- minor (vote)
: mozilla17
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-18 11:55 PST by :Aryeh Gregor (away until August 15)
Modified: 2012-07-30 10:13 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
handling pixels and numbers directly in ProcessTranslatePart (1.09 KB, patch)
2012-03-06 10:46 PST, Nathan Froyd [:froydnj]
matt.woodrow: feedback+
Details | Diff | Review
handling pixels and numbers directly in ProcessTranslatePart (3.41 KB, patch)
2012-03-09 06:31 PST, Nathan Froyd [:froydnj]
bzbarsky: review+
nfroyd: feedback+
Details | Diff | Review
handling pixels and numbers directly in ProcessTranslatePart (3.41 KB, patch)
2012-07-26 06:30 PDT, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-01-18 11:55:28 PST
Test-case:

data:text/html,<!doctype html>
<div style="-moz-transform:
translateX(-10px)
scaleX(1000)
translateX(0.01px)
scaleX(0.001)
"></div>
<script>
document.body.textContent =
getComputedStyle(document.querySelector("div"))
.MozTransform
</script>

Mathematically, this should be the identity matrix.  IE9 is "matrix(1, 0, 0, 1, -2.23517e-007, 0)", and Chrome 17 dev is "matrix(1, 0, 0, 1, -0.00000022351741790771484, 0)", both of which are reasonable.  Firefox 12.0a1 (2012-01-17), however, is "matrix(1, 0, 0, 1, 6.66667px, 0px)", which is over six pixels off.  (Opera Next 12.00 alpha is even worse: "matrix(1, 0, 0, 1, -10, 0)".)

It's reasonable if there are errors that are small fractions of a pixel at the end of the day, but they shouldn't accumulate like this.  I hit this sort of rounding error while fuzz-testing with composing various random-ish matrices; I suspect it won't be uncommon for it to add up to a few pixels in real pages, if people compose a reasonably long chain of matrices.  Can we use higher precision for intermediate computations?

This is causing Gecko test failures in my CSS transform tests, where I set a tolerance of 0.05px for getComputedStyle() and Gecko slips above that in some cases: http://hg.csswg.org/test/file/e617571dec78/contributors/aryehgregor/incoming
Comment 1 Boris Zbarsky [:bz] 2012-01-18 17:08:06 PST
We're presumably storing the lengths in nscoord units, which are accurate to 1/60px.  So we can represent 0.016666px and we can represent 0px, but nothing in between.  Hence the observed 6.66667px of error, after multiplying by 1000.

This isn't a matter of intermediate computations; the actual computed value storage is almost certainly in nscoord.

I agree that for the transform case we should perhaps reconsider this....
Comment 2 :Aryeh Gregor (away until August 15) 2012-01-19 08:23:27 PST
If you compute "translateX(-10px) scaleX(1000) translateX(0.01px) scaleX(0.001)" exactly, it works out to the identity matrix.  So it's not bad for this purpose if the computed value is stored imprecisely -- the identity matrix can be stored exactly!  But the result doesn't work out to the identity in Gecko in the first place, which appears to be because the transformations are converted to imprecise matrices *before* they're composed, rather than after.

I'm not objecting if the computed matrix is *also* stored more precisely, though.
Comment 3 Boris Zbarsky [:bz] 2012-01-19 09:40:19 PST
The computed value is a list of transformations, not a single matrix.  Otherwise transitions of transforms would not work right, for example.
Comment 4 :Aryeh Gregor (away until August 15) 2012-01-19 10:38:02 PST
Hmm, right.  So it's only the *resolved* value that's a single matrix.  It's confusing that authors don't have direct access to computed values, and the way of getting resolved values is called getComputedStyle().

In that case, yes, I think it makes sense to store the computed values here with more precision.  As it stands, it's easy to write reasonable-looking transforms that behave completely differently in Gecko vs. IE/WebKit due to rounding errors.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-19 12:41:28 PST
Storing floats seems like a good idea. It would reduce float<->int conversions too.
Comment 6 Nathan Froyd [:froydnj] 2012-03-06 10:46:17 PST
Created attachment 603350 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Simply handling pixels and numbers directly in ProcessTranslatePart so that we don't effectively do float((nscoord)(pixel * aAppUnitsPerPixel))/aAppUnitsPerPixel is enough to give us the same answers as Chrome here.

I assume just using GetFloatValue directly isn't the Way Things Are Done, though; should there be some sort of wrapper in nsPresContext for this?
Comment 7 Boris Zbarsky [:bz] 2012-03-06 11:51:11 PST
Comment on attachment 603350 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Oh, so this _was_ a case of intermediate computations, sort of.

Is aAppUnitsPerMatrixUnit guaranteed to be the same as application-wide number of app units in a CSS px?  Worth checking with someone who really knows this code...  In either case, the whole "percent" thing there is pointless, right?  And if you do this then CalcLength in that file can go away.

That said, this will also give differences in behavior when translate() is used vs when, say, relative positioning is used.  That may be ok....
Comment 8 :Aryeh Gregor (away until August 15) 2012-03-06 16:38:10 PST
(In reply to Boris Zbarsky (:bz) from comment #7)
> That said, this will also give differences in behavior when translate() is
> used vs when, say, relative positioning is used.  That may be ok....

What's an example?
Comment 9 Boris Zbarsky [:bz] 2012-03-06 18:18:14 PST
<div style="position: relative; left: -10px">
  <div style="-moz-transform: scaleX(1000)">
    <div style="position: relative; left: 0.01px;">
      <div style="-moz-transform: scaleX(0.001)">

or so
Comment 10 :Aryeh Gregor (away until August 15) 2012-03-07 11:06:51 PST
Ah, right.
Comment 11 Matt Woodrow (:mattwoodrow) 2012-03-08 20:50:08 PST
Comment on attachment 603350 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

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

Unfortunately aAppUnitsPerMatrixUnit isn't always fixed, it's affected by zooming. Though the callers of ReadTransforms seem really inconsistent here, the a lot are fixed to nsPresContext::AppUnitsPerCSSPixel(), others are checking AppUnitsPerDevPixel(), this might be worth checking as a follow-up.

For this I think we can just multiply by the ratio between aAppUnitsPerMatrixUnit and nsPresContext::AppUnitsPerCSSPixel().

As bz suggested, the percent piece here will always be 0.

Idea looks good though!
Comment 12 Nathan Froyd [:froydnj] 2012-03-09 06:31:19 PST
Created attachment 604394 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Here's a cleaned up version implementing Matt's suggestions and also ditching CalcLength.
Comment 13 Mozilla RelEng Bot 2012-03-09 06:35:52 PST
Autoland Patchset:
	Patches: 604394
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=fe93ca799c91
Try run started, revision fe93ca799c91. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=fe93ca799c91
Comment 14 Mozilla RelEng Bot 2012-03-09 10:46:10 PST
Try run for fe93ca799c91 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fe93ca799c91
Results (out of 216 total builds):
    exception: 7
    success: 168
    warnings: 40
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-fe93ca799c91
Comment 15 Boris Zbarsky [:bz] 2012-03-15 10:04:03 PDT
Comment on attachment 604394 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

>+++ b/layout/style/nsStyleTransformMatrix.cpp
>+    return aValue.GetFloatValue() *
>+           (aAppUnitsPerMatrixUnit / float(nsPresContext::AppUnitsPerCSSPixel()));

I believe the division is backwards.  The old code ended up doing:

  NSAppUnitsToFloatPixels(
    nsPresContext::CSSPixelsToAppUnits(aValue.GetFloatValue()),
    aAppUnitsPerMatrixUnit
  )

That corresponds to multiplying the result of GetFloatValue() by
nsPresContext::AppUnitsPerCSSPixel and then dividing by aAppUnitsPerMatrixUnit,
no?

r=me with that fixed and ideally a test added that would have caused the backwards division.
Comment 16 Nathan Froyd [:froydnj] 2012-03-15 10:19:04 PDT
(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 604394 [details] [diff] [review]
> handling pixels and numbers directly in ProcessTranslatePart
> 
> >+++ b/layout/style/nsStyleTransformMatrix.cpp
> >+    return aValue.GetFloatValue() *
> >+           (aAppUnitsPerMatrixUnit / float(nsPresContext::AppUnitsPerCSSPixel()));
> 
> I believe the division is backwards.

I may have misunderstood Matt's explanation; I also failed to actually think about what the code was doing. :)

I will try to put together a testcase.
Comment 17 Nathan Froyd [:froydnj] 2012-05-07 13:47:41 PDT
I haven't been able to generate testcases, so was going to check in the patch.  However, this try run:

https://tbpl.mozilla.org/?tree=Try&rev=292acc29933e

showed reftest failures with the patch modified as per bz's review in comment 15.  I haven't investigated, though I do wonder if there's some subtle rounding behavior at work here.
Comment 18 Nathan Froyd [:froydnj] 2012-07-26 06:30:00 PDT
Created attachment 646114 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Updated per bz's review.
Comment 19 Nathan Froyd [:froydnj] 2012-07-26 06:32:22 PDT
Another try run showed all green for reftests:

https://tbpl.mozilla.org/?tree=Try&rev=b07993954399

so I'm not sure what was wrong with the run in comment 17.  Maybe an intermittent orange?

Checked in:

http://hg.mozilla.org/integration/mozilla-inbound/rev/363045309cb0
Comment 20 Matt Brubeck (:mbrubeck) 2012-07-26 14:08:10 PDT
https://hg.mozilla.org/mozilla-central/rev/363045309cb0

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