[dart:ui] remove expensive index assertion in Vertices. #53558
Conversation
|
Where are we expecting this validation to occur then? At some point we have to protect against reading past the ends of the data arrays. |
|
We don't read the indices though, we pass it to the gpu driver. I suppose we should validate that reasonable things happen there. |
|
verifying on GLES/Vulkan/Metal backend, no crashes. We don't ever read the index data on the CPU |
|
We should add a test case that using out of range index values works fine if that is the expected behavior. We may not currently read them on the CPU, but might in the future. One example is the computation of the bounds of the vertices. Currently their bounds are the bounds of all vertices, but if we have 100k vertices and only 100 indices then there is wasted effort there - so we might in the future switch to using the indices for bounds (possibly with a quick test as to whether there are more vertices than indices or vice versa) - at which point we want to make sure that there is a test case that would remind us that OOB indices are specifically allowed... |
|
I can add a test at the DL-impeller level and produces a golden. SG? |
|
For rendering - as long as it goes through DlBuilder. ui.Canvas is basically a pass through, but starting at Builder we have increasing reasons to inspect the data. Another thing to verify is constructing the Dart Vertices object itself - the constructor might decide to prune the data at some point if indices is small? (Probably not....? But good to have a test that makes sure it doesn't trip over a bad index just in case) |
…/engine into remove_expensive_assertion
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
|
PTAL @flar |
|
Golden file changes are available for triage from new commit, Click here to view. |
|
Jim is out this week. maybe @jtmcdole PTAL? |
|
@jonahwilliams Can we land this? |
|
@chinmaygarde did you mean to close this? We should be able to land it, didn't realize it got reviewed. |
…151674) flutter/engine@36dccf7...6b36113 2024-07-12 [email protected] Roll Skia from 9ea5603242c1 to 923034db7728 (1 revision) (flutter/engine#53841) 2024-07-12 [email protected] Roll Dart SDK from 797d3df745d1 to e986ed9d0bc1 (1 revision) (flutter/engine#53840) 2024-07-12 [email protected] Roll Skia from 7a91f0a4b7a0 to 9ea5603242c1 (1 revision) (flutter/engine#53839) 2024-07-12 [email protected] Roll Skia from 9529b8ad9e45 to 7a91f0a4b7a0 (2 revisions) (flutter/engine#53838) 2024-07-12 [email protected] Roll Skia from 38f355af4f36 to 9529b8ad9e45 (1 revision) (flutter/engine#53837) 2024-07-12 [email protected] Roll Skia from 14c8d318615d to 38f355af4f36 (1 revision) (flutter/engine#53836) 2024-07-12 [email protected] Roll Skia from ddf045505cb9 to 14c8d318615d (1 revision) (flutter/engine#53835) 2024-07-12 [email protected] Roll Fuchsia Linux SDK from 0e47sje8wkJ08sGJ6... to VlZIUknh6dnA23owe... (flutter/engine#53834) 2024-07-12 [email protected] Manual roll Dart SDK from fb546f313557 to 797d3df745d1 (8 revisions) (flutter/engine#53832) 2024-07-11 [email protected] [Impeller] Ensure full transform is applied to text contents (flutter/engine#53819) 2024-07-11 [email protected] Roll ICU from 43953f57b037 to 9408c6fd4a39 (6 revisions) (flutter/engine#53827) 2024-07-11 [email protected] Update Life-of-a-Flutter-Frame.md (flutter/engine#53829) 2024-07-11 [email protected] Update Setting-up-the-Engine-development-environment.md (flutter/engine#53828) 2024-07-11 [email protected] [web] retrieve hostElement for an implicit view (flutter/engine#53296) 2024-07-11 [email protected] [dart:ui] remove expensive index assertion in Vertices. (flutter/engine#53558) 2024-07-11 [email protected] [Impeller] Enable fixed-rate compression support in Vulkan. (flutter/engine#53292) 2024-07-11 [email protected] Roll Skia from 037d5f8a727f to ddf045505cb9 (1 revision) (flutter/engine#53824) 2024-07-11 [email protected] Add instructions for source debugging with Xcode when using RBE. (flutter/engine#53822) 2024-07-11 [email protected] Roll Skia from 004c81523e44 to 037d5f8a727f (1 revision) (flutter/engine#53818) 2024-07-11 [email protected] [Impeller] move more aiks tests to DL. (flutter/engine#53792) 2024-07-11 [email protected] Roll Skia from ec4a1e03f7b0 to 004c81523e44 (23 revisions) (flutter/engine#53813) 2024-07-11 [email protected] Roll Skia from 2783ba54bf8e to ec4a1e03f7b0 (9 revisions) (flutter/engine#53797) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 0e47sje8wkJ0 to VlZIUknh6dnA If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Iterating through every index value is really, really expensive. Especially if users are suppllying hundreds of thousands of vertices.