Precision of GeometryMapper::SourceToDestinationProjection() |
|||
Issue description
For now GeometryMapper::SourceToDetinationProjection() uses the following algorithm to calculate the projection between two transforms:
source_from_plane_root_or_screen * destination_to_plane_root_or_screen
In the following example:
For example,
t0
|
t1: Scale(32767)
|
t2: Rotate(1)
|
t3: Identity
|
t4: Identity
When we GoemetryMapper::SourceToDestinationProjection(t4, t3), we don't just get Inverse(t4) which is identity, but get
(Inverse(t4) * Inverse(t3) * Inverse(t2) * Inverse(t1))
* (t1 * t2 * t3)
which contains accumulated errors when calculating inversions and multiplications of t1 and t2.
The error defeats some optimizations for 2d translations unless we tolerate the errors.
Based on that 2d translations (for scroll translation, paint offset translation, compositing hint, isolation, etc.) are very common in our transform tree, I'm thinking of a simple solution: to introduce root_of_2d_translation and to_2d_translation_root in GeometryMapperTransformCache, and add the following code in GeometryMapper::SourceToDestinationProjectionInternal():
if (source_cache.root_of_2d_translation() == destination_cache.root_of_2d_translation()) {
success = true;
if (destination == source_cache.root_of_2d_translation())
return source_cache.to_2d_translation_root();
temp = destination_cache.to_2d_translation_root().Inverse();
if (source != destination.root_of_2d_translation())
temp.Multiply(source_cache.to_2d_translation_root());
return temp;
}
As 2d translation is the most common transform type in our transform tree, it's common that root_of_2d_translation is the same as plane_root and screen_root, and we can just save one copy of the accumulated transform in common cases, and allocate memory for accumulated transforms for plane and screen only if they are different.
Wdyt?
,
Oct 3
,
Oct 3
I created an experimental CL https://chromium-review.googlesource.com/c/chromium/src/+/1259663. Running perf tests with it.
,
Oct 3
Yup - sounds like exactly what I was seeing. I still need to setup the case in a test to confirm and I haven't had time but bug 888202 was basically that adding a viewport scale transform (from pinch-zoom) would cause ChunkToLayerMapper::MapVisualRect to miss the Translate2d optimization in the matrix transform due to an "off by epsilon" error.
,
Oct 3
bokan@ can you check if https://chromium-review.googlesource.com/c/chromium/src/+/1259663 can fix bug 888202 ?
,
Oct 4
My experiment CL works in my unit test.
,
Oct 4
Will do - I'm flying today though and am out tomorrow so might have to wait until Monday.
,
Oct 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b767df9fc88b55eee8c3cffe6245101bd1248e0 commit 4b767df9fc88b55eee8c3cffe6245101bd1248e0 Author: Xianzhu Wang <wangxianzhu@chromium.org> Date: Thu Oct 04 23:09:22 2018 [PE] Improve precision of GeometryMapper for 2d translations 2d translations (e.g. for paint offset translation, scroll translation) are very common in our paint property trees. In many cases GeometryMapper::SourceToDestinationProjection() deals with source and destination transforms under a common subtree that contains 2d translations only, and we expect that the result to be a 2d translation. However, previously, in the case we used source_from_plane_root_or_screen * destination_to_plane_root_or_screen which includes transforms and inverse of transforms of some common ancestors of the input transforms, and contains accumulated floating point errors when calculating the inverses and multiplications of the common ancestors. The new test case in this CL shows the problem. Now let GeometryMapperTransformCache cache the 2d translation root of the transform node. In GeometryMapper::SourceToDestinationProjection(), if the input transforms are under the same 2d translation root, we can use the fast path to calculate the result which will be a 2d translation. Also reduce the size of the GeometryMapperTransformCache in common cases by allocating plane root transform and screen transform only when needed. Bug: 891799 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I926b337469d0c8d79ee6a63f7c0e2f15781266d2 Reviewed-on: https://chromium-review.googlesource.com/c/1259663 Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#596908} [modify] https://crrev.com/4b767df9fc88b55eee8c3cffe6245101bd1248e0/third_party/blink/renderer/platform/BUILD.gn [modify] https://crrev.com/4b767df9fc88b55eee8c3cffe6245101bd1248e0/third_party/blink/renderer/platform/graphics/paint/geometry_mapper.cc [modify] https://crrev.com/4b767df9fc88b55eee8c3cffe6245101bd1248e0/third_party/blink/renderer/platform/graphics/paint/geometry_mapper_test.cc [modify] https://crrev.com/4b767df9fc88b55eee8c3cffe6245101bd1248e0/third_party/blink/renderer/platform/graphics/paint/geometry_mapper_transform_cache.cc [modify] https://crrev.com/4b767df9fc88b55eee8c3cffe6245101bd1248e0/third_party/blink/renderer/platform/graphics/paint/geometry_mapper_transform_cache.h [add] https://crrev.com/4b767df9fc88b55eee8c3cffe6245101bd1248e0/third_party/blink/renderer/platform/graphics/paint/geometry_mapper_transform_cache_test.cc [modify] https://crrev.com/4b767df9fc88b55eee8c3cffe6245101bd1248e0/third_party/blink/renderer/platform/graphics/paint/transform_paint_property_node.h
,
Oct 8
|
|||
►
Sign in to add a comment |
|||
Comment 1 by schenney@chromium.org
, Oct 3Status: Assigned (was: Untriaged)