From 25054fb47016409853f7275d39b58cea77faf7ab Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 3 Jun 2020 10:35:14 -0700 Subject: [PATCH] Implement GetAllocationSize for Vertices (#18756) * Implement GetAllocationSize for Vertices * Reflect vertex buffer size in pictures --- ci/licenses_golden/licenses_flutter | 1 + lib/ui/BUILD.gn | 2 + lib/ui/fixtures/ui_test.dart | 29 +++++++++++++ lib/ui/painting.dart | 11 ++--- lib/ui/painting/canvas.cc | 12 +++--- lib/ui/painting/canvas.h | 4 +- lib/ui/painting/picture.cc | 12 +++--- lib/ui/painting/picture.h | 8 ++-- lib/ui/painting/picture_recorder.cc | 2 +- lib/ui/painting/vertices.cc | 24 +++++------ lib/ui/painting/vertices.h | 15 ++++--- lib/ui/painting/vertices_unittests.cc | 61 +++++++++++++++++++++++++++ shell/common/BUILD.gn | 59 +++++++++++++++++--------- testing/dart/canvas_test.dart | 41 ++++++++++++++++++ 14 files changed, 215 insertions(+), 66 deletions(-) create mode 100644 lib/ui/painting/vertices_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index d45a03cf18..ae49238407 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -354,6 +354,7 @@ FILE: ../../../flutter/lib/ui/painting/single_frame_codec.cc FILE: ../../../flutter/lib/ui/painting/single_frame_codec.h FILE: ../../../flutter/lib/ui/painting/vertices.cc FILE: ../../../flutter/lib/ui/painting/vertices.h +FILE: ../../../flutter/lib/ui/painting/vertices_unittests.cc FILE: ../../../flutter/lib/ui/plugins.dart FILE: ../../../flutter/lib/ui/plugins/callback_cache.cc FILE: ../../../flutter/lib/ui/plugins/callback_cache.h diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index a581708f03..f1100c3f66 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -164,6 +164,7 @@ if (current_toolchain == host_toolchain) { "painting/image_decoder_test.cc", "painting/image_decoder_test.h", "painting/image_decoder_unittests.cc", + "painting/vertices_unittests.cc", "window/pointer_data_packet_converter_unittests.cc", ] @@ -171,6 +172,7 @@ if (current_toolchain == host_toolchain) { ":ui", ":ui_unittests_fixtures", "//flutter/common", + "//flutter/shell/common:shell_test_fixture_sources", "//flutter/testing", "//flutter/testing:dart", "//flutter/testing:opengl", diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index dfd68d7416..852edd6dc9 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -3,10 +3,39 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:typed_data'; import 'dart:ui'; void main() {} +@pragma('vm:entry-point') +void createVertices() { + const int uint16max = 65535; + + final Int32List colors = Int32List(uint16max); + final Float32List coords = Float32List(uint16max * 2); + final Uint16List indices = Uint16List(uint16max); + final Float32List positions = Float32List(uint16max * 2); + colors[0] = const Color(0xFFFF0000).value; + colors[1] = const Color(0xFF00FF00).value; + colors[2] = const Color(0xFF0000FF).value; + colors[3] = const Color(0xFF00FFFF).value; + indices[1] = indices[3] = 1; + indices[2] = indices[5] = 3; + indices[4] = 2; + positions[2] = positions[4] = positions[5] = positions[7] = 250.0; + + final Vertices vertices = Vertices.raw( + VertexMode.triangles, + positions, + textureCoordinates: coords, + colors: colors, + indices: indices, + ); + _validateVertices(vertices); +} +void _validateVertices(Vertices vertices) native 'ValidateVertices'; + @pragma('vm:entry-point') void frameCallback(FrameInfo info) { print('called back'); diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index dbcbc7d01d..b56856b40e 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -3249,8 +3249,7 @@ class Vertices extends NativeFieldWrapperClass2 { ? Uint16List.fromList(indices) : null; - _constructor(); - if (!_init(mode.index, encodedPositions, encodedTextureCoordinates, encodedColors, encodedIndices)) + if (!_init(this, mode.index, encodedPositions, encodedTextureCoordinates, encodedColors, encodedIndices)) throw ArgumentError('Invalid configuration for vertices.'); } @@ -3287,14 +3286,12 @@ class Vertices extends NativeFieldWrapperClass2 { if (indices != null && indices.any((int i) => i < 0 || i >= positions.length)) throw ArgumentError('"indices" values must be valid indices in the positions list.'); - _constructor(); - if (!_init(mode.index, positions, textureCoordinates, colors, indices)) + if (!_init(this, mode.index, positions, textureCoordinates, colors, indices)) throw ArgumentError('Invalid configuration for vertices.'); } - void _constructor() native 'Vertices_constructor'; - - bool _init(int mode, + bool _init(Vertices outVertices, + int mode, Float32List positions, Float32List textureCoordinates, Int32List colors, diff --git a/lib/ui/painting/canvas.cc b/lib/ui/painting/canvas.cc index 2ee718cd2f..367518b901 100644 --- a/lib/ui/painting/canvas.cc +++ b/lib/ui/painting/canvas.cc @@ -293,7 +293,7 @@ void Canvas::drawImage(const CanvasImage* image, if (!image) Dart_ThrowException( ToDart("Canvas.drawImage called with non-genuine Image.")); - image_allocation_size_ += image->GetAllocationSize(); + external_allocation_size_ += image->GetAllocationSize(); canvas_->drawImage(image->image(), x, y, paint.paint()); } @@ -315,7 +315,7 @@ void Canvas::drawImageRect(const CanvasImage* image, ToDart("Canvas.drawImageRect called with non-genuine Image.")); SkRect src = SkRect::MakeLTRB(src_left, src_top, src_right, src_bottom); SkRect dst = SkRect::MakeLTRB(dst_left, dst_top, dst_right, dst_bottom); - image_allocation_size_ += image->GetAllocationSize(); + external_allocation_size_ += image->GetAllocationSize(); canvas_->drawImageRect(image->image(), src, dst, paint.paint(), SkCanvas::kFast_SrcRectConstraint); } @@ -341,7 +341,7 @@ void Canvas::drawImageNine(const CanvasImage* image, SkIRect icenter; center.round(&icenter); SkRect dst = SkRect::MakeLTRB(dst_left, dst_top, dst_right, dst_bottom); - image_allocation_size_ += image->GetAllocationSize(); + external_allocation_size_ += image->GetAllocationSize(); canvas_->drawImageNine(image->image(), icenter, dst, paint.paint()); } @@ -351,7 +351,7 @@ void Canvas::drawPicture(Picture* picture) { if (!picture) Dart_ThrowException( ToDart("Canvas.drawPicture called with non-genuine Picture.")); - image_allocation_size_ += picture->GetAllocationSize(); + external_allocation_size_ += picture->GetAllocationSize(); canvas_->drawPicture(picture->picture().get()); } @@ -380,7 +380,7 @@ void Canvas::drawVertices(const Vertices* vertices, if (!vertices) Dart_ThrowException( ToDart("Canvas.drawVertices called with non-genuine Vertices.")); - + external_allocation_size_ += vertices->GetAllocationSize(); canvas_->drawVertices(vertices->vertices(), blend_mode, *paint.paint()); } @@ -406,7 +406,7 @@ void Canvas::drawAtlas(const Paint& paint, static_assert(sizeof(SkRect) == sizeof(float) * 4, "SkRect doesn't use floats."); - image_allocation_size_ += atlas->GetAllocationSize(); + external_allocation_size_ += atlas->GetAllocationSize(); canvas_->drawAtlas( skImage.get(), reinterpret_cast(transforms.data()), reinterpret_cast(rects.data()), diff --git a/lib/ui/painting/canvas.h b/lib/ui/painting/canvas.h index 8f5a459ab7..e626b96bbf 100644 --- a/lib/ui/painting/canvas.h +++ b/lib/ui/painting/canvas.h @@ -170,7 +170,7 @@ class Canvas : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); - size_t image_allocation_size() const { return image_allocation_size_; } + size_t external_allocation_size() const { return external_allocation_size_; } private: explicit Canvas(SkCanvas* canvas); @@ -179,7 +179,7 @@ class Canvas : public RefCountedDartWrappable { // which does not transfer ownership. For this reason, we hold a raw // pointer and manually set to null in Clear. SkCanvas* canvas_; - size_t image_allocation_size_ = 0; + size_t external_allocation_size_ = 0; }; } // namespace flutter diff --git a/lib/ui/painting/picture.cc b/lib/ui/painting/picture.cc index 6457d74ab4..ee3f2053c9 100644 --- a/lib/ui/painting/picture.cc +++ b/lib/ui/painting/picture.cc @@ -28,18 +28,18 @@ DART_BIND_ALL(Picture, FOR_EACH_BINDING) fml::RefPtr Picture::Create(Dart_Handle dart_handle, flutter::SkiaGPUObject picture, - size_t image_allocation_size) { - auto canvas_picture = - fml::MakeRefCounted(std::move(picture), image_allocation_size); + size_t external_allocation_size) { + auto canvas_picture = fml::MakeRefCounted(std::move(picture), + external_allocation_size); canvas_picture->AssociateWithDartWrapper(dart_handle); return canvas_picture; } Picture::Picture(flutter::SkiaGPUObject picture, - size_t image_allocation_size) + size_t external_allocation_size) : picture_(std::move(picture)), - image_allocation_size_(image_allocation_size) {} + external_allocation_size_(external_allocation_size) {} Picture::~Picture() = default; @@ -61,7 +61,7 @@ void Picture::dispose() { size_t Picture::GetAllocationSize() const { if (auto picture = picture_.get()) { return picture->approximateBytesUsed() + sizeof(Picture) + - image_allocation_size_; + external_allocation_size_; } else { return sizeof(Picture); } diff --git a/lib/ui/painting/picture.h b/lib/ui/painting/picture.h index 20d21ad3ba..8f1879e1af 100644 --- a/lib/ui/painting/picture.h +++ b/lib/ui/painting/picture.h @@ -26,7 +26,7 @@ class Picture : public RefCountedDartWrappable { ~Picture() override; static fml::RefPtr Create(Dart_Handle dart_handle, flutter::SkiaGPUObject picture, - size_t image_allocation_size); + size_t external_allocation_size); sk_sp picture() const { return picture_.get(); } @@ -45,14 +45,14 @@ class Picture : public RefCountedDartWrappable { uint32_t height, Dart_Handle raw_image_callback); - size_t image_allocation_size() const { return image_allocation_size_; } + size_t external_allocation_size() const { return external_allocation_size_; } private: Picture(flutter::SkiaGPUObject picture, - size_t image_allocation_size_); + size_t external_allocation_size_); flutter::SkiaGPUObject picture_; - size_t image_allocation_size_; + size_t external_allocation_size_; }; } // namespace flutter diff --git a/lib/ui/painting/picture_recorder.cc b/lib/ui/painting/picture_recorder.cc index ab044adbf3..809f761e96 100644 --- a/lib/ui/painting/picture_recorder.cc +++ b/lib/ui/painting/picture_recorder.cc @@ -56,7 +56,7 @@ fml::RefPtr PictureRecorder::endRecording(Dart_Handle dart_picture) { Picture::Create(dart_picture, UIDartState::CreateGPUObject( picture_recorder_.finishRecordingAsPicture()), - canvas_->image_allocation_size()); + canvas_->external_allocation_size()); canvas_->Clear(); canvas_->ClearDartWrapper(); diff --git a/lib/ui/painting/vertices.cc b/lib/ui/painting/vertices.cc index 41425e8496..089be64e8c 100644 --- a/lib/ui/painting/vertices.cc +++ b/lib/ui/painting/vertices.cc @@ -27,11 +27,6 @@ void DecodeInts(const tonic::Int32List& ints, T* out) { } // namespace -static void Vertices_constructor(Dart_NativeArguments args) { - UIDartState::ThrowIfUIOperationsProhibited(); - DartCallConstructor(&Vertices::Create, args); -} - IMPLEMENT_WRAPPERTYPEINFO(ui, Vertices); #define FOR_EACH_BINDING(V) V(Vertices, init) @@ -43,19 +38,16 @@ Vertices::Vertices() {} Vertices::~Vertices() {} void Vertices::RegisterNatives(tonic::DartLibraryNatives* natives) { - natives->Register({{"Vertices_constructor", Vertices_constructor, 1, true}, - FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); -} - -fml::RefPtr Vertices::Create() { - return fml::MakeRefCounted(); + natives->Register({FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); } -bool Vertices::init(SkVertices::VertexMode vertex_mode, +bool Vertices::init(Dart_Handle vertices_handle, + SkVertices::VertexMode vertex_mode, const tonic::Float32List& positions, const tonic::Float32List& texture_coordinates, const tonic::Int32List& colors, const tonic::Uint16List& indices) { + UIDartState::ThrowIfUIOperationsProhibited(); uint32_t builderFlags = 0; if (texture_coordinates.data()) builderFlags |= SkVertices::kHasTexCoords_BuilderFlag; @@ -89,9 +81,15 @@ bool Vertices::init(SkVertices::VertexMode vertex_mode, builder.indices()); } - vertices_ = builder.detach(); + auto vertices = fml::MakeRefCounted(); + vertices->vertices_ = builder.detach(); + vertices->AssociateWithDartWrapper(vertices_handle); return true; } +size_t Vertices::GetAllocationSize() const { + return sizeof(Vertices) + vertices_->approximateSize(); +} + } // namespace flutter diff --git a/lib/ui/painting/vertices.h b/lib/ui/painting/vertices.h index 2e9339147e..1b4c8601df 100644 --- a/lib/ui/painting/vertices.h +++ b/lib/ui/painting/vertices.h @@ -24,16 +24,17 @@ class Vertices : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); - static fml::RefPtr Create(); - - bool init(SkVertices::VertexMode vertex_mode, - const tonic::Float32List& positions, - const tonic::Float32List& texture_coordinates, - const tonic::Int32List& colors, - const tonic::Uint16List& indices); + static bool init(Dart_Handle vertices_handle, + SkVertices::VertexMode vertex_mode, + const tonic::Float32List& positions, + const tonic::Float32List& texture_coordinates, + const tonic::Int32List& colors, + const tonic::Uint16List& indices); const sk_sp& vertices() const { return vertices_; } + size_t GetAllocationSize() const override; + private: Vertices(); diff --git a/lib/ui/painting/vertices_unittests.cc b/lib/ui/painting/vertices_unittests.cc new file mode 100644 index 0000000000..0e44815ab0 --- /dev/null +++ b/lib/ui/painting/vertices_unittests.cc @@ -0,0 +1,61 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/common/task_runners.h" +#include "flutter/fml/synchronization/waitable_event.h" +#include "flutter/lib/ui/painting/vertices.h" +#include "flutter/runtime/dart_vm.h" +#include "flutter/shell/common/shell_test.h" +#include "flutter/shell/common/thread_host.h" +#include "flutter/testing/testing.h" + +namespace flutter { +namespace testing { + +TEST_F(ShellTest, VerticesAccuratelyReportsSize) { + fml::AutoResetWaitableEvent message_latch; + + auto nativeValidateVertices = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + intptr_t peer = 0; + Dart_Handle result = Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer); + ASSERT_FALSE(Dart_IsError(result)); + Vertices* vertices = reinterpret_cast(peer); + // GT because we don't want to be too dependent on platform specific + // differences, or minor changes to Skia. The actual value of this on + // macOS as of the test writing is 1441890ul. Just need to assert it's + // big enough to get the Dart GC's attention. + ASSERT_GT(vertices->GetAllocationSize(), 1300000ul); + message_latch.Signal(); + }; + + Settings settings = CreateSettingsForFixture(); + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + AddNativeCallback("ValidateVertices", + CREATE_NATIVE_ENTRY(nativeValidateVertices)); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("createVertices"); + + shell->RunEngine(std::move(configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch.Wait(); + DestroyShell(std::move(shell), std::move(task_runners)); +} + +} // namespace testing +} // namespace flutter diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index 9ec4ac91a5..cd1e40608b 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -199,14 +199,10 @@ if (enable_unittests) { fixtures = [ "fixtures/shelltest_screenshot.png" ] } - shell_host_executable("shell_unittests") { + source_set("shell_test_fixture_sources") { + testonly = true + sources = [ - "animator_unittests.cc", - "canvas_spy_unittests.cc", - "input_events_unittests.cc", - "persistent_cache_unittests.cc", - "pipeline_unittests.cc", - "renderer_context_manager_unittests.cc", "renderer_context_test.cc", "renderer_context_test.h", "shell_test.cc", @@ -215,27 +211,21 @@ if (enable_unittests) { "shell_test_external_view_embedder.h", "shell_test_platform_view.cc", "shell_test_platform_view.h", - "shell_unittests.cc", "vsync_waiters_test.cc", "vsync_waiters_test.h", ] - deps = [ - ":shell_unittests_fixtures", - ":shell_unittests_gpu_configuration", - "//flutter/assets", - "//flutter/common", + public_deps = [ "//flutter/flow", "//flutter/fml/dart", - "//flutter/lib/ui:ui", + "//flutter/lib/ui", + "//flutter/runtime", "//flutter/shell", + "//flutter/shell/common", "//flutter/testing", "//flutter/testing:dart", ] - - if (!defined(defines)) { - defines = [] - } + defines = [] # SwiftShader only supports x86/x64_64 if (target_cpu == "x86" || target_cpu == "x64") { @@ -245,7 +235,10 @@ if (enable_unittests) { "shell_test_platform_view_gl.h", ] - deps += [ "//flutter/testing:opengl" ] + public_deps += [ + "//flutter/shell/gpu:gpu_surface_gl", + "//flutter/testing:opengl", + ] defines += [ "SHELL_ENABLE_GL" ] } @@ -256,7 +249,8 @@ if (enable_unittests) { "shell_test_platform_view_vulkan.h", ] - deps += [ + public_deps += [ + "//flutter/shell/gpu:gpu_surface_vulkan", "//flutter/testing:vulkan", "//flutter/vulkan", ] @@ -266,6 +260,31 @@ if (enable_unittests) { } } + shell_host_executable("shell_unittests") { + sources = [ + "animator_unittests.cc", + "canvas_spy_unittests.cc", + "input_events_unittests.cc", + "persistent_cache_unittests.cc", + "pipeline_unittests.cc", + "renderer_context_manager_unittests.cc", + "shell_unittests.cc", + ] + + deps = [ + ":shell_test_fixture_sources", + ":shell_unittests_fixtures", + ":shell_unittests_gpu_configuration", + "//flutter/assets", + "//flutter/common", + "//flutter/shell", + ] + + if (!defined(defines)) { + defines = [] + } + } + if (is_fuchsia) { fuchsia_test_archive("shell_tests") { deps = [ diff --git a/testing/dart/canvas_test.dart b/testing/dart/canvas_test.dart index b02a69fd3b..ac35140a51 100644 --- a/testing/dart/canvas_test.dart +++ b/testing/dart/canvas_test.dart @@ -245,4 +245,45 @@ void main() { expect(picture2.approximateBytesUsed, greaterThan(minimumExpected)); }); + + test('Vertex buffer size reflected in picture size for drawVertices', () async { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + + const int uint16max = 65535; + + final Int32List colors = Int32List(uint16max); + final Float32List coords = Float32List(uint16max * 2); + final Uint16List indices = Uint16List(uint16max); + final Float32List positions = Float32List(uint16max * 2); + colors[0] = const Color(0xFFFF0000).value; + colors[1] = const Color(0xFF00FF00).value; + colors[2] = const Color(0xFF0000FF).value; + colors[3] = const Color(0xFF00FFFF).value; + indices[1] = indices[3] = 1; + indices[2] = indices[5] = 3; + indices[4] = 2; + positions[2] = positions[4] = positions[5] = positions[7] = 250.0; + + final Vertices vertices = Vertices.raw( + VertexMode.triangles, + positions, + textureCoordinates: coords, + colors: colors, + indices: indices, + ); + canvas.drawVertices(vertices, BlendMode.src, Paint()); + final Picture picture = recorder.endRecording(); + + + const int minimumExpected = uint16max * 4; + expect(picture.approximateBytesUsed, greaterThan(minimumExpected)); + + final PictureRecorder recorder2 = PictureRecorder(); + final Canvas canvas2 = Canvas(recorder2); + canvas2.drawPicture(picture); + final Picture picture2 = recorder2.endRecording(); + + expect(picture2.approximateBytesUsed, greaterThan(minimumExpected)); + }); } -- GitLab