From a96a8cc9ce6a46d08e3b1bb76d2d76dc4cfe2ace Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 2 Jun 2020 15:12:06 -0700 Subject: [PATCH] Make images contribute to Picture allocation size, more aggressively release references when dispose is called (flutter/engine#18706) SkImage references get held by both our Image and Picture objects. The GC has no idea about this, and so sometimes pictures look very small when they're actually holding a reference to a large chunk of memory. This patch helps make sure that the GC can more adequately catch the real size impact of Picture objects, and combined with an upstream patch in Dart allows for much more aggressive collection of unused image related memory. --- engine/src/flutter/lib/ui/painting/canvas.cc | 5 ++ engine/src/flutter/lib/ui/painting/canvas.h | 3 ++ engine/src/flutter/lib/ui/painting/image.cc | 1 + .../flutter/lib/ui/painting/image_decoder.cc | 4 +- engine/src/flutter/lib/ui/painting/picture.cc | 19 +++++--- engine/src/flutter/lib/ui/painting/picture.h | 10 +++- .../lib/ui/painting/picture_recorder.cc | 9 ++-- .../src/flutter/testing/dart/canvas_test.dart | 47 +++++++++++++++++++ 8 files changed, 84 insertions(+), 14 deletions(-) diff --git a/engine/src/flutter/lib/ui/painting/canvas.cc b/engine/src/flutter/lib/ui/painting/canvas.cc index 3b06b394947..2ee718cd2f2 100644 --- a/engine/src/flutter/lib/ui/painting/canvas.cc +++ b/engine/src/flutter/lib/ui/painting/canvas.cc @@ -293,6 +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(); canvas_->drawImage(image->image(), x, y, paint.paint()); } @@ -314,6 +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(); canvas_->drawImageRect(image->image(), src, dst, paint.paint(), SkCanvas::kFast_SrcRectConstraint); } @@ -339,6 +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(); canvas_->drawImageNine(image->image(), icenter, dst, paint.paint()); } @@ -348,6 +351,7 @@ void Canvas::drawPicture(Picture* picture) { if (!picture) Dart_ThrowException( ToDart("Canvas.drawPicture called with non-genuine Picture.")); + image_allocation_size_ += picture->GetAllocationSize(); canvas_->drawPicture(picture->picture().get()); } @@ -402,6 +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(); canvas_->drawAtlas( skImage.get(), reinterpret_cast(transforms.data()), reinterpret_cast(rects.data()), diff --git a/engine/src/flutter/lib/ui/painting/canvas.h b/engine/src/flutter/lib/ui/painting/canvas.h index e431da8bbf8..8f5a459ab7e 100644 --- a/engine/src/flutter/lib/ui/painting/canvas.h +++ b/engine/src/flutter/lib/ui/painting/canvas.h @@ -170,6 +170,8 @@ class Canvas : public RefCountedDartWrappable { static void RegisterNatives(tonic::DartLibraryNatives* natives); + size_t image_allocation_size() const { return image_allocation_size_; } + private: explicit Canvas(SkCanvas* canvas); @@ -177,6 +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; }; } // namespace flutter diff --git a/engine/src/flutter/lib/ui/painting/image.cc b/engine/src/flutter/lib/ui/painting/image.cc index 151d76ec6a6..126205530fa 100644 --- a/engine/src/flutter/lib/ui/painting/image.cc +++ b/engine/src/flutter/lib/ui/painting/image.cc @@ -38,6 +38,7 @@ Dart_Handle CanvasImage::toByteData(int format, Dart_Handle callback) { void CanvasImage::dispose() { ClearDartWrapper(); + image_.reset(); } size_t CanvasImage::GetAllocationSize() const { diff --git a/engine/src/flutter/lib/ui/painting/image_decoder.cc b/engine/src/flutter/lib/ui/painting/image_decoder.cc index 604b41d7dcd..5b322e1d8de 100644 --- a/engine/src/flutter/lib/ui/painting/image_decoder.cc +++ b/engine/src/flutter/lib/ui/painting/image_decoder.cc @@ -278,7 +278,7 @@ static SkiaGPUObject UploadRasterImage( SkSafeUnref(static_cast(context)); }, image.get()); - result = {texture_image, nullptr}; + result = {std::move(texture_image), nullptr}; }) .SetIfFalse([&result, context = io_manager->GetResourceContext(), &pixmap, queue = io_manager->GetSkiaUnrefQueue()] { @@ -293,7 +293,7 @@ static SkiaGPUObject UploadRasterImage( FML_LOG(ERROR) << "Could not make x-context image."; result = {}; } else { - result = {texture_image, queue}; + result = {std::move(texture_image), queue}; } })); diff --git a/engine/src/flutter/lib/ui/painting/picture.cc b/engine/src/flutter/lib/ui/painting/picture.cc index 051dd27dc1f..6457d74ab47 100644 --- a/engine/src/flutter/lib/ui/painting/picture.cc +++ b/engine/src/flutter/lib/ui/painting/picture.cc @@ -26,17 +26,20 @@ IMPLEMENT_WRAPPERTYPEINFO(ui, Picture); DART_BIND_ALL(Picture, FOR_EACH_BINDING) -fml::RefPtr Picture::Create( - Dart_Handle dart_handle, - flutter::SkiaGPUObject picture) { - auto canvas_picture = fml::MakeRefCounted(std::move(picture)); +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); canvas_picture->AssociateWithDartWrapper(dart_handle); return canvas_picture; } -Picture::Picture(flutter::SkiaGPUObject picture) - : picture_(std::move(picture)) {} +Picture::Picture(flutter::SkiaGPUObject picture, + size_t image_allocation_size) + : picture_(std::move(picture)), + image_allocation_size_(image_allocation_size) {} Picture::~Picture() = default; @@ -52,11 +55,13 @@ Dart_Handle Picture::toImage(uint32_t width, void Picture::dispose() { ClearDartWrapper(); + picture_.reset(); } size_t Picture::GetAllocationSize() const { if (auto picture = picture_.get()) { - return picture->approximateBytesUsed(); + return picture->approximateBytesUsed() + sizeof(Picture) + + image_allocation_size_; } else { return sizeof(Picture); } diff --git a/engine/src/flutter/lib/ui/painting/picture.h b/engine/src/flutter/lib/ui/painting/picture.h index 093a13d2dca..20d21ad3ba7 100644 --- a/engine/src/flutter/lib/ui/painting/picture.h +++ b/engine/src/flutter/lib/ui/painting/picture.h @@ -8,6 +8,7 @@ #include "flutter/flow/skia_gpu_object.h" #include "flutter/lib/ui/dart_wrapper.h" #include "flutter/lib/ui/painting/image.h" +#include "flutter/lib/ui/ui_dart_state.h" #include "third_party/skia/include/core/SkPicture.h" namespace tonic { @@ -24,7 +25,8 @@ class Picture : public RefCountedDartWrappable { public: ~Picture() override; static fml::RefPtr Create(Dart_Handle dart_handle, - flutter::SkiaGPUObject picture); + flutter::SkiaGPUObject picture, + size_t image_allocation_size); sk_sp picture() const { return picture_.get(); } @@ -43,10 +45,14 @@ class Picture : public RefCountedDartWrappable { uint32_t height, Dart_Handle raw_image_callback); + size_t image_allocation_size() const { return image_allocation_size_; } + private: - explicit Picture(flutter::SkiaGPUObject picture); + Picture(flutter::SkiaGPUObject picture, + size_t image_allocation_size_); flutter::SkiaGPUObject picture_; + size_t image_allocation_size_; }; } // namespace flutter diff --git a/engine/src/flutter/lib/ui/painting/picture_recorder.cc b/engine/src/flutter/lib/ui/painting/picture_recorder.cc index d86c1c0fd55..ab044adbf37 100644 --- a/engine/src/flutter/lib/ui/painting/picture_recorder.cc +++ b/engine/src/flutter/lib/ui/painting/picture_recorder.cc @@ -52,9 +52,12 @@ fml::RefPtr PictureRecorder::endRecording(Dart_Handle dart_picture) { if (!isRecording()) return nullptr; - fml::RefPtr picture = Picture::Create( - dart_picture, UIDartState::CreateGPUObject( - picture_recorder_.finishRecordingAsPicture())); + fml::RefPtr picture = + Picture::Create(dart_picture, + UIDartState::CreateGPUObject( + picture_recorder_.finishRecordingAsPicture()), + canvas_->image_allocation_size()); + canvas_->Clear(); canvas_->ClearDartWrapper(); canvas_ = nullptr; diff --git a/engine/src/flutter/testing/dart/canvas_test.dart b/engine/src/flutter/testing/dart/canvas_test.dart index 25d1ed04dc4..b02a69fd3bf 100644 --- a/engine/src/flutter/testing/dart/canvas_test.dart +++ b/engine/src/flutter/testing/dart/canvas_test.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. // @dart = 2.6 +import 'dart:async'; import 'dart:io'; import 'dart:typed_data'; import 'dart:ui'; @@ -13,6 +14,24 @@ import 'package:test/test.dart'; typedef CanvasCallback = void Function(Canvas canvas); +Future createImage(int width, int height) { + final Completer completer = Completer(); + decodeImageFromPixels( + Uint8List.fromList(List.generate( + width * height * 4, + (int pixel) => pixel % 255, + )), + width, + height, + PixelFormat.rgba8888, + (Image image) { + completer.complete(image); + }, + ); + + return completer.future; +} + void testCanvas(CanvasCallback callback) { try { callback(Canvas(PictureRecorder(), const Rect.fromLTRB(0.0, 0.0, 0.0, 0.0))); @@ -198,4 +217,32 @@ void main() { await fuzzyGoldenImageCompare(image, 'canvas_test_dithered_gradient.png'); expect(areEqual, true); }, skip: !Platform.isLinux); // https://github.com/flutter/flutter/issues/53784 + + test('Image size reflected in picture size for image*, drawAtlas, and drawPicture methods', () async { + final Image image = await createImage(100, 100); + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + const Rect rect = Rect.fromLTWH(0, 0, 100, 100); + canvas.drawImage(image, Offset.zero, Paint()); + canvas.drawImageRect(image, rect, rect, Paint()); + canvas.drawImageNine(image, rect, rect, Paint()); + canvas.drawAtlas(image, [], [], [], BlendMode.src, rect, Paint()); + final Picture picture = recorder.endRecording(); + + // Some of the numbers here appear to utilize sharing/reuse of common items, + // e.g. of the Paint() or same `Rect` usage, etc. + // The raw utilization of a 100x100 picture here should be 53333: + // 100 * 100 * 4 * (4/3) = 53333.333333.... + // To avoid platform specific idiosyncrasies and brittleness against changes + // to Skia, we just assert this is _at least_ 4x the image size. + const int minimumExpected = 53333 * 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)); + }); }