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

Implement image resurrection#22575

Merged
fluttergithubbot merged 1 commit intoflutter:masterfrom
yjbanov:ck-image-resurrection
Nov 20, 2020
Merged

Implement image resurrection#22575
fluttergithubbot merged 1 commit intoflutter:masterfrom
yjbanov:ck-image-resurrection

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Nov 18, 2020 •
edited
Loading

Description

  • Teach SkiaObjectBox to resurrect its underlying Skia object and make CkImage resurrectable. This should plug leaking images that are not held onto by pictures. Fixing pictures will be a future PR.
  • Consolidate finalization logic into an abstract Collector class with a ProductionCollector implementation for production usage, and TestCollector implementation for tests.
  • Add a setUpCanvasKitTest that performs validation and clean-up of Skia objects, making sure test state does not leak from test to test (GC is unpredictable, sometimes causing one test collecting objects created by another tests, resulting in hard-to-explain test failures).

Related Issues

Progress towards flutter/flutter#67148.

Tests

Updated image_test.dart and skia_object_cache_test.dart.

@google-cla google-cla bot added the cla: yes label Nov 18, 2020
@yjbanov yjbanov marked this pull request as draft November 18, 2020 16:58
@yjbanov
Copy link
Contributor Author

yjbanov commented Nov 18, 2020

Sorry, sent for review prematurely. I haven't made CkAnimatedImage resurrectable.

@yjbanov yjbanov force-pushed the ck-image-resurrection branch from 3544f3c to 35f24ab Compare November 19, 2020 22:22
@yjbanov yjbanov force-pushed the ck-image-resurrection branch from 35f24ab to 9038a9f Compare November 20, 2020 00:26
@yjbanov yjbanov marked this pull request as ready for review November 20, 2020 00:27
@yjbanov
Copy link
Contributor Author

yjbanov commented Nov 20, 2020

This is ready for review.

harryterkelsen
harryterkelsen approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM

@yjbanov yjbanov 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 Nov 20, 2020
@fluttergithubbot fluttergithubbot merged commit b502fcb into flutter:master Nov 20, 2020
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 24, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 24, 2020
@engine-flutter-autoroll
b502fcb Implement image resurrection (flutter/engine#22575)
a548abf
chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020
@yjbanov @chaselatta
Implement image resurrection (flutter#22575)
94ce0ef
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@harryterkelsen harryterkelsen harryterkelsen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yjbanov @harryterkelsen @fluttergithubbot