From f22fef9e08a95b2c66399e14b7e6980fef497699 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 25 Jun 2024 07:56:29 -0700 Subject: [PATCH 1/3] [dart:ui] remove expensive index assertion in Vertices. --- lib/ui/painting.dart | 42 +++++++++++++++++++-------------- testing/dart/painting_test.dart | 16 ++++++++++++- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 1e02e5d410c51..dc3050f4f5ca4 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -4762,17 +4762,20 @@ base class Vertices extends NativeFieldWrapperClass1 { if (textureCoordinates != null && textureCoordinates.length != positions.length) { throw ArgumentError('"positions" and "textureCoordinates" lengths must match.'); } - if (indices != null) { - for (int index = 0; index < indices.length; index += 1) { - if (indices[index] >= positions.length) { - throw ArgumentError( - '"indices" values must be valid indices in the positions list ' - '(i.e. numbers in the range 0..${positions.length - 1}), ' - 'but indices[$index] is ${indices[index]}, which is too big.', - ); + assert(() { + if (indices != null) { + for (int index = 0; index < indices.length; index += 1) { + if (indices[index] >= positions.length) { + throw ArgumentError( + '"indices" values must be valid indices in the positions list ' + '(i.e. numbers in the range 0..${positions.length - 1}), ' + 'but indices[$index] is ${indices[index]}, which is too big.', + ); + } } } - } + return true; + }()); final Float32List encodedPositions = _encodePointList(positions); final Float32List? encodedTextureCoordinates = (textureCoordinates != null) ? _encodePointList(textureCoordinates) @@ -4849,17 +4852,20 @@ base class Vertices extends NativeFieldWrapperClass1 { if (textureCoordinates != null && textureCoordinates.length != positions.length) { throw ArgumentError('"positions" and "textureCoordinates" lengths must match.'); } - if (indices != null) { - for (int index = 0; index < indices.length; index += 1) { - if (indices[index] * 2 >= positions.length) { - throw ArgumentError( - '"indices" values must be valid indices in the positions list ' - '(i.e. numbers in the range 0..${positions.length ~/ 2 - 1}), ' - 'but indices[$index] is ${indices[index]}, which is too big.', - ); + assert(() { + if (indices != null) { + for (int index = 0; index < indices.length; index += 1) { + if (indices[index] * 2 >= positions.length) { + throw ArgumentError( + '"indices" values must be valid indices in the positions list ' + '(i.e. numbers in the range 0..${positions.length ~/ 2 - 1}), ' + 'but indices[$index] is ${indices[index]}, which is too big.', + ); + } } } - } + return true; + }()); if (!_init(this, mode.index, positions, textureCoordinates, colors, indices)) { throw ArgumentError('Invalid configuration for vertices.'); } diff --git a/testing/dart/painting_test.dart b/testing/dart/painting_test.dart index 87924853908f5..5c90cc6512107 100644 --- a/testing/dart/painting_test.dart +++ b/testing/dart/painting_test.dart @@ -34,6 +34,12 @@ void main() { }); test('Vertices.raw checks', () { + bool assertsEnabled = false; + assert(() { + assertsEnabled = true; + return true; + }()); + try { Vertices.raw( VertexMode.triangles, @@ -43,6 +49,8 @@ void main() { } on ArgumentError catch (e) { expect('$e', 'Invalid argument(s): "positions" must have an even number of entries (each coordinate is an x,y pair).'); } + + Object? indicesError; try { Vertices.raw( VertexMode.triangles, @@ -51,8 +59,14 @@ void main() { ); throw 'Vertices.raw did not throw the expected error.'; } on ArgumentError catch (e) { - expect('$e', 'Invalid argument(s): "indices" values must be valid indices in the positions list (i.e. numbers in the range 0..2), but indices[2] is 5, which is too big.'); + indicesError = e; } + if (assertsEnabled) { + expect('$indicesError', 'Invalid argument(s): "indices" values must be valid indices in the positions list (i.e. numbers in the range 0..2), but indices[2] is 5, which is too big.'); + } else { + expect(indicesError, null); + } + Vertices.raw( // This one does not throw. VertexMode.triangles, Float32List.fromList(const [0.0, 0.0]), From c62db1d5326a39567be4b376214a57e63651f206 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 5 Jul 2024 10:55:24 -0700 Subject: [PATCH 2/3] add test that uses invalid indicies for rendering and bounds computation. --- .../aiks_dl_vertices_unittests.cc | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/impeller/display_list/aiks_dl_vertices_unittests.cc b/impeller/display_list/aiks_dl_vertices_unittests.cc index c59a26a26e44d..85371a02eaec9 100644 --- a/impeller/display_list/aiks_dl_vertices_unittests.cc +++ b/impeller/display_list/aiks_dl_vertices_unittests.cc @@ -354,5 +354,29 @@ TEST_P(AiksTest, DrawVerticesPremultipliesColors) { ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } +TEST_P(AiksTest, DrawVerticesWithInvalidIndices) { + std::vector positions = { + SkPoint::Make(100, 300), SkPoint::Make(200, 100), SkPoint::Make(300, 300), + SkPoint::Make(200, 500)}; + std::vector indices = {0, 1, 2, 0, 2, 3, 99, 100, 101}; + + auto vertices = flutter::DlVertices::Make( + flutter::DlVertexMode::kTriangles, positions.size(), positions.data(), + /*texture_coordinates=*/nullptr, /*colors=*/nullptr, indices.size(), + indices.data()); + + EXPECT_EQ(vertices->bounds(), SkRect::MakeLTRB(100, 100, 300, 500)); + + flutter::DisplayListBuilder builder; + flutter::DlPaint paint; + paint.setBlendMode(flutter::DlBlendMode::kSrcOver); + paint.setColor(flutter::DlColor::kRed()); + + builder.DrawRect(SkRect::MakeLTRB(0, 0, 400, 400), paint); + builder.DrawVertices(vertices, flutter::DlBlendMode::kSrc, paint); + + ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); +} + } // namespace testing } // namespace impeller From fcb94b2ba6edf8326d665c1208ed8aa78ec25dea Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 5 Jul 2024 12:45:45 -0700 Subject: [PATCH 3/3] ++ --- testing/impeller_golden_tests_output.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index affa64d1f9e4a..e66a26af21609 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -590,6 +590,9 @@ impeller_Play_AiksTest_DrawVerticesSolidColorTrianglesWithIndices_Vulkan.png impeller_Play_AiksTest_DrawVerticesSolidColorTrianglesWithoutIndices_Metal.png impeller_Play_AiksTest_DrawVerticesSolidColorTrianglesWithoutIndices_OpenGLES.png impeller_Play_AiksTest_DrawVerticesSolidColorTrianglesWithoutIndices_Vulkan.png +impeller_Play_AiksTest_DrawVerticesWithInvalidIndices_Metal.png +impeller_Play_AiksTest_DrawVerticesWithInvalidIndices_OpenGLES.png +impeller_Play_AiksTest_DrawVerticesWithInvalidIndices_Vulkan.png impeller_Play_AiksTest_EmptySaveLayerIgnoresPaint_Metal.png impeller_Play_AiksTest_EmptySaveLayerIgnoresPaint_OpenGLES.png impeller_Play_AiksTest_EmptySaveLayerIgnoresPaint_Vulkan.png