Avoid implicit conversions to floats in dart:ui #40098
Conversation
Dart has a built in `double` type. We currently sometimes let
Dart's FFI mechanisms automatically truncate this to float (like in
the path API), and sometimes implicitly convert Dart doubles to
C++ floats (aka SkScalar and impeller:Scalar). We also do implicit
conversions when dealing with `Float64List` data, especially w.r.t.
matrices. This causes problems when applications use very large
double values which then get silently truncated to infinity, especially
in matrices and values that end up being used in matrix concatenation.
This is solved by the following.
- Enable `-Wimplicit-float-conversion` for dart:ui's C++
sources.
- Add a bunch of `f`s to C++ literals.
- Create a utility method for truncating doubles to floats
in a well defined manner, avoiding truncation from a
finite double value to an infinite float value.
- Delete Tonic's converters from double to float, since
these silently truncate to infinity before the engine C++ code
even touches them.
Fixes flutter/flutter#121299
lib/ui/floating_point.h
Outdated
| return std::max( | ||
| std::numeric_limits<float>::lowest(), | ||
| std::min(std::numeric_limits<float>::max(), static_cast<float>(value))); |
There was a problem hiding this comment.
I think std::clamp can be used here for potentially faster code and easier to maintain.
|
In some offline conversation with @flar: We may want to push the narrowing down til after DL has done its thing (and after the layer tree code has done its thing) to avoid premature precision loss. But that only really makes sense if we can get those types to use doubles as well, particularly around their matrix code (which today uses SkMatrix/SkM44, which are both float based). |
|
(I'm going to hold off on updating merge conflicts until I get more feedback) |
|
Neither FLT_MAX or INFINITY is what the user expects. Using one or the other hardly matters. An assertion ought to be added earlier (closer to the user) where this sort of error may occur. I am not sure if this is an improvement. |
Part of how I got to this point was trying to add that assertion. But what does it tell the user to do? Clamp to FLT_MAX? Something else entirely? |
|
From the latest conversation in triage, I think a case can be made that this is less surprising than the current state of things. I'd really like the application author to be in control of the precision and how its clamped but I don't think that is feasible even in the medium to long term. I think we should cleanup this patch and land this. |
dnfield
added
the
autosubmit
…gine#40098) (#123433) Roll Flutter Engine from 608d5b214faa to 6c2dafed5077 (1 revision)
…lutter/engine#40098) (#123433) Commit: da5dc9dbed7e40bcb9b99b2c880c2483cf748e8a
|
FYI, it looks like this may have introduced tiny differences in the AA pixel values -- check out the cluster around coordinates I approved the diff since it was not perceptible. |
|
Note that if this did indeed cause those small differences, we're likely to see a bunch of Scuba failures when this rolls into g3 as well. |
Avoid implicit conversions to floats in dart:ui
|
Frob filed a bug against this PR that said it caused some minor scuba differences in google3. |
Dart has a built in
doubletype. We currently sometimes let Dart's FFI mechanisms automatically truncate this to float (like in the path API), and sometimes implicitly convert Dart doubles to C++ floats (aka SkScalar and impeller:Scalar). We also do implicit conversions when dealing withFloat64Listdata, especially w.r.t. matrices. This causes problems when applications use very large double values which then get silently truncated to infinity, especially in matrices and values that end up being used in matrix concatenation.This is solved by the following.
-Wimplicit-float-conversionfor dart:ui's C++ sources.fs to C++ literals.Fixes flutter/flutter#121299