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

Use hint freed specifically for image disposal #20754

Merged
merged 17 commits into from
Sep 2, 2020
Merged
Show file tree
Hide file tree
Changes from 11 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
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ FILE: ../../../flutter/lib/ui/fixtures/hello_loop_2.webp
FILE: ../../../flutter/lib/ui/fixtures/ui_test.dart
FILE: ../../../flutter/lib/ui/geometry.dart
FILE: ../../../flutter/lib/ui/hash_codes.dart
FILE: ../../../flutter/lib/ui/hint_freed_delegate.h
FILE: ../../../flutter/lib/ui/hooks.dart
FILE: ../../../flutter/lib/ui/io_manager.h
FILE: ../../../flutter/lib/ui/isolate_name_server.dart
Expand Down Expand Up @@ -337,6 +338,7 @@ FILE: ../../../flutter/lib/ui/painting/image_decoder.h
FILE: ../../../flutter/lib/ui/painting/image_decoder_unittests.cc
FILE: ../../../flutter/lib/ui/painting/image_descriptor.cc
FILE: ../../../flutter/lib/ui/painting/image_descriptor.h
FILE: ../../../flutter/lib/ui/painting/image_dispose_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
Expand Down
1 change: 1 addition & 0 deletions lib/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ if (enable_unittests) {
public_configs = [ "//flutter:export_dynamic_symbols" ]

sources = [
"painting/image_dispose_unittests.cc",
"painting/image_encoding_unittests.cc",
"painting/vertices_unittests.cc",
"window/platform_configuration_unittests.cc",
Expand Down
59 changes: 59 additions & 0 deletions lib/ui/fixtures/ui_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';
import 'dart:typed_data';
import 'dart:ui';

Expand Down Expand Up @@ -73,3 +74,61 @@ Future<void> encodeImageProducesExternalUint8List() async {
void _encodeImage(Image i, int format, void Function(Uint8List result))
native 'EncodeImage';
void _validateExternal(Uint8List result) native 'ValidateExternal';

@pragma('vm:entry-point')
Future<void> pumpImage() async {
const int width = 4000;
const int height = 4000;
final Completer<Image> completer = Completer<Image>();
decodeImageFromPixels(
Uint8List.fromList(List<int>.filled(width * height * 4, 0xFF)),
width,
height,
PixelFormat.rgba8888,
(Image image) => completer.complete(image),
);
final Image image = await completer.future;

final FrameCallback renderBlank = (Duration duration) {
image.dispose();

final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawRect(Rect.largest, Paint());
final Picture picture = recorder.endRecording();

final SceneBuilder builder = SceneBuilder();
builder.addPicture(Offset.zero, picture);

final Scene scene = builder.build();
window.render(scene);
scene.dispose();
window.onBeginFrame = (Duration duration) {
window.onDrawFrame = _onBeginFrameDone;
};
window.scheduleFrame();
};

final FrameCallback renderImage = (Duration duration) {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawImage(image, Offset.zero, Paint());
final Picture picture = recorder.endRecording();

final SceneBuilder builder = SceneBuilder();
builder.addPicture(Offset.zero, picture);

_captureImageAndPicture(image, picture);

final Scene scene = builder.build();
window.render(scene);
scene.dispose();
window.onBeginFrame = renderBlank;
window.scheduleFrame();
};

window.onBeginFrame = renderImage;
window.scheduleFrame();
}
void _captureImageAndPicture(Image image, Picture picture) native 'CaptureImageAndPicture';
Future<void> _onBeginFrameDone() native 'OnBeginFrameDone';
24 changes: 24 additions & 0 deletions lib/ui/hint_freed_delegate.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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.

#ifndef FLUTTER_LIB_UI_HINT_FREED_DELEGATE_H_
#define FLUTTER_LIB_UI_HINT_FREED_DELEGATE_H_

namespace flutter {

class HintFreedDelegate {
public:
//----------------------------------------------------------------------------
/// @brief Notifies the engine that native bytes might be freed if a
/// garbage collection ran at the next NotifyIdle period.
///
/// @param[in] size The number of bytes freed. This size adds to any
/// previously supplied value, rather than replacing.
///
virtual void HintFreed(size_t size) = 0;
};

} // namespace flutter

#endif // FLUTTER_LIB_UI_HINT_FREED_DELEGATE_H_
6 changes: 0 additions & 6 deletions lib/ui/painting/canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ void Canvas::clipPath(const CanvasPath* path, bool doAntiAlias) {
ToDart("Canvas.clipPath called with non-genuine Path."));
return;
}
external_allocation_size_ += path->path().approximateBytesUsed();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the engine no longer be reporting these sizes as the external allocation size for Dart objects with native fields? Or does this only affect HintFreed? (Same question here and below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I can split this into another patch, but basically this seems like it was a mistake. We've already moved the reporting of images for this.

The basic problem is that from scene to scene, we might be reusing these things, and end up reporting vastly inflated numbers to the VM, and then that creates lots of wasteful garbage collections.

canvas_->clipPath(path->path(), doAntiAlias);
}

Expand Down Expand Up @@ -310,7 +309,6 @@ void Canvas::drawPath(const CanvasPath* path,
ToDart("Canvas.drawPath called with non-genuine Path."));
return;
}
external_allocation_size_ += path->path().approximateBytesUsed();
canvas_->drawPath(path->path(), *paint.paint());
}

