Closed Bug 719173 Opened 12 years ago Closed 12 years ago

Specifying multiple transforms loses precision in the translate part

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: ayg, Assigned: froydnj)

Details

Attachments

(1 file, 2 obsolete files)

handling pixels and numbers directly in ProcessTranslatePart
3.41 KB, patch
froydnj
: review+
Details | Diff | Splinter Review 
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
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....
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.
The computed value is a list of transformations, not a single matrix.  Otherwise transitions of transforms would not work right, for example.
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.
Storing floats seems like a good idea. It would reduce float<->int conversions too.
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?
Attachment #603350 - Flags: feedback?(bzbarsky)
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....
Attachment #603350 - Flags: feedback?(bzbarsky) → feedback?(matt.woodrow)
(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?
<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
Ah, right.
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!
Attachment #603350 - Flags: feedback?(matt.woodrow) → feedback+
Here's a cleaned up version implementing Matt's suggestions and also ditching CalcLength.
Attachment #603350 - Attachment is obsolete: true
Attachment #604394 - Flags: review?(bzbarsky)
Attachment #604394 - Flags: feedback+
Whiteboard: [autoland-try:-b do -p all -u all -t none]
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
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.
Attachment #604394 - Flags: review?(bzbarsky) → review+
(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.
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.
Updated per bz's review.
Attachment #604394 - Attachment is obsolete: true
Attachment #646114 - Flags: review+
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
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/363045309cb0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: