From 3f37b239417f30db5b7c0cb2e9bb14cc81f41ced Mon Sep 17 00:00:00 2001 From: Zach Anderson Date: Fri, 3 Jul 2020 14:32:01 -0700 Subject: [PATCH] Avoid a copy in EncodeImage --- ci/licenses_golden/licenses_flutter | 1 + lib/ui/BUILD.gn | 1 + lib/ui/fixtures/ui_test.dart | 21 ++++++ lib/ui/painting/image_encoding.cc | 28 ++++++-- lib/ui/painting/image_encoding_unittests.cc | 75 +++++++++++++++++++++ 5 files changed, 119 insertions(+), 7 deletions(-) create mode 100644 lib/ui/painting/image_encoding_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 5183b93542444..d864a1bf084a7 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -337,6 +337,7 @@ FILE: ../../../flutter/lib/ui/painting/image_decoder.h FILE: ../../../flutter/lib/ui/painting/image_decoder_unittests.cc FILE: ../../../flutter/lib/ui/painting/image_encoding.cc FILE: ../../../flutter/lib/ui/painting/image_encoding.h +FILE: ../../../flutter/lib/ui/painting/image_encoding_unittests.cc FILE: ../../../flutter/lib/ui/painting/image_filter.cc FILE: ../../../flutter/lib/ui/painting/image_filter.h FILE: ../../../flutter/lib/ui/painting/image_shader.cc diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index 6df56ae91acdd..c5b21b82273b4 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -161,6 +161,7 @@ if (current_toolchain == host_toolchain) { sources = [ "painting/image_decoder_unittests.cc", + "painting/image_encoding_unittests.cc", "painting/vertices_unittests.cc", "window/pointer_data_packet_converter_unittests.cc", ] diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index ecd42cc041bf5..72a63f44ea809 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -44,3 +44,24 @@ void frameCallback(FrameInfo info) { @pragma('vm:entry-point') void messageCallback(dynamic data) { } + + +// Draw a circle on a Canvas that has a PictureRecorder. Take the image from +// the PictureRecorder, and encode it as png. Check that the png data is +// backed by an external Uint8List. +@pragma('vm:entry-point') +Future encodeImageProducesExternalUint8List() async { + final PictureRecorder pictureRecorder = PictureRecorder(); + final Canvas canvas = Canvas(pictureRecorder); + final Paint paint = Paint() + ..color = Color.fromRGBO(255, 255, 255, 1.0) + ..style = PaintingStyle.fill; + final Offset c = Offset(50.0, 50.0); + canvas.drawCircle(c, 25.0, paint); + final Picture picture = pictureRecorder.endRecording(); + final Image image = await picture.toImage(100, 100); + _encodeImage(image, ImageByteFormat.png.index, _validateExternal); +} +void _encodeImage(Image i, int format, void Function(Uint8List result)) + native 'EncodeImage'; +void _validateExternal(Uint8List result) native 'ValidateExternal'; diff --git a/lib/ui/painting/image_encoding.cc b/lib/ui/painting/image_encoding.cc index c82f623c00af1..e48b76e63726e 100644 --- a/lib/ui/painting/image_encoding.cc +++ b/lib/ui/painting/image_encoding.cc @@ -35,6 +35,13 @@ enum ImageByteFormat { kPNG, }; +void FinalizeSkData(void* isolate_callback_data, + Dart_WeakPersistentHandle handle, + void* peer) { + SkData* buffer = reinterpret_cast(peer); + buffer->unref(); +} + void InvokeDataCallback(std::unique_ptr callback, sk_sp buffer) { std::shared_ptr dart_state = callback->dart_state().lock(); @@ -44,11 +51,17 @@ void InvokeDataCallback(std::unique_ptr callback, tonic::DartState::Scope scope(dart_state); if (!buffer) { DartInvoke(callback->value(), {Dart_Null()}); - } else { - Dart_Handle dart_data = tonic::DartConverter::ToDart( - buffer->bytes(), buffer->size()); - DartInvoke(callback->value(), {dart_data}); + return; } + FML_DCHECK(buffer->unique()); + // If we have the only reference to the data, then don't copy it. Rather, + // shift ownership of the pointer to a Dart external Uint8List. + void* bytes = buffer->writable_data(); + const intptr_t length = buffer->size(); + void* peer = reinterpret_cast(buffer.release()); + Dart_Handle dart_data = Dart_NewExternalTypedDataWithFinalizer( + Dart_TypedData_kUint8, bytes, length, peer, length, FinalizeSkData); + DartInvoke(callback->value(), {dart_data}); } sk_sp ConvertToRasterUsingResourceContext( @@ -222,9 +235,10 @@ void EncodeImageAndInvokeDataCallback( auto encode_task = [callback_task = std::move(callback_task), format, ui_task_runner](sk_sp raster_image) { sk_sp encoded = EncodeImage(std::move(raster_image), format); - ui_task_runner->PostTask( - [callback_task = std::move(callback_task), - encoded = std::move(encoded)] { callback_task(encoded); }); + ui_task_runner->PostTask([callback_task = std::move(callback_task), + encoded = std::move(encoded)]() mutable { + callback_task(std::move(encoded)); + }); }; ConvertImageToRaster(std::move(image), encode_task, raster_task_runner, diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc new file mode 100644 index 0000000000000..a216a43e7d644 --- /dev/null +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -0,0 +1,75 @@ +// 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/image_encoding.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, EncodeImageGivesExternalTypedData) { + fml::AutoResetWaitableEvent message_latch; + + auto nativeEncodeImage = [&](Dart_NativeArguments args) { + auto image_handle = Dart_GetNativeArgument(args, 0); + auto format_handle = Dart_GetNativeArgument(args, 1); + auto callback_handle = Dart_GetNativeArgument(args, 2); + + intptr_t peer = 0; + Dart_Handle result = Dart_GetNativeInstanceField( + image_handle, tonic::DartWrappable::kPeerIndex, &peer); + ASSERT_FALSE(Dart_IsError(result)); + CanvasImage* canvas_image = reinterpret_cast(peer); + + int64_t format = -1; + result = Dart_IntegerToInt64(format_handle, &format); + ASSERT_FALSE(Dart_IsError(result)); + + result = EncodeImage(canvas_image, format, callback_handle); + ASSERT_TRUE(Dart_IsNull(result)); + }; + + auto nativeValidateExternal = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + + auto typed_data_type = Dart_GetTypeOfExternalTypedData(handle); + EXPECT_EQ(typed_data_type, Dart_TypedData_kUint8); + + message_latch.Signal(); + }; + + Settings settings = CreateSettingsForFixture(); + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + AddNativeCallback("EncodeImage", CREATE_NATIVE_ENTRY(nativeEncodeImage)); + AddNativeCallback("ValidateExternal", + CREATE_NATIVE_ENTRY(nativeValidateExternal)); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("encodeImageProducesExternalUint8List"); + + 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