Expand Down Expand Up @@ -391,7 +389,6 @@ void Canvas::drawPicture(Picture* picture) {
ToDart("Canvas.drawPicture called with non-genuine Picture."));
return;
}
external_allocation_size_ += picture->GetAllocationSize();
canvas_->drawPicture(picture->picture().get());
}

Expand Down Expand Up @@ -424,7 +421,6 @@ void Canvas::drawVertices(const Vertices* vertices,
ToDart("Canvas.drawVertices called with non-genuine Vertices."));
return;
}
external_allocation_size_ += vertices->GetAllocationSize();
canvas_->drawVertices(vertices->vertices(), blend_mode, *paint.paint());
}

Expand Down Expand Up @@ -453,7 +449,6 @@ void Canvas::drawAtlas(const Paint& paint,
static_assert(sizeof(SkRect) == sizeof(float) * 4,
"SkRect doesn't use floats.");

external_allocation_size_ += atlas->GetAllocationSize();
canvas_->drawAtlas(
skImage.get(), reinterpret_cast<const SkRSXform*>(transforms.data()),
reinterpret_cast<const SkRect*>(rects.data()),
Expand All @@ -477,7 +472,6 @@ void Canvas::drawShadow(const CanvasPath* path,
->window()
->viewport_metrics()
.device_pixel_ratio;
external_allocation_size_ += path->path().approximateBytesUsed();
flutter::PhysicalShapeLayer::DrawShadow(canvas_, path->path(), color,
elevation, transparentOccluder, dpr);
}
Expand Down
3 changes: 0 additions & 3 deletions lib/ui/painting/canvas.h
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,13 @@ class Canvas : public RefCountedDartWrappable<Canvas> {

static void RegisterNatives(tonic::DartLibraryNatives* natives);

size_t external_allocation_size() const { return external_allocation_size_; }

private:
explicit Canvas(SkCanvas* canvas);

// The SkCanvas is supplied by a call to SkPictureRecorder::beginRecording,
// which does not transfer ownership. For this reason, we hold a raw
// pointer and manually set to null in Clear.
SkCanvas* canvas_;
size_t external_allocation_size_ = 0;
};

} // namespace flutter
Expand Down
4 changes: 4 additions & 0 deletions lib/ui/painting/image.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ Dart_Handle CanvasImage::toByteData(int format, Dart_Handle callback) {
}

void CanvasImage::dispose() {
auto hint_freed_delegate = UIDartState::Current()->GetHintFreedDelegate();
if (hint_freed_delegate) {
hint_freed_delegate->HintFreed(GetAllocationSize());
}
image_.reset();
ClearDartWrapper();
}
Expand Down
121 changes: 121 additions & 0 deletions lib/ui/painting/image_dispose_unittests.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
// 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.

#define FML_USED_ON_EMBEDDER

#include "flutter/common/task_runners.h"
#include "flutter/fml/synchronization/waitable_event.h"
#include "flutter/lib/ui/painting/image.h"
#include "flutter/lib/ui/painting/picture.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 {

class ImageDisposeTest : public ShellTest {
public:
template <class T>
T* GetNativePeer(Dart_NativeArguments args, int index) {
auto handle = Dart_GetNativeArgument(args, index);
intptr_t peer = 0;
EXPECT_FALSE(Dart_IsError(Dart_GetNativeInstanceField(
handle, tonic::DartWrappable::kPeerIndex, &peer)));
return reinterpret_cast<T*>(peer);
}

// Used to wait on Dart callbacks or Shell task runner flushing
fml::AutoResetWaitableEvent message_latch_;

fml::AutoResetWaitableEvent picture_finalizer_latch_;
static void picture_finalizer(void* isolate_callback_data, void* peer) {
auto latch = reinterpret_cast<fml::AutoResetWaitableEvent*>(peer);
latch->Signal();
}

sk_sp<SkPicture> current_picture_;
sk_sp<SkImage> current_image_;
};

TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) {
auto native_capture_image_and_picture = [&](Dart_NativeArguments args) {
CanvasImage* image = GetNativePeer<CanvasImage>(args, 0);
Picture* picture = GetNativePeer<Picture>(args, 1);
ASSERT_FALSE(image->image()->unique());
ASSERT_FALSE(picture->picture()->unique());
current_image_ = image->image();
current_picture_ = picture->picture();

Dart_NewFinalizableHandle(Dart_GetNativeArgument(args, 1),
&picture_finalizer_latch_, 0, &picture_finalizer);
};

auto native_on_begin_frame_done = [&](Dart_NativeArguments args) {
message_latch_.Signal();
};

Settings settings = CreateSettingsForFixture();
auto task_runner = CreateNewThread();
TaskRunners task_runners("test", // label
GetCurrentTaskRunner(), // platform
task_runner, // raster
task_runner, // ui
task_runner // io
);

AddNativeCallback("CaptureImageAndPicture",
CREATE_NATIVE_ENTRY(native_capture_image_and_picture));
AddNativeCallback("OnBeginFrameDone",
CREATE_NATIVE_ENTRY(native_on_begin_frame_done));

std::unique_ptr<Shell> shell = CreateShell(std::move(settings), task_runners);

ASSERT_TRUE(shell->IsSetup());

SetViewportMetrics(shell.get(), 800, 600);

shell->GetPlatformView()->NotifyCreated();

auto configuration = RunConfiguration::InferFromSettings(settings);
configuration.SetEntrypoint("pumpImage");

shell->RunEngine(std::move(configuration), [&](auto result) {
ASSERT_EQ(result, Engine::RunStatus::Success);
});

message_latch_.Wait();

ASSERT_TRUE(current_picture_);
ASSERT_TRUE(current_image_);

// Make sure we wait longer than the animator's notify idle.
// which is 51ms in animator.cc.
message_latch_.Reset();
task_runner->PostDelayedTask([&]() { message_latch_.Signal(); },
fml::TimeDelta::FromMilliseconds(60));
message_latch_.Wait();
picture_finalizer_latch_.Wait();

// Force a drain the SkiaUnrefQueue.
message_latch_.Reset();
task_runner->PostTask([&, io_manager = shell->GetIOManager()]() {
io_manager->GetSkiaUnrefQueue()->Drain();
message_latch_.Signal();
});
message_latch_.Wait();

EXPECT_TRUE(current_picture_->unique());
current_picture_.reset();

EXPECT_TRUE(current_image_->unique());
current_image_.reset();

shell->GetPlatformView()->NotifyDestroyed();
DestroyShell(std::move(shell), std::move(task_runners));
}

} // namespace testing
} // namespace flutter
18 changes: 7 additions & 11 deletions lib/ui/painting/picture.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,17 @@ IMPLEMENT_WRAPPERTYPEINFO(ui, Picture);

