Avoid a copy in EncodeImage (flutter#19504)

Author: zanderso

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

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",
     ]

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<void> 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';

lib/ui/painting.dart

@@ -1600,17 +1600,20 @@ class Image extends NativeFieldWrapperClass2 {
   /// The number of image pixels along the image's vertical axis.
   int get height native 'Image_height';
 
-  /// Converts the [Image] object into a byte array.
+  /// Converts the [Image] object into a read-only byte array.
   ///
   /// The [format] argument specifies the format in which the bytes will be
   /// returned.
   ///
   /// Returns a future that completes with the binary image data or an error
-  /// if encoding fails.
+  /// if encoding fails. Note that attempting to write to the returned
+  /// [ByteData] will result in an [UnsupportedError] exception.
   Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) {
     return _futurize((_Callback<ByteData> callback) {
       return _toByteData(format.index, (Uint8List? encoded) {
-        callback(encoded!.buffer.asByteData());
+        // [encoded] wraps a read-only SkData buffer, so we wrap it here in
+        // an [UnmodifiableByteDataView].
+        callback(UnmodifiableByteDataView(encoded!.buffer.asByteData()));
       });
     });
   }

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<SkData*>(peer);
+  buffer->unref();
+}
+
 void InvokeDataCallback(std::unique_ptr<DartPersistentValue> callback,
                         sk_sp<SkData> buffer) {
   std::shared_ptr<tonic::DartState> dart_state = callback->dart_state().lock();
@@ -44,11 +51,15 @@ void InvokeDataCallback(std::unique_ptr<DartPersistentValue> callback,
   tonic::DartState::Scope scope(dart_state);
   if (!buffer) {
     DartInvoke(callback->value(), {Dart_Null()});
-  } else {
-    Dart_Handle dart_data = tonic::DartConverter<tonic::Uint8List>::ToDart(
-        buffer->bytes(), buffer->size());
-    DartInvoke(callback->value(), {dart_data});
+    return;
   }
+  // SkData are generally read-only.
+  void* bytes = const_cast<void*>(buffer->data());
+  const intptr_t length = buffer->size();
+  void* peer = reinterpret_cast<void*>(buffer.release());
+  Dart_Handle dart_data = Dart_NewExternalTypedDataWithFinalizer(
+      Dart_TypedData_kUint8, bytes, length, peer, length, FinalizeSkData);
+  DartInvoke(callback->value(), {dart_data});
 }
 
 sk_sp<SkImage> ConvertToRasterUsingResourceContext(
@@ -222,9 +233,10 @@ void EncodeImageAndInvokeDataCallback(
   auto encode_task = [callback_task = std::move(callback_task), format,
                       ui_task_runner](sk_sp<SkImage> raster_image) {
     sk_sp<SkData> 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,

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<CanvasImage*>(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> 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