Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[dart:ui] remove expensive index assertion in Vertices. #53558

Merged
merged 6 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions impeller/display_list/aiks_dl_vertices_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,30 @@ TEST_P(AiksTest, DrawVerticesPremultipliesColors) {
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

TEST_P(AiksTest, DrawVerticesWithInvalidIndices) {
std::vector<SkPoint> positions = {
SkPoint::Make(100, 300), SkPoint::Make(200, 100), SkPoint::Make(300, 300),
SkPoint::Make(200, 500)};
std::vector<uint16_t> 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()));
}

// All four vertices should form a solid red rectangle with no gaps.
// The blur rectangle drawn under them should not be visible.
TEST_P(AiksTest, DrawVerticesTextureCoordinatesWithFragmentShader) {
Expand Down
42 changes: 24 additions & 18 deletions lib/ui/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.');
}
Expand Down
16 changes: 15 additions & 1 deletion testing/dart/painting_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ void main() {
});

test('Vertices.raw checks', () {
bool assertsEnabled = false;
assert(() {
assertsEnabled = true;
return true;
}());

try {
Vertices.raw(
VertexMode.triangles,
Expand All @@ -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,
Expand All @@ -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 <double>[0.0, 0.0]),
Expand Down
3 changes: 3 additions & 0 deletions testing/impeller_golden_tests_output.txt
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,9 @@ impeller_Play_AiksTest_DrawVerticesSolidColorTrianglesWithoutIndices_Vulkan.png
impeller_Play_AiksTest_DrawVerticesTextureCoordinatesWithFragmentShader_Metal.png
impeller_Play_AiksTest_DrawVerticesTextureCoordinatesWithFragmentShader_OpenGLES.png
impeller_Play_AiksTest_DrawVerticesTextureCoordinatesWithFragmentShader_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
Expand Down