DART_BIND_ALL(Picture, FOR_EACH_BINDING)

fml::RefPtr<Picture> Picture::Create(Dart_Handle dart_handle,
flutter::SkiaGPUObject<SkPicture> picture,
size_t external_allocation_size) {
auto canvas_picture = fml::MakeRefCounted<Picture>(std::move(picture),
external_allocation_size);
fml::RefPtr<Picture> Picture::Create(
Dart_Handle dart_handle,
flutter::SkiaGPUObject<SkPicture> picture) {
auto canvas_picture = fml::MakeRefCounted<Picture>(std::move(picture));

canvas_picture->AssociateWithDartWrapper(dart_handle);
return canvas_picture;
}

Picture::Picture(flutter::SkiaGPUObject<SkPicture> picture,
size_t external_allocation_size)
: picture_(std::move(picture)),
external_allocation_size_(external_allocation_size) {}
Picture::Picture(flutter::SkiaGPUObject<SkPicture> picture)
: picture_(std::move(picture)) {}

Picture::~Picture() = default;

Expand All @@ -62,8 +59,7 @@ void Picture::dispose() {

size_t Picture::GetAllocationSize() const {
if (auto picture = picture_.get()) {
return picture->approximateBytesUsed() + sizeof(Picture) +
external_allocation_size_;
return picture->approximateBytesUsed() + sizeof(Picture);
} else {
return sizeof(Picture);
}
Expand Down
9 changes: 2 additions & 7 deletions lib/ui/painting/picture.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ class Picture : public RefCountedDartWrappable<Picture> {
public:
~Picture() override;
static fml::RefPtr<Picture> Create(Dart_Handle dart_handle,
flutter::SkiaGPUObject<SkPicture> picture,
size_t external_allocation_size);
flutter::SkiaGPUObject<SkPicture> picture);

sk_sp<SkPicture> picture() const { return picture_.get(); }

Expand All @@ -45,14 +44,10 @@ class Picture : public RefCountedDartWrappable<Picture> {
uint32_t height,
Dart_Handle raw_image_callback);

size_t external_allocation_size() const { return external_allocation_size_; }

private:
Picture(flutter::SkiaGPUObject<SkPicture> picture,
size_t external_allocation_size_);
Picture(flutter::SkiaGPUObject<SkPicture> picture);

flutter::SkiaGPUObject<SkPicture> picture_;
size_t external_allocation_size_;
};

} // namespace flutter
Expand Down
8 changes: 3 additions & 5 deletions lib/ui/painting/picture_recorder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,9 @@ fml::RefPtr<Picture> PictureRecorder::endRecording(Dart_Handle dart_picture) {
return nullptr;
}

fml::RefPtr<Picture> picture =
Picture::Create(dart_picture,
UIDartState::CreateGPUObject(
picture_recorder_.finishRecordingAsPicture()),
canvas_->external_allocation_size());
fml::RefPtr<Picture> picture = Picture::Create(
dart_picture, UIDartState::CreateGPUObject(
picture_recorder_.finishRecordingAsPicture()));

canvas_->Invalidate();
canvas_ = nullptr;
Expand Down
Loading