Refactor CanvasKit image ref counting; fix a minor memory leak #22549
Conversation
yjbanov
force-pushed
the
ck-image-ref-counting
branch
from
d913ded to
ca27c58
Compare
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
LGTM. I didn't know we added this clone functionality to Image. What is the use case?
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Overall I think this makes sense, but I have concerns about using raw integer to do refcounting - it'd be very easy to break and makes each caller responsible for implementing the logic to not break it.
| @override | ||
| void dispose() { | ||
| box.delete(); | ||
| if (_disposed) { |
There was a problem hiding this comment.
I think we should probably throw here rather than make it idempotent. On the VM implementation, that's what we do, and in general in the framework we don't allow multiple-disposal.
| if (_disposed) { | ||
| return; | ||
| } |
| if (_refs.isEmpty) { | ||
| void unref(R debugWrapper) { | ||
| assert(!_isDeleted, 'Attempted to unref an already deleted Skia object.'); | ||
| _refCount -= 1; |
There was a problem hiding this comment.
this API frightens me. It'd be very easy for someone to make a mistake with it and call unref multiple times, which would artificially decrease the refcount.
I think we should still maintain a hash set of references, and only allow the caller to remove itself.
There was a problem hiding this comment.
This is handled by the assertion below, but I added an error message so we don't throw an empty AssertionError.
yjbanov
force-pushed
the
ck-image-ref-counting
branch
from
8cb07c1 to
9ef9577
Compare
yjbanov
added
the
waiting for tree to go green
|
This pull request is not suitable for automatic merging in its current state.
|
fluttergithubbot
removed
the
waiting for tree to go green
angjieli
added
the
waiting for tree to go green
…er#22549) * Refactor SkiaObjectBox ref counting * make CkAnimatedImage a Codec * disallow double dispose; better assertion messages
6 participants
Description
Imageclones share the sameSkiaObjectBoxand track them using anint refCount(list of referrers is still available in debug mode, but in release mode we just increment/decrementrefCount).SkAnimatedImagestops implementingui.Image(it doesn't need to).CkAnimatedImageCodec: it wasn't adding value; insteadCkAnimagedImagenow implementsui.Codec.SkData.deleteafter converting it to a localUint8ListintoByteData, fixing a memory leak.Related Issues
Progress towards flutter/flutter#67148.
Tests
Updated relevant existing tests.