Repository navigation
[web] Unify ui.ImageFilter between CanvasKit and Skwasm - #191021
Conversation
Part of flutter#175630. Refactors `ui.ImageFilter` to share core business logic between CanvasKit and Skwasm through the `EngineImageFilter` layer. Backend-specific implementations are intentionally kept minimal, with both backends now only implementing `BackendImageFilter` to provide the final native calls.
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the web engine's image filter implementation by introducing a shared EngineImageFilter abstraction to manage the lifecycle and caching of backend-specific filters (CanvasKit and Skwasm), replacing the previous backend-coupled design. The review feedback highlights a performance concern where EngineBlurImageFilter and EngineComposeImageFilter only cache a single filter instance, leading to cache thrashing and redundant allocations when shared across different tile mode contexts. It is recommended to cache these filters in a map keyed by the resolved TileMode and to add tests verifying this multi-context caching behavior.
|
|
||
| if (_cachedFilter != null) { | ||
| _imageFilterFinalizer.detach(_cachedFilter!); | ||
| _cachedFilter!.dispose(); |
There was a problem hiding this comment.
I have two concerns here, but I'm not sure how valid they are, so feel free to push back:
- There's a potential of
_cachedFilterleakage when the framework keeps creating throw-awayImageFilters. The framework doesn't explicitly dispose of image filters AFAICT. - If the same
ImageFilterinstance is used in the same frame with multiple tile modes. The older ones will be disposed before Skia is done with them.
There was a problem hiding this comment.
I found that instances created by CkPaint.toSkPaint are always immediately disposed after use (in canvas.dart). So maybe point 1 is not valid?
There was a problem hiding this comment.
- Even if the framework never explicitly disposes the
ImageFilter, it should still get GC'ed and the finalizer will run for the cached filter. - Even if they are disposed on the Dart side, the Skia side won't dispose of them if they are referenced by a DisplayList or Paint object because they use smart pointers. Disposing on the Dart side only decrements the ref count
| final EngineImageFilter engineFilter; | ||
| if (filter is ui.ColorFilter) { | ||
| convertible = CkColorFilterImageFilter(colorFilter: filter as EngineColorFilter); | ||
| engineFilter = EngineColorFilterImageFilter(colorFilter: filter as EngineColorFilter); |
There was a problem hiding this comment.
This may not be a new issue, but this line of code means we are creating a new native/backend color filter on every frame. We are also not benefiting from any caching.
There was a problem hiding this comment.
Good point, I added caching here.
…ImageFilter, and move filter test
|
Thanks, @mdebbar. PTAL! |
7fcb4be to
0d30aa0
Compare
harryterkelsen
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/191021, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 2 more review(s) in order to merge this PR.
|
mdebbar
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
Part of #175630.
Refactors
ui.ImageFilterto share core business logic between CanvasKit and Skwasm through theEngineImageFilterlayer. Backend-specific implementations are intentionally kept minimal, with both backends now only implementingBackendImageFilterto provide the final native calls.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.