AnimatedVisualPlayer: Port WinUI implementation#23889
AnimatedVisualPlayer: Port WinUI implementation#23889morning4coffe-dev wants to merge 4 commits into
Conversation
github-actions
Bot
added
platform/android 🤖
|
Claude finished @morning4coffe-dev's task in 6m 58s —— View job Review complete
SummaryVerdict: fix-first — one blocking-thread issue; the rest are lower-severity. Issues found
O(n) LINQ scans in
What looks good
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { | ||
| try | ||
| { | ||
| var animation = Task.Run(() => CreateAnimation(animationJson)).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Blocking the UI thread — GetAwaiter().GetResult() on a Task.Run call blocks the calling thread while Skottie parses the JSON animation. This method is called from TryCreateAnimatedVisualCore → UpdateContent → OnSourcePropertyChanged, all on the UI thread. For larger Lottie assets this stalls the UI for the full parse duration.
Suggested fix: push the synchronous parse off the UI thread using the same async/dispatch pattern LottieVisualSourceBase already uses for LoadAnimationAsync, or at minimum wrap TryCreate so LottieAnimatedVisual.TryCreate is called from a background thread and the result is marshalled back before TryCreateAnimatedVisual returns — though that would require making TryCreateAnimatedVisual async, which is a deeper change.
At a minimum, CreateAnimation should not be wrapped in Task.Run(...).GetAwaiter().GetResult() — if Skottie's Animation.TryCreate must remain synchronous here, call it directly without the Task.Run wrapper (which just adds overhead without any UI-thread protection) and document the blocking behaviour explicitly.
// Before (blocks UI thread via Task.Run + GetResult):
var animation = Task.Run(() => CreateAnimation(animationJson)).GetAwaiter().GetResult();
// After (call directly on UI thread; document that it blocks, or refactor to async):
var animation = CreateAnimation(animationJson);
| _finalValue = finalValue; | ||
| _duration = duration; | ||
| _totalDuration = iterationBehavior == AnimationIterationBehavior.Forever ? TimeSpan.MaxValue : duration * iterationCount; | ||
| _iterationCount = iterationBehavior == AnimationIterationBehavior.Forever ? Math.Max(iterationCount, 1) : Math.Max(iterationCount, 1); |
There was a problem hiding this comment.
Dead ternary — both branches are identical. The condition iterationBehavior == AnimationIterationBehavior.Forever controls which branch executes, but both produce exactly Math.Max(iterationCount, 1). The Forever branch presumably wanted to avoid clamping to iterationCount (since the loop is infinite, the count doesn't bound the playhead), but the value written to _iterationCount is the same regardless.
GetCurrentProgress already guards the Forever case independently (it uses WrapProgress instead of _iterationCount), so this doesn't introduce a runtime bug — but it's misleading code. Consider simplifying:
// Current (dead ternary):
_iterationCount = iterationBehavior == AnimationIterationBehavior.Forever
? Math.Max(iterationCount, 1)
: Math.Max(iterationCount, 1);
// Suggested:
_iterationCount = Math.Max(iterationCount, 1);
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23889/wasm-skia-net9/index.html |
|
The build 224209 found UI Test snapshots differences: Details
|
|
|
|
Claude finished @morning4coffe-dev's task in 8m 36s —— View job Review complete
SummaryVerdict: fix-first — the blocking-thread call and the new lock inconsistency both need to land before merge; the remainder are lower-severity. Previous inline findings — still unresolved (inline comments already exist on the PR):
New findings from this review pass:
O(n) LINQ scans — pre-existing, not in diff: What landed in the fix commit (
|
|
|
||
| if (animationJson is null) | ||
| { | ||
| return hasLoadFailure ? null! : CreatePendingAnimatedVisual(compositor); |
There was a problem hiding this comment.
null! on a non-nullable return type — TryCreateAnimatedVisualCore is declared as returning IAnimatedVisual2 (non-nullable), but this line and line 131 both return null!, silencing the compiler warning with a null-forgiving operator. The WinUI TryCreateAnimatedVisual contract permits null, so the correct fix is to change the method's return type to IAnimatedVisual2?:
| return hasLoadFailure ? null! : CreatePendingAnimatedVisual(compositor); | |
| return hasLoadFailure ? null : CreatePendingAnimatedVisual(compositor); |
Then update the method signature at line 106:
private IAnimatedVisual2? TryCreateAnimatedVisualCore(Compositor compositor, out object diagnostics, bool createAnimations)and fix line 131 to drop ?? null!. With a ? return type the compiler enforces null checks at every call site instead of relying on ! suppression.
| throw new NotImplementedException(); | ||
| _loadRevoker.Disposable = null; | ||
| _animationDataSubscription.Disposable = null; | ||
| _hasPendingAnimatedVisualInvalidation = false; |
There was a problem hiding this comment.
Lock inconsistency for _hasPendingAnimatedVisualInvalidation and _pendingAnimatedVisualInvalidation — these two fields are read and written inside lock (_stateGate) in RaiseAnimatedVisualInvalidatedAsync and reset inside lock (_stateGate) in RaiseAnimatedVisualInvalidatedCore, but here in ClearLoadState they are reset outside the lock. A background LoadAnimationAsync task can concurrently enter RaiseAnimatedVisualInvalidatedAsync, acquire the lock, observe _hasPendingAnimatedVisualInvalidation == false, and set it to true — then the UI thread runs ClearLoadState and resets it to false without the lock, leaving the coalescing flag corrupted. Wrap these two lines in lock (_stateGate) for consistency with every other write site:
| _hasPendingAnimatedVisualInvalidation = false; | |
| lock (_stateGate) | |
| { | |
| _hasPendingAnimatedVisualInvalidation = false; | |
| _pendingAnimatedVisualInvalidation = Task.CompletedTask; | |
| } |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23889/wasm-skia-net9/index.html |
|
Claude finished @morning4coffe-dev's task in 4m 53s —— View job Review complete
SummaryVerdict: fix-first — the 4 findings from the previous two review passes remain open. No new blocking issues were introduced by the latest two commits. What landed in
|
| Severity | File | Finding |
|---|---|---|
| 🔴 High | LottieVisualSource.skia.cs:65 |
Task.Run(...).GetAwaiter().GetResult() blocks UI thread |
| 🟡 Medium | LottieVisualSourceBase.cs:229 |
_hasPendingAnimatedVisualInvalidation / _pendingAnimatedVisualInvalidation reset outside _stateGate lock in ClearLoadState |
| 🟡 Medium | KeyFrameEvaluator.cs:46 |
Dead ternary — both branches identical |
| 🟢 Low | LottieVisualSourceBase.cs:125 |
null! suppression on non-nullable return type; change to IAnimatedVisual2? |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23889/wasm-skia-net9/index.html |
|
The build 224238 found UI Test snapshots differences: Details
|
github-actions
Bot
added
the
area/build
|
Claude finished @morning4coffe-dev's task in 1m 38s —— View job Review complete
SummaryVerdict: fix-first — the 4 findings from previous review passes remain open. The latest commit introduces no new issues. Latest commit (
|
| Severity | File | Finding |
|---|---|---|
| 🔴 High | LottieVisualSource.skia.cs:65 |
Task.Run(...).GetAwaiter().GetResult() blocks the UI thread during Skottie JSON parse |
| 🟡 Medium | LottieVisualSourceBase.cs:229 |
_hasPendingAnimatedVisualInvalidation / _pendingAnimatedVisualInvalidation reset outside _stateGate lock in ClearLoadState — inconsistent with every other write site |
| 🟡 Medium | KeyFrameEvaluator.cs:46 |
Dead ternary — both branches produce Math.Max(iterationCount, 1) |
| 🟢 Low | LottieVisualSourceBase.cs:125 |
null! suppression on non-nullable return type; change to IAnimatedVisual2? |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23889/wasm-skia-net9/index.html |
|
The build 224270 found UI Test snapshots differences: Details
|
3 participants
GitHub Issue: closes #23881
PR Type:
✨ Feature
What changed? 🚀
Ports
AnimatedVisualPlayerdirectly from Microsoft WinUI commit3cae15f0for the Skia renderer.AnimationControllerplayback-rate, progress, seeking, and commit behavior.IAnimatedVisualSource3/IAnimatedVisual2and Composition-driven rendering.This targets
feature/breakingchanges. The public Uno-only members previously added toIAnimatedVisualSourceare removed in favor of the WinUI contract.PR Checklist ✅
Screenshots Compare Test Runresults.Validation
git diff --check