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

Implement Image.clone for CanvasKit #21555

Merged
merged 7 commits into from
Oct 2, 2020

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Oct 2, 2020

Description

Implements Image.dispose and Image.clone properly so that flutter/flutter#67100 can be relanded.

Creates a RefCountedSkiaObjectBox subclass of SkiaObjectBox to support this.

Related Issues

flutter/flutter#67100
flutter/flutter#66688
flutter/flutter#56482
#21057

Tests

I added the following tests:

Tests that Image.dispose and Image.clone/Image.isCloneOf work properly.

Tests for clone and stacktrace functionality on RefCountedSkiaObjectBox.

return RefCountedSkiaObjectBox._(wrapper, skObject, _refs);
}

/// Removes the reference count for the [skObject].
Copy link
Contributor

Choose a reason for hiding this comment

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

"Removes the reference count" doesn't sound right. Maybe "Decrement the reference count" or "Remove this box from reference list"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and done.

///
/// The [delete] method may be called any number of times. The box
/// will only delete the object once.
class RefCountedSkiaObjectBox extends SkiaObjectBox {
Copy link
Contributor

Choose a reason for hiding this comment

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

The image classes are the only users of SkiaObjectBox. Can we just make SkiaObjectBox ref-counted and not have two classes?

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm


CkAnimatedImage._(this._skAnimatedImage, SkiaObjectBox? boxToClone) {
if (boxToClone != null) {
box = boxToClone.clone(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assert that the image in boxToClone and _skAnimatedImage are the same?

Alternatively, we could split this into two specialized constructors.


CkImage._(this.skImage, SkiaObjectBox? boxToClone) {
if (boxToClone != null) {
box = boxToClone.clone(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as in CkAnimatedImage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these asserts

@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Oct 2, 2020
@fluttergithubbot fluttergithubbot merged commit 89a46af into flutter:master Oct 2, 2020
@dnfield dnfield deleted the image_dispose branch October 2, 2020 20:09
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 2, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 2, 2020
dnfield added a commit to dnfield/flutter that referenced this pull request Oct 2, 2020
Changes since last time:

- Test for CanvasKit image rendering
  (flutter#67176)
- Fix CanvasKit dispose impl
  (flutter/engine#21555)
- Update internal google3 customer with a problematic ImageStream
  Listener impl (cl/335091311)

This reverts commit 473358d.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 2, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2020
dnfield added a commit to flutter/flutter that referenced this pull request Oct 5, 2020
* Reland dispose images when done (#67100)

Changes since last time:

- Test for CanvasKit image rendering
  (#67176)
- Fix CanvasKit dispose impl
  (flutter/engine#21555)
- Update internal google3 customer with a problematic ImageStream
  Listener impl (cl/335091311, cl/335459002)

This reverts commit 473358d.
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Oct 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants