Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add f64 version of transform.to_transform_3d_matrix #23622

Closed
saschanaz opened this issue Jun 24, 2019 · 6 comments
Closed

Add f64 version of transform.to_transform_3d_matrix #23622

saschanaz opened this issue Jun 24, 2019 · 6 comments
Labels

Comments

@saschanaz
Copy link
Contributor

@saschanaz saschanaz commented Jun 24, 2019

DOMMatrix on Firefox 69 now uses double, but when parsing CSS through new DOMMatrix("matrix(...)") it depends on transform.to_transform_3d_matrix() and then loses precision. (And thus failing relevant WPT tests.)

https://github.com/mozilla/gecko-dev/blob/da14c413ef663eb1ba246799e94a240f81c42488/dom/base/DOMMatrix.cpp#L778

https://github.com/mozilla/gecko-dev/blob/a7a4c4c9df4b7b96c8d35b9f642c8b00a041092f/layout/style/ServoCSSParser.cpp#L55

https://github.com/mozilla/gecko-dev/blob/ea4843bd849cef3ecf281f8826626eea42550bda/servo/ports/geckolib/glue.rs#L6329

impl<T: ToMatrix> Transform<T> {
/// Return the equivalent 3d matrix of this transform list.
/// We return a pair: the first one is the transform matrix, and the second one
/// indicates if there is any 3d transform function in this transform list.
#[cfg_attr(rustfmt, rustfmt_skip)]
pub fn to_transform_3d_matrix(
&self,
reference_box: Option<&Rect<Au>>
) -> Result<(Transform3D<CSSFloat>, bool), ()> {
let cast_3d_transform = |m: Transform3D<f64>| -> Transform3D<CSSFloat> {
use std::{f32, f64};
let cast = |v: f64| { v.min(f32::MAX as f64).max(f32::MIN as f64) as f32 };
Transform3D::row_major(
cast(m.m11), cast(m.m12), cast(m.m13), cast(m.m14),
cast(m.m21), cast(m.m22), cast(m.m23), cast(m.m24),
cast(m.m31), cast(m.m32), cast(m.m33), cast(m.m34),
cast(m.m41), cast(m.m42), cast(m.m43), cast(m.m44),
)
};
// We intentionally use Transform3D<f64> during computation to avoid error propagation
// because using f32 to compute triangle functions (e.g. in create_rotation()) is not
// accurate enough. In Gecko, we also use "double" to compute the triangle functions.
// Therefore, let's use Transform3D<f64> during matrix computation and cast it into f32
// in the end.
let mut transform = Transform3D::<f64>::identity();
let mut contain_3d = false;
for operation in &*self.0 {
let matrix = operation.to_3d_matrix(reference_box)?;
contain_3d |= operation.is_3d();
transform = transform.pre_mul(&matrix);
}
Ok((cast_3d_transform(transform), contain_3d))
}
}

Could we add a f64 version of the API so that we won't lose precision?

Example:

new DOMMatrix("matrix(0.3333333333333333, 0, 0, 0, 0, 0)").m11
// expected: 0.3333333333333333
// actual: 0.3333333432674408

Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1560705

@jdm
Copy link
Member

@jdm jdm commented Jun 24, 2019

@emilio Any opinion on this question?

@jdm jdm added the A-stylo label Jun 24, 2019
@highfive
Copy link

@highfive highfive commented Jun 24, 2019

cc @emilio

@emilio
Copy link
Member

@emilio emilio commented Jun 24, 2019

@saschanaz fwiw you can edit that code directly in Firefox, I'd file a bug in the "CSS Parsing and Computation" component. I'm happy to help out or write the patch.

@emilio
Copy link
Member

@emilio emilio commented Jun 24, 2019

That being said we parse matrix as a matrix of CSSFloats which is a f32 under the hood, so I'm not sure how fixing just that function would help.

@saschanaz
Copy link
Contributor Author

@saschanaz saschanaz commented Jun 24, 2019

we parse matrix as a matrix of CSSFloats which is a f32 under the hood

In that case the parser should support f64 first... So this Number should be modified to use f64, right?

/// A CSS `<number>` specified value.
///
/// https://drafts.csswg.org/css-values-3/#number-value
#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, PartialOrd, ToShmem)]
pub struct Number {
/// The numeric value itself.
value: CSSFloat,
/// If this number came from a calc() expression, this tells how clamping
/// should be done on the value.
calc_clamping_mode: Option<AllowedNumericType>,
}

Looks like it will cause a large downstream changes.

@emilio
Copy link
Member

@emilio emilio commented Jun 24, 2019

Presumably, and yes, not only that but also will double memory usage in most places. We store a lot of these...

@saschanaz saschanaz mentioned this issue Jun 30, 2019
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jul 1, 2019
Support DOMMatrix string constructor

<!-- Please describe your changes on the following line: -->

Implemented DOMMatrix string constructor per [the spec](https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-dommatrixreadonly).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23664, fix #23622

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23665)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 1, 2019
Support DOMMatrix string constructor

<!-- Please describe your changes on the following line: -->

Implemented DOMMatrix string constructor per [the spec](https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-dommatrixreadonly).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #23664, fix #23622

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23665)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.