Visitar URL original
fix(imaging): Release a decoded image when nothing displays it by mikernet · Pull Request #25124 · unoplatform/uno · GitHub
Skip to content

fix(imaging): Release a decoded image when nothing displays it - #25124

Merged
13 commits merged into
unoplatform:masterfrom
mikernet:dev/mikernet/image-release-25114
Oct 28, 2026
Merged

13 commits merged into
unoplatform:masterfrom
mikernet:dev/mikernet/image-release-25114

Conversation

@mikernet

@mikernet mikernet commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

GitHub Issue: closes #25114, and fixes the Image part of #25596 on master

PR Type:

🐞 Bugfix

What changed? 🚀

On Skia, decoded pixels were only freed through a chain of finalizers. Paging through large images with the bitmap cache off could hold hundreds of megabytes, and WebAssembly memory grew to each peak and never shrank (#25114). This PR releases decoded images that nothing displays, as WinUI does: WinUI frees an image's surfaces once it has been off the tree for 1 s (sc_imageSurfaceWrapperReleaseDelayMilliseconds, checked by CCoreServices::TrimMemoryOnSurfaceWrappers).

  • Release after 1 s. When a source loses its last subscriber, the decoded data it owns is released 1 s later, at the end of a layout pass, unless something subscribes again. An element that is moved, re-templated or recycled in the meantime keeps its pixels.
  • Byte limit. While undisplayed images hold more than FeatureConfiguration.Image.MaxUndisplayedImageBytes, the oldest go first. With 0, they go after the next layout pass.
  • Replaced pixels stay until the replacement is drawn, as on WinUI, so changing a source doesn't blank the Image in between.
  • Cancelled and superseded loads release what they decoded.
  • Image and ImageBrush:
    • Image drops its subscription when unloaded and subscribes again when loaded.
    • ImageBrush keeps one subscription across UriSource changes.
  • Events as on WinUI:
    • ImageOpened is raised once per source, not again after a re-decode.
    • A failed source shows nothing.
  • Unchanged:

Why a byte limit

WinUI needs no bound: it decodes at the rendered size, and it runs on Windows desktops, without the memory limits of the browser and mobile targets. Uno decodes in full unless DecodePixelWidth/DecodePixelHeight is set: a 4000×2667 photo shown 1000 pixels wide holds 43 MB, not 3 MB. So paging can leave hundreds of megabytes waiting within that second:

  • WebAssembly memory never shrinks, and the app crashes past 2 GB. Mobile browsers also discard memory-heavy background tabs, so a user who switches to another app (to copy something, say) can come back to a reloaded page and lose unsaved changes.
  • iOS terminates an app over its memory footprint, even in the foreground. Android kills large background processes first.

The defaults are 128 MB on desktop, 64 MB on Android, iOS and tvOS, and 32 MB on WebAssembly. Apps can change them (feature-flags.md).

Open question: would you prefer different defaults, or one value everywhere?

Behavior to be aware of

  • An Image that shows a source whose data was released (it comes back, or it is another Image, such as a recycled item) shows nothing until the image is decoded again, but already has the image's size, as on WinUI. A cache-off URI downloads again, and a stream source decodes the clone it took.
  • An Image that was never added to the tree doesn't open its source, so its BitmapImage raises no ImageOpened until the Image is loaded. On master it did. This matches WinUI: When_Image_Never_In_Tree_Then_Source_Not_Opened passes there too. An app that preloads through an off-tree Image now sees the image open once it is shown.
  • A brush given another ImageSource shows nothing until that source opens. It used to keep the old image.
  • An Image away for 1 s during a download cancels an uncached load.
  • A collapsed Image keeps what it last drew until it is measured or leaves the tree.

Also fixes: long-lived sources keeping Images alive (#25596)

On master, Image subscribes to its source until its Source changes, and the source keeps that callback in a strong list. The callback captures the Image. So a source that outlives its images, such as an icon in a static field or in app resources, keeps every Image that ever showed it alive, with everything those Images reference. This PR has Image subscribe only while it's loaded, so the source lets go once the Image leaves the tree.

When_Shared_Source_Outlives_Unloaded_Images_Then_Images_Collected covers a stream bitmap, a cached UriSource and an SVG source, each shared by two Images. It fails on master and passes here (the review's probes checked the cached case on master). Two related paths are not changed here:

Tests

Given_Image_Release (Skia) checks each release directly. Tests that need the loopback image server, or that hold the UI thread, run on desktop only. Two tests in Given_Image use only the public API, and also pass on WinUI:

  • When_Source_Shown_Again_While_It_Opens_Again_Then_AutoSized_Image_Keeps_Its_Size (the same Image, and another one)
  • When_Image_Never_In_Tree_Then_Source_Not_Opened
Test list
  • Waits:
    • When_Unloaded_Then_Decoded_Image_Released
    • When_Moved_Before_Next_Layout_Pass_Then_Decoded_Image_Kept
    • When_Reentered_Within_Release_Delay_Then_Decoded_Image_Kept
    • When_Unused_Past_Release_Delay_Then_Released
    • When_Moved_During_Layout_After_Loaded_Then_Decoded_Image_Kept
    • When_Undisplayed_Images_Exceed_Limit_Then_Oldest_Released_First
    • When_Over_Limit_Behind_A_Download_Then_Undisplayed_Image_Still_Released
    • When_Replaced_Image_Released_After_Pass_Then_Older_Waiting_Image_Kept
    • When_Many_Images_Wait_Then_Each_Starts_Its_Wait_At_Once
  • Replaced pixels:
    • When_Source_Changed_While_Displayed_Then_Previous_Pixels_Kept_Until_New_Shown
    • When_UriSource_Changed_Then_Previous_Decoded_Image_Released
    • When_Source_Changed_During_Notification_Then_Skipped_Subscriber_Keeps_Its_Pixels
    • When_Source_Changed_During_Successive_Notifications_Then_Only_Shown_Pixels_Kept
    • When_Source_Changed_By_Last_Subscriber_Then_Previous_Pixels_Released
    • When_Stream_Set_Again_From_Its_Completion_Then_Previous_Frames_Released
    • When_Many_Images_Share_A_Replaced_Source_Then_Previous_Image_Kept_Until_Last_Draws_Replacement
  • Images that leave and return:
    • When_Image_Showing_Replaced_Pixels_Leaves_Then_They_Wait_Like_Undisplayed_Ones
    • When_Image_Returns_After_Replacement_Arrived_Then_Replaced_Pixels_Released
    • When_Image_Returns_After_Source_Cleared_Then_Replaced_Pixels_Not_Kept
    • When_Cached_Image_Returns_After_Source_Settled_Then_Shows_Nothing
    • When_Moved_Between_Delivery_And_Measure_Then_Image_Keeps_Drawing_Its_Pixels
    • When_Unloaded_Between_Delivery_And_Measure_Then_Drawn_Pixels_Wait_For_It
    • When_Images_Leave_With_The_Same_Pixels_Then_Each_Can_Come_Back_With_Them
  • Brushes and re-entrancy:
    • When_Rendered_Brush_Changes_UriSource_Then_It_Keeps_Drawing_Until_Replaced
    • When_Rendered_Brush_ImageSource_Replaced_Then_Previous_Image_Not_Drawn
    • When_Cached_Open_Handler_Changes_Source_Then_Image_Releases_Replacement_On_Unload
    • When_Cached_Open_Handler_Changes_Brush_Source_Then_Brush_Draws_Replacement
    • When_Cached_Open_Handler_Unloads_And_Disposes_Then_Image_Opens_On_Reload
  • Superseded opens and events:
    • When_Requester_Cancelled_After_Decode_Then_Abandoned_Surface_Released
    • When_Stream_Load_Superseded_After_Decode_Then_Abandoned_Surface_Released
    • When_RenderTargetBitmap_Open_Superseded_Then_Abandoned_Surface_Released
    • When_PixelWidth_Handler_Changes_Source_Then_Replacement_Raises_ImageOpened
    • When_PixelWidth_Handler_Switches_To_Cached_Source_Then_Size_Is_The_Replacement
    • When_Source_Fails_Then_Previous_Image_Not_Shown
    • When_Failed_Image_Reloaded_Then_Source_Not_Retried
    • When_Decoded_Again_After_Release_Then_ImageOpened_Raised_Once
    • When_Cached_Image_Unloaded_Then_Shown_Again_Without_Reopen
  • Sources and disposal:
    • When_Source_Replaced_Then_Previous_Decoded_Image_Released
    • When_Source_Changed_While_Unloaded_Then_Decoded_On_Load
    • When_Cache_Disabled_Then_Cached_Surface_Not_Released
    • When_Stream_Disposed_After_SetSourceAsync_Then_Decoded_Again_On_Reload
    • When_Disposed_While_Displayed_Then_Opened_Again
    • When_Disposed_While_Downloading_Then_Subscriber_Still_Opened
    • When_Listener_Unsubscribes_Another_During_Notification_Then_It_Is_Not_Called
    • When_Listener_Changes_Source_During_Notification_Then_Stale_Data_Not_Delivered

Each test added for a review finding fails without its fix.

Runs:

  • Skia desktop, 23 image-related classes: 298 passed and 1 failed, with 5 skipped by platform and 1 inconclusive (it needs 100% scaling).
  • WebAssembly, 9 image classes: 104 passed and 1 failed, with 30 desktop-only tests skipped and 2 inconclusive.
  • WinUI: the two Given_Image tests above pass.

The one failure on both, When_WriteableBitmap_Assigned_With_Data_Present, also fails with master's code on this machine, which runs at 125% display scaling: the test finds a blue pixel at the corner of the rectangle it checks.

The history was squashed into logical commits after the review rounds; commit SHAs in earlier comments refer to the previous history (kept at mikernet:dev/mikernet/image-release-25114-presquash).

PR Checklist ✅

@mikernet

mikernet commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

For a servicing/6.8 backport: ported onto the 6.8.0-dev.212 commit at Singulink@c13eba7251 and Singulink@f443e7c102 (branch Singulink:custom/6.8). On 6.8 the surface type is SkiaCompositionSurface, the Skia Image partial gets the load/unload overrides, and the WebAssembly decode is changed from a pinned managed array to SKImage.FromPixelCopy, which master already has. This is the build the measurements in #25114 come from.

@github-actions github-actions Bot added the area/skia ✏️ Categorizes an issue or PR as relevant to Skia label Oct 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Surface ownership and lifecycle gaps can still leak pixels or release shared cached surfaces prematurely.

5 open findings
What changed in this PR

Adds deterministic release and reload behavior for decoded Skia image surfaces.

Changes:

  • Releases unused decoded frames and textures.
  • Manages image subscriptions across unload/reload and cancellation.
  • Adds Skia runtime regression tests.

Review outcome: changes required due to five correctness findings involving ownership, cancellation, and lifecycle handling.

File Description
BitmapImage.cs Tracks cache ownership and cancellation.
ImageSource.crossruntime.cs Releases unreferenced image data.
ImageData.cs Adds surface test instrumentation.
Image.cs Manages subscriptions during unload/reload.
Given_Image_Release.cs Tests deterministic image release.
CompositionImageSurface.skia.cs Adds deterministic frame release.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI balanced review requested due to automatic review settings October 8, 2026 16:18
@mikernet
mikernet force-pushed the dev/mikernet/image-release-25114 branch from c84ccfe to 8620cb9 Compare October 8, 2026 16:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Cached downloads remain tied to one requester’s cancellation token, causing shared-load failures and abandoned-surface retention.

3 open findings
4 resolved since last review

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI balanced review requested due to automatic review settings October 8, 2026 17:07
@mikernet
mikernet force-pushed the dev/mikernet/image-release-25114 branch from 8620cb9 to 5cdea60 Compare October 8, 2026 17:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

LRU eviction can orphan surfaces permanently marked as cache-owned, leaving them dependent on finalization.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

mikernet added a commit to mikernet/uno that referenced this pull request Oct 8, 2026
…ancellation (port of unoplatform#25124 amendments)

Ports the amendments made to unoplatform#25124 on master onto the 6.8 image release
fix (c13eba7, f443e7c).

Ownership travels with the result: ImageData gains IsShared and
AsShared(), a BitmapImage marks the result of a load that went through
the shared bitmap cache as shared on the task every sharer awaits, and
ReleaseSurface skips shared data itself. The source-wide SharesImageData
flag and the ReleaseAbandonedImageData partial are gone, so a cached
surface is never released by one of its sources no matter which open of
that source handles it, and a URI requester cancelled out of its wait
always attaches the releasing continuation (a no-op for shared data).
ReleaseImageData cancels the open in flight before releasing. On 6.8
ReleaseSurface lives in ImageSource.crossruntime.cs with a Skia-only
body, since that file also compiles for WebAssembly and Reference;
ImageSource.skia.cs is removed.

The cache is consulted only when caching is enabled and the source does
not ignore it (already the 6.8 shape), so a source that owns its decode
is never handed a shared one.

The Skia Image subscribes to a bitmap source only while loaded, and
OnLoaded subscribes again, so a recycled container's new source is not
decoded off-tree.

ImageSource.UnloadImageData releases the decoded data instead of just
dropping it, so a UriSource change on an opened bitmap releases the
previous decode (native Android/iOS keep the plain reset).

A stream load superseded while decoding releases the surface it produced
and drops the result instead of raising ImageOpened and taking the pixel
size of a stream the source no longer has.

The Given_BitmapImage runtime tests load their Images through a loaded
panel, as an unloaded Image no longer subscribes.

unoplatform#25114
mikernet added a commit to mikernet/uno that referenced this pull request Oct 8, 2026
… tip

The test counters are incremented with Interlocked (the decode and the releasing continuation run on
thread-pool threads), and the cache test unloads both cache-on requesters before asserting that the shared
surface is not released, as the unoplatform#25124 branch does now that a cached entry is only evicted when its load fails.
@MartinZikmund

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@nventive-devops

Copy link
Copy Markdown
Contributor

The build 238500 found UI Test snapshots differences: skia-linux-screenshots: 118, skia-windows-screenshots: 1149

Details
  • skia-linux-screenshots: 118 changed over 2458

    🚨🚨 Comparison Details (first 20) 🚨🚨
    • CalendarView_Theming.png-dark
    • CalendarView_Theming.png
    • CompositionEffectBrush.png-dark
    • CompositionEffectBrush.png
    • CompositionNineGridBrush_Source_Changes.png-dark
    • CompositionNineGridBrush_Source_Changes.png
    • ButtonClippingTestsControl.png-dark
    • ButtonClippingTestsControl.png
    • Buttons.png-dark
    • Buttons.png
    • ClipboardTests.png-dark
    • Focus_FocusVisual_Properties.png-dark
    • Focus_FocusVisual_Properties.png
    • DisplayInformation.png-dark
    • ElementLevelTheme.png-dark
    • ElementLevelTheme.png
    • DropDownButtonPage.png-dark
    • Examples.png
    • Gamepad_CurrentReading.png-dark
    • Gamepad_Enumeration.png-dark
  • skia-windows-screenshots: 1149 changed over 2474

    🚨🚨 Comparison Details (first 20) 🚨🚨
    • Attributed_text_Simple.png
    • AutoBorderStretchwithleftmargin.png-dark
    • Basics.png-dark
    • Basics.png
    • BitmapIcon_Sizing.png-dark
    • BitmapIcon_Sizing.png
    • Border_Simple_No_Background_With_TextBox.png-dark
    • Border_Simple_No_Background_With_TextBox.png
    • Brushes_ImplicitConvert.png-dark
    • Brushes_ImplicitConvert.png
    • ButtonClippingTestsControl.png-dark
    • ButtonClippingTestsControl.png
    • Button_Opacity_Automated.png-dark
    • Button_Opacity_Automated.png
    • Capture with overlap.png-dark
    • Capture with overlap.png
    • ClipboardTests.png-dark
    • ColorAnimationUsingKeyFrames_Fill.png-dark
    • ColorAnimationUsingKeyFrames_Fill.png
    • ComboBox_Legacy.png-dark

@morning4coffe-dev morning4coffe-dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified: the PR's own commit 5cdea60 (8 files) against its base, read at head via refs/uno-review/25124, plus CI rollup, the 8 review threads, linked issue #25114, and the WinUI reference (imagesource.cpp LeaveImpl). Not built or run.

Verdict: COMMENT

Findings (head 5cdea60, ranked by severity):

  1. [Medium] The default configuration is not covered. FeatureConfiguration.cs:232 sets EnableBitmapImageCache = true. With the cache on, URI BitmapImage loads are cache-owned (BitmapImage.cs:279-281, AsShared at :309) and the source never releases them; the surface lives until LRU eviction and then finalization. The PR body says this, but issue #25114 reports growth measured with the cache disabled and its Expected behavior is deterministic release when nothing displays the image. Please confirm the issue accepts a cache-off-only fix, or extend release to shared entries.

  2. [Medium] Re-entering the tree blanks the Image and re-decodes. Image.cs:186-196 clears the sprite brush and drops the subscription on unload. On reload, the source's data was cleared (ImageSource.crossruntime.cs:99-104), and BitmapImage only overrides TryOpenSourceAsync (BitmapImage.cs:166), so Open() takes the DispatcherQueue path (crossruntime:145-170). Before this PR, Subscribe delivered the retained _imageData synchronously (crossruntime:69-71). Consequences: every Image that re-enters the tree shows nothing for at least one dispatcher turn, including cached sources; with the cache off, a URI source is downloaded again. The PR body says "decodes again" but mentions neither the blank frame nor the re-download.

  3. [Medium] Screenshot evidence is unresolved. The bot comment (IC_kwDOB-UUPs8AAAABaq0ntg, build 238500) reports 118 changed images on skia-linux-screenshots and 1149 on skia-windows-screenshots. The Screenshot Comparison check concludes SUCCESS, so it does not gate, and the checklist item "Validated PR Screenshots Compare Test Run results" is unchecked. Change 2 alters when images attach to the tree and could shift captured frames. Please show these diffs are baseline noise from #25101 and not from this PR.

  4. [Medium-Low] Dispose and UnloadImageData now cancel an in-flight open even when subscribers remain. ImageSource.cs:217-222 calls ReleaseImageData(), which sets _opening.Disposable = null (crossruntime:99-104), cancelling the open. Callers: ImageSource.Dispose (ImageSource.cs:194), BitmapImage.cs:44, SvgImageSource.cs:60. The two OnUriSourceChanged paths re-open through InvalidateSource() under UNO_REFERENCE_API (true for Skia and WASM, Uno.CrossTargetting.targets:44-59). A plain Dispose() with subscribers still attached never re-opens, so they never receive OnOpened. Before the PR the in-flight open completed and notified. Suggested fix: re-open when _subscriptions.Count > 0 in UnloadImageData, or separate cancel from release.

  5. [Low] SVG is not covered by the release. InvalidateSource() opens an SvgImageSource even with zero subscribers (crossruntime:128), so the result stays in _imageData with no subscriber. ReleaseSurface only handles CompositionImageSurface (crossruntime:112), so CompositionSvgSurface is never released deterministically. The claim that the source releases "as soon as its last subscriber leaves" does not hold for SVG. Either skip the zero-subscriber open for SVG or document the gap.

  6. [Low] Notification snapshot can hand released data to listeners. OnOpened (crossruntime:183-197) iterates a copy of _subscriptions. If a listener drops the last subscription, ReleaseImageData releases data mid-loop, and later snapshot entries (including ones already unsubscribed) still receive it. Suggest checking membership before each callback and deferring the release until after the loop.

  7. [Low] Release ordering is inconsistent with the stated invariant. OnUnloaded clears the sprite before releasing (Image.cs:192-195). OnSourceChanged releases first (Image.cs:33) and clears the brush after (Image.cs:35). Harmless on the UI thread, but swap the two lines so both paths match the comment's rule.

  8. [Low, question] Pending render. The paint path takes GetTexture() and DrawImage (CompositionSurfaceBrush.skia.cs:127-139). On Skia the texture is an SKImage from FromPixelCopy (SkiaDrawingFactory.cs CreateTexture; SkiaTexture.Free calls Image.Dispose). SKImage is ref-counted, so draws already recorded keep their pixels. I did not confirm that painting runs only on the UI thread; if it can run elsewhere, ReleaseFrames (CompositionImageSurface.skia.cs:133) racing GetTexture is a use-after-dispose. Please state the thread guarantee. The only release off the UI thread is the cancelled-requester continuation (BitmapImage.cs:353-357, TaskScheduler.Default); it is safe because no requester holds that result. WebGpu texture lifetime was not checked.

  9. [Low] Test seams in shipped code: ImageData.cs:34-43 (Interlocked counter on every decode), crossruntime:91-94 (ReleasedSurfacesForTesting), BitmapImage.cs:153-158 (GetCachedImageDataTaskForTesting). Prefer InternalsVisibleTo. Given_Image_Release.cs asserts > deltas on these global counters, which can pass due to unrelated releases; prefer per-instance observation.

  10. [Info] Review threads: 8 total, 5 unresolved and not outdated. I checked each against head and they look addressed: IsLoaded guard (Image.cs:44), UnloadImageData now releases (ImageSource.cs:221), stream cancellation gate (BitmapImage.cs:242-248), shared load on CancellationToken.None (BitmapImage.cs:299), AsShared inside the task (BitmapImage.cs:309). Resolve them. The LRU thread is partly valid: an entry evicted while in flight keeps its shared mark, so its surface falls back to finalization. That is not a correctness bug, but it is not deterministic release either.

  11. [Info] WriteableBitmap.cs:58-69 now copies the full pixel buffer on every subscribe-from-empty, because TryOpenSourceSync always calls CopyPixels. Acceptable; note the cost.

  12. [Info] Checked and consistent: ImageBrush keeps its subscription, so ImageDataCache (ImageBrush.crossruntime.cs:10, :18-34) is not released while attached. The LRU Remove(key, value) matches its neighbours. WinUI imagesource.cpp LeaveImpl calls RegisterForCleanupOnLeave (about lines 439-444), supporting release-on-leave; WinUI defers cleanup to a list, while this PR releases synchronously.

Not validated: no build, no runtime tests, no screenshot run. CI rollup is all SUCCESS (Desktop Linux and Windows Skia runtime tests, WASM Skia runtime tests, iOS and Android sample builds). The Given_Image_Release results and "fails on master without the release" are author-reported.

Copilot AI balanced review requested due to automatic review settings October 16, 2026 13:50
@mikernet

mikernet commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

I merged master into the branch: #25100 and #25101 are in it, so the PR's diff is its own change again. The changes for this review are 12f8b07..6136bda.

  1. This PR covers the decodes a source owns, which is what kept growing in the cache-off measurements of [Skia] A decoded image's pixels are released only through a finalizer chain, never when the image leaves the screen #25114. With the cache on, the LRU (MaxBitmapImageCacheCount, 100 entries) keeps recent decodes for reuse, and an entry it evicts still falls back to finalization (your point in 10, [Skia] A bitmap cache entry evicted by the LRU leaves its shared decoded surface to finalization #25128). A follow-up PR stacked on this one will replace that decoded-pixel LRU: a decode stays shared by the sources that hold it, a shared decode nothing displays waits on the same pending-release list as the owned ones from 2 (1 s, plus the byte limit, oldest first), and a byte-bounded cache of the encoded bytes keeps a re-decode from downloading again. The same byte limit then covers cache-owned decodes, which addresses [Skia] A bitmap cache entry evicted by the LRU leaves its shared decoded surface to finalization #25128. If you'd rather have it in this PR, I can fold it in.

  2. Fixed in 0bf2d0d, 11d1d6a and bfa953b.

    • (a) A result shared through the cache now stays on its source when the last subscriber leaves. The source never releases shared data, so clearing it only forced a re-open. An Image that re-enters the tree with the cache on gets it back synchronously when it subscribes again: no dispatcher turn, no re-open, no second ImageOpened.
    • (b) An owned result is no longer released on the last unsubscribe. As you said in 12, WinUI waits: it frees the surfaces of an image that left the tree once it has been off it for sc_imageSurfaceWrapperReleaseDelayMilliseconds, 1 s, because freeing them at once made images that come back take several frames to reload (corep.h). This PR does the same: the source puts what it decoded on a pending-release list (PendingImageReleases), which releases it after 1 s unless something subscribes again first. WinUI's wait has no size bound (TrimMemoryOnSurfaceWrappers only checks the time), but WinUI decodes to the rendered size by default (DecodeToRenderSize), so a 4000×2667 slide shown 1000 pixels wide holds under 3 MB there, while Uno decodes it in full unless DecodePixelWidth is set, about 43 MB. So the waiting images are also bounded in bytes, counting 4 bytes per pixel of their decoded frames and of the texture drawn from them: while they hold more than FeatureConfiguration.Image.MaxUndisplayedImageBytes, the oldest are released first, and a limit of 0 releases each of them at the end of the next layout pass. doc/articles/feature-flags.md describes the setting.
    • Releases run at the end of a layout pass, after CoreServices.UpdateLayoutForAllRoots raises Loaded, and only for images let go before that pass began, so an element re-parented, re-templated or recycled before the next pass, or by the layout that follows Loaded within one, keeps its pixels even when it exceeds the limit on its own. A tick is requested whenever a release is due (a timer for the 1 s), so they also happen on an idle UI, and a busy UI cannot starve them the way it can a Low-priority dispatcher item. A subscriber that arrives while an open is still in flight waits for it rather than restarting it.
    • The byte limit matters most where memory is tightest: on WebAssembly the linear memory is capped at 2 GB and never shrinks, so a peak is paid for the rest of the session and going over the cap is an out-of-memory crash, and mobile browsers discard memory-heavy background tabs eagerly. iOS has no swap: an app over its limit is terminated, and under memory pressure background apps with the largest footprint go first. Android's low-memory killer prefers background and large processes, and decoded pixels are native memory outside the managed heap's view. Desktop has far more headroom, hence its larger default.
    • Open question: the limit defaults to 128 MB on desktop, 64 MB on Android, iOS and tvOS and 32 MB on WebAssembly; would you prefer different values, or one value everywhere?
    • (c) An owned result that was released (nothing displayed it past the delay or the limit) decodes again on re-entry, and a cache-off URI downloads again; the Image is blank until the decode lands. The body's "Behaviour to be aware of" says so.
    • (d) WinUI raises ImageOpened once per source: CImageSource::FireImageOpened checks m_publicBitmapEventFired, which only ResetCallbackEvents re-arms when the URI or stream is set (imagesource.cpp). BitmapImage.ImageOpened and Image.ImageOpened are now raised once per source: a re-decode after a release raises neither, a new UriSource or stream raises both, and a failed re-decode still raises ImageFailed.
    • I also fixed a regression of 5cdea60 in the same area (11d1d6a): changing what a displayed source holds released the pixels still being drawn, so a new UriSource with the cache off, SetSourceAsync or RenderAsync on a displayed bitmap, or Dispose, blanked it until the new decode landed (Image.Source itself still clears at once, as on WinUI). The replaced data now stays until the subscribers show the new data, as WinUI keeps the surface while it switches URIs (CImageSource::ReloadImage). In 1309493, a RenderTargetBitmap open superseded before its continuation ran (a new RenderAsync of a live preview, Dispose) now releases the surface it built: the continuation was bound to the open's token, so its result never reached Open.
    • Stream sources: BitmapSource.SetSourceCore keeps a clone, and each decode clones that clone. RandomAccessStreamOverStream.CloneStream and InMemoryRandomAccessStream.CloneStream copy into a new MemoryStream, so disposing the app's stream does not affect a re-decode (When_Stream_Disposed_After_SetSourceAsync_Then_Decoded_Again_On_Reload). A FileRandomAccessStream clone reopens its file by path, so that one needs the file to still exist.
    • 5cdea60's message says the Image "clears its sprite first so nothing draws a released surface". That is no longer true, see 7; 0bf2d0d's message has the current account.
  3. Baseline noise, not this PR. PRs by another author (Martin Zikmund) built in the same window show the same counts: fix(automation): Align ListViewBaseAutomationPeer.GetChildrenCore return type with WinUI #25141 (build 238473) had 118 linux / 1165 windows, fix(pivot): Measure PivotItem content with the Pivot's width (port PivotPanel) #25143 (238482) 118 / 1146 and fix(text): Shape variable fonts at the instance's axis position #25154 (238589) 118 / 1145, against 118 / 1149 for this PR's build 238500.

  4. Fixed in de163b2. Dispose now opens the source again when subscribers remain; they keep their previous pixels until it delivers (When_Disposed_While_Displayed_Then_Opened_Again, When_Disposed_While_Downloading_Then_Subscriber_Still_Opened). Dispose does not change what the source shows, so this raises no second ImageOpened. I put it in Dispose rather than UnloadImageData because the other callers (BitmapImage and SvgImageSource on a UriSource change, SvgImageSource.SetSourceAsync) call InvalidateSource right after it, which re-opens once the new source is set. Re-opening inside UnloadImageData would run before InitFromUri: for SvgImageSource that starts a load of the previous AbsoluteUri, which TryOpenSvgImageData then reuses for the new one.

  5. The SVG surface is a parsed vector document (CompositionSvgSurface replays it each frame), not decoded pixels, and SvgImageSource opens without subscribers by design and retains _svgSurface itself, so clearing _imageData freed nothing and only forced a re-open. The pending releases now skip SvgImageSource (0bf2d0d): its document lives until its source changes or it is disposed, as before. The body scopes the release to decoded bitmaps.

  6. Fixed in 670ede9. OnOpened calls a listener from the snapshot only if it is still subscribed, and stops once a listener has changed the source (which drops the data being delivered); the new open delivers its data to every subscriber. A release from a last unsubscribe inside the loop is deferred anyway since 2(b). Covered by When_Listener_Unsubscribes_Another_During_Notification_Then_It_Is_Not_Called and When_Listener_Changes_Source_During_Notification_Then_Stale_Data_Not_Delivered.

  7. Fixed in 0bf2d0d: OnSourceChanged clears the sprite before dropping the subscription. OnUnloaded no longer clears the sprite: an Image that comes back in time keeps its pixels even if a frame is recorded before Loaded is raised, and an unloaded sprite is not drawn. A sprite left on a surface that was released draws nothing (CompositionImageSurface.Size is null without frames, so CompositionSurfaceBrush.Paint returns before GetTexture), and ReleaseFrames raises a property change that reaches the sprite through its brush (PropagateChanged → Visual.OnPropertyChangedCore → InvalidatePaint), which drops its cached recording, so nothing keeps the texture.

  8. Recording, where GetTexture and DrawImage run, is on the UI thread: CompositionTarget.Render and OnRenderFrameOpportunity call NativeDispatcher.CheckThreadAccess() (CompositionTarget.Rendering.cs:273, CompositionTarget.RenderScheduling.cs:262), and CompositionSurfaceBrush.Paint (CompositionSurfaceBrush.skia.cs:127) is reached from that recording. ReleaseFrames also runs on the UI thread: the pending releases run at the end of a layout pass, InvalidateSource/UnloadImageData are called from property changes, and Open's continuation is dispatched. The off-thread exception is the one you found, the cancelled-requester continuation (BitmapImage.cs:363), on an uncached result no source holds. So GetTexture and ReleaseFrames never race. What can run on another thread is presenting a recorded frame, and a recording holds its own reference to every texture it drew: on Skia the session draws into an SKPictureRecorder canvas (SkiaDrawingSession.cs:328), so the SKPicture refs the SKImage and SkiaTexture.Free only drops the surface's reference; on WebGPU, WebGpuCommandRecorder.TrackTexture (WebGpuCommandRecorder.cs:561) AddRefs each drawn texture until the recording is disposed, and WebGpuTexture.Free (WebGpuDrawingFactory.cs:338) defers the GPU release to the render thread's next frame start. A present in flight keeps its pixels on both.

  9. Done in 6136bda. ImageData.CompositionSurfacesCreatedForTesting and ImageSource.ReleasedSurfacesForTesting are gone. The tests observe one instance: the surface the source delivers to subscribers (through the internal Subscribe) and whether its frames were released, and, for results that never reach the source (a superseded or cancelled decode), a test-only IImageEncoderDecoder wrapper installed through the existing internal ImageEncoderDecoder.Current setter, which records the images each decode produced. The tests set the delay through PendingImageReleases.ReleaseDelay, an internal override like WinUI's m_testOverrideImageSurfaceWrapperReleaseDelay. BitmapImage.GetCachedImageDataTaskForTesting came with fix(imaging): Keep a shared BitmapImage download alive when one requester cancels, drop failed loads from the cache #25101 (already merged): a static lookup with no cost per decode, used by its cache-eviction test.

  10. All eight threads are resolved now. On the LRU one: agreed, an evicted entry is not released deterministically; see 1.

  11. Noted: WriteableBitmap copies its full buffer again when it is shown after a release; the body lists it.

  12. You're right, and my reading of LeaveImpl was wrong: WinUI does release an image's surfaces after it leaves the tree, about 1 s later, through TrimMemoryOnSurfaceWrappers (xcpcore.cpp) at the end of every render walk. For a BitmapImage the hardware surface holds the decoded pixels (a software copy is kept only without encoded bits, as for WriteableBitmap), and WinUI decodes again from the encoded data its ImageCache keeps. This PR now follows that timing (2(b)), with the byte limit on top because Uno decodes at full size, and the body and commit text describe WinUI's lifetime that way.

Tests: 12 of the new tests were run against the previous head (5cdea60) and fail there (the stream test, which guards existing behaviour, passes), the two added last fail with their fix taken out, and the release tests fail with the pending releases disabled. On this head, Skia desktop passes the image, imaging, icon, panel, path, PersonPicture, FlipView and composition classes that use images (164 passed), and Skia WebAssembly the image and imaging classes (93 passed); the one failure on both, When_WriteableBitmap_Assigned_With_Data_Present, fails the same way with master's image code on my machine. The body lists the runs.

@nventive-devops

Copy link
Copy Markdown
Contributor

The build 240476 found UI Test snapshots differences: skia-linux-screenshots: 118, skia-windows-screenshots: 2458

Details
  • skia-linux-screenshots: 118 changed over 2458

    🚨🚨 Comparison Details (first 20) 🚨🚨
    • CalendarView_Theming.png-dark
    • CalendarView_Theming.png
    • Buttons.png-dark
    • Buttons.png
    • ButtonClippingTestsControl.png-dark
    • ButtonClippingTestsControl.png
    • ClipboardTests.png-dark
    • CompositionEffectBrush.png-dark
    • CompositionEffectBrush.png
    • CompositionNineGridBrush_Source_Changes.png-dark
    • CompositionNineGridBrush_Source_Changes.png
    • DropDownButtonPage.png-dark
    • ImageBrushInList.png-dark
    • ImageBrushInList.png
    • DisplayInformation.png-dark
    • Examples.png
    • ImageBrushAlignmentXY.png-dark
    • ImageBrushAlignmentXY.png
    • DoubleImageBrushInList.png-dark
    • DoubleImageBrushInList.png
  • skia-windows-screenshots: 2458 changed over 2488

    🚨🚨 Comparison Details (first 20) 🚨🚨
    • Accessibility_ScreenReader.png-dark
    • AnimatedBackVisualSourceSamplePage.png
    • AndroidWindowInsets.png
    • AppBar_KeyBoard.png-dark
    • AppBarButtonWithIconTest.png
    • AnimatedVisualsTestbedPage.png-dark
    • Accessibility_ScreenReader.png
    • Application_DispatcherShutdownMode.png-dark
    • AppBar_KeyBoard.png
    • ApplicationViewMode.png-dark
    • AppBarToggleButtonTest.png-dark
    • AppWindowPresenters.png
    • AppWindowClosing.png-dark
    • Application_DispatcherShutdownMode.png
    • ApplicationViewSizing.png
    • Attributed_text_FontSize_Changing.png-dark
    • AppWindowPositionAndSize.png
    • AppWindowTitleBar Properties.png-dark
    • AppWindowPositionAndSize.png-dark
    • AuthenticationBroker_Demo.png-dark

@Xiaoy312 Xiaoy312 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR with a focus on the #25596 part (long-lived sources keeping their consumers alive), and checked the claims at runtime: a throwaway probe class run on master (55cddbc) and on this PR's head (4fcb28c), Skia desktop, Release, one attempt per test. The probes keep weak references to the elements and force collections for 3 s; some read or clear private fields by reflection to show which references hold what. The probe source is attached at the end.

Summary. The Image fix works, including the default cached-UriSource path that the PR's own test does not cover. Nothing below blocks the merge. Three small changes are worth making first: the commit message (1) and two lines in "Behavior to be aware of" (3, 4).

Findings

  1. The commit message would close #25596, which is still half open. 4fcb28c ends with Fixes https://github.com/unoplatform/uno/issues/25596, so merging will probably auto-close the issue. The ImageBrush half still leaks on this branch (P06), so Refs or Part of would fit better.

  2. ImageBrush still leaks, through exactly two references.

    • The two references are the source's subscription to the brush (ImageBrush.crossruntime.cs, Subscribe(OnSourceOpened)) and Invalidated += brush.OnInvalidateRender (ImageBrush.cs).
    • Either one alone keeps the brushes alive (P07, P08). Clearing both frees them (P09). A follow-up therefore has to fix both: making only one of them weak changes nothing.
    • Each brush keeps the Border it paints alive through its inheritance-context parent, and the Border is freed when the brush is. Fixing those two references therefore also covers the "keeps the element it painted" part of #25596.
    • The UriSource property-changed callback this PR removes from ImageBrush was not holding the brush: master frees the brushes once those two references are cleared (P09).
  3. An auto-sized Image that comes back after a release can collapse while its image downloads again.

    • IsSourceReady() depends on _currentSurface.Size, which is null after ReleaseFrames. An Image without an explicit size therefore lays out at 0×0 until the new decode lands.
    • It shows only when that decode is slow. With the cache off and a URI that takes 1.5 s, the Image was 0×0 when loaded, made a second request, and went back to 100×100 when the download finished, so the layout shifts (P13). A stream source decodes again before the layout pass and keeps its size (P10, P12). Cached URIs are never released.
    • So it needs the opt-in cache-off setting. The body already mentions the blank image and the second download; the layout shift is worth one more line there. Measuring with the source's last known pixel size until the new decode lands would avoid it.
  4. An Image outside the tree no longer opens its source. On master, setting Source on an Image that was never added to the tree still opens the BitmapImage and raises ImageOpened. On this branch it does neither (P11). That is likely WinUI's behaviour, but it is a visible change for apps that preload through an off-tree Image, so it belongs in "Behavior to be aware of".

  5. Nit: a test case for a cached UriSource. The PR's test covers a stream bitmap and an SVG. The default case, a cached UriSource, also works (P03), because a surface refers back to its brushes through weak references. A test case for it would protect that.

  6. Latent, nothing to change today: ReleaseFrames on a texture-backed surface. On a surface built with the CompositionImageSurface(ITexture) constructor, ReleaseFrames would dispose the texture for good. No ImageSource creates one today. IsReleasable could exclude such surfaces in case one ever does.

  7. For information: a source with a single Image still keeps it.

    • Here the source's inheritance-context parent (_associatedParent) is the only reference left (P04, P05). Clearing it frees the Image on this branch but not on master, where the subscription also holds it.
    • With #25102, the Image half of #25596 is therefore fully fixed, as the body says.

Probe results

"Freed" counts the elements collected after unload. Each "clears …" row uses reflection to remove that reference after unload, which shows whether it is the one holding the element. The two-element probes use two consumers so that the source's inheritance-context parent is cleared and only the other references remain.

Probe Scenario master this PR Verdict
P01 2 Images share a stream BitmapImage; freed after unload 0/2 2/2 Fixed by this PR
P02 Same, SvgImageSource 0/2 2/2 Fixed by this PR
P03 Same, cached UriSource (bitmap cache on, the default) 0/2 2/2 Fixed by this PR, including the default path the PR's test doesn't cover (finding 5)
PR test When_Shared_Source_Outlives_Unloaded_Images_Then_Images_Collected (bitmap, SVG) (see P01, P02) pass, pass Matches what the PR reports
P04 1 Image is the source's only user; freed after unload 0/1 0/1 Still leaks, as the PR body says (#25102's scope)
P05 P04, clears the source's _associatedParent 0/1 1/1 On this branch, _associatedParent is the only reference left; on master the subscription also holds the Image (finding 7)
P06 2 ImageBrushes (on 2 Borders) share a stream bitmap; freed after unload brushes 0/2, borders 0/2 brushes 0/2, borders 0/2 ImageBrush still leaks, and so does the Border it paints (findings 1, 2)
P07 P06, clears only the source's subscriptions 0/2, 0/2 0/2, 0/2 The Invalidated handler alone keeps the brushes alive
P08 P06, clears only the source's Invalidated handlers 0/2, 0/2 0/2, 0/2 The subscription alone keeps them alive
P09 P06, clears both 2/2, 2/2 2/2, 2/2 Those two are the only references holding the brushes; each Border is freed with its brush (finding 2)
P10 Auto-sized Image, stream source: unload, wait 2.5 s, load again stays 100×100 released, stays 100×100 No collapse: the stream decodes again before layout
P12 P10, panel width changed first so the Image is measured again when it loads stays 100×100 released, stays 100×100 No collapse, as in P10
P13 P12, bitmap cache off, URI that takes 1.5 s to respond 100×100 when loaded, 1 request released, 0×0 when loaded, then 100×100, 2 requests Collapses while it downloads again (finding 3)
P11 Image never added to the tree, UriSource set; is BitmapImage.ImageOpened raised? yes no Behaviour change (finding 4)

Notes:

  • P09 also passing on master is how I found that ImageBrush has no third reference (finding 2).
  • P13 is read from the logged ActualWidth/ActualHeight right after the Image loads. Its SizeChanged-based assertion passed on both branches, because the 0×0 state did not raise SizeChanged.
  • Not covered: WebAssembly, and servicing/6.8.

Probe source (throwaway, Skia only; drop it under src/Uno.UI.RuntimeTests/): Given_Probe25596.cs.txt

@Xiaoy312 Xiaoy312 self-assigned this Oct 20, 2026
An icon held in a static field or an app resource outlives the Images
that show it. On master an Image subscribes to its source until its
Source changes, and the source keeps that callback, which captures the
Image, in a strong list: every Image that ever showed the icon stays
alive, with everything it references.

An Image now subscribes only while it is loaded, so the source lets go
of it once it leaves the tree. Cover a bitmap, a cached URI and an SVG
source, each shared by two Images, as icons are: a source with a single
owner still keeps it as its inheritance context parent, which this does
not change.

Refs unoplatform#25596
An auto-sized Image showing a source that released its pixels, because
nothing displayed them for the release delay, measured a surface with
no size and laid out at 0×0 until the source decoded them again. With
the bitmap cache off, a URI downloads again meanwhile, so the layout
around the Image collapsed and then grew back. This happened to an
Image coming back to the tree, and to another Image given the source,
such as a recycled list item.

A source now keeps the pixel size of what it released until it opens
again or changes, and an Image measures with it meanwhile, as WinUI
keeps the image's size. A source that fails or is cleared still leaves
the Image empty.
An Image now subscribes to its source only while it is loaded, so one
that was never added to the tree no longer opens its source or raises
ImageOpened, as on WinUI. Cover it, and that the source opens once the
Image is loaded.
Releasing a surface built around a texture would dispose the texture,
which is its content: nothing could decode it again. No image source
creates such a surface today, but one that did would have shown
nothing once its Images came back. A source now neither counts nor
releases one, and the surface itself ignores a release.
Copilot AI balanced review requested due to automatic review settings October 21, 2026 20:33
@mikernet
mikernet force-pushed the dev/mikernet/image-release-25114 branch from 4fcb28c to 772c328 Compare October 21, 2026 20:33
mikernet added a commit to mikernet/uno that referenced this pull request Oct 21, 2026
With the bitmap cache on, the decode shared by the BitmapImages that
load the same uri and decode size was owned by an LRU of
MaxBitmapImageCacheCount (100) entries. No source released it, and the
LRU kept it whether or not anything displayed it. An entry the LRU
evicted, or a shared load whose requesters were all cancelled, was left
to finalization (unoplatform#25128).

A shared decode now counts its uses: the opens waiting for it, and the
sources whose subscribers draw it or were given it to draw. Once it has
none, it waits on the same pending-release list as the decodes sources
own (1 s and MaxUndisplayedImageBytes, oldest first). When released, its
frames are freed and it leaves the cache, and a source that still holds
it opens again on its next subscriber. It keeps its pixel size, which an
Image showing that source measures with until then, as for the decodes
sources own.

An open hands its use over to its source once the source counts its own
use. Any other way out of the open (cancelled, failed, superseded, a
handler that throws) ends it, so nothing can release the result in
between, and nothing keeps it for good. A load whose requesters were all
cancelled keeps running for later ones, as since unoplatform#25101, and its result
then waits like any other.

The sharing follows WinUI's ImageCache, which leaves the provider's
store once the last BitmapImage using it lets it go (MUX Reference:
ImageProvider::EnsureCacheEntry, ImageProvider::RemoveImageCache). Like
that store, the cache does not own its decodes. An ImageBrush keeps its
subscription for its lifetime, so a source can be collected while it
still counts as a user. Sources are therefore held weakly, and a
collected one is dropped the next time the decode's uses change.

A subscriber that comes back still drawing a shared decode its source
replaced while it was away uses it again until it draws what replaced
it, as unoplatform#25124 lets it take back replaced pixels it owns.

The LRUCache helper had no other user and is removed.
MaxBitmapImageCacheCount no longer bounds anything; the next commit
marks it obsolete.
mikernet added a commit to mikernet/uno that referenced this pull request Oct 21, 2026
Shared decodes are kept while something draws them or was given them to
draw: while either of two sharing sources shows one, and for a moved
Image, an Image a re-entrant change skipped, one that comes back while
the replacement downloads, a drawn brush, an Image that left between
delivery and measure, and a collapsed Image. Once nothing does, they are
released on the same list and limit as owned decodes, and opened again
from one new decode.

An open superseded while taking its pixel size, or whose ImageOpened
handler throws, gives up its shared result, as does a shared load whose
requesters were all cancelled, a brush given another ImageSource, and a
source an ImageOpened handler switched away from. A source collected
while subscribed keeps no decode in the cache, alone or shared with a
live one, and does not hide the live one's use.

A released download is decoded again from its encoded bytes, unless the
response was unsuccessful, redirected (desktop), or said no-store,
no-cache or max-age=0. The encoded cache drops the least recently used
image once full, bytes unused past the expiry, and what a lowered bound
or a disabled cache no longer allows. On WebAssembly, where a test
cannot listen, a stand-in for fetch serves the image and counts its
requests.

Two tests from unoplatform#25124 assumed a shared decode is never released: the
within-delay one now waits with a long delay instead of a limit of 0,
and the cache-off one also checks that the shared decode goes, on a
decode size of its own. The failed-source test also checks the release
with the cache on.
mikernet added a commit to Singulink/uno that referenced this pull request Oct 21, 2026
Port of unoplatform#25124's bfa20af and 129365e, with the
part of unoplatform#25559's fc608f4 that keeps a released shared decode's size.
An auto-sized Image showing a source that released its pixels, because
nothing displayed them for the release delay, laid out at 0×0 until the
source decoded them again. With the bitmap cache off, a URI downloads
again meanwhile; with it on, a released shared decode decodes again, or
downloads again when its response could not be kept. This happened to
an Image coming back to the tree, and to another Image given the
source, such as a recycled list item.

A source now keeps the pixel size of what it released until it opens
again or changes, a released shared decode keeps its own, and an Image
measures with it meanwhile, as WinUI keeps the image's size. A source
that fails or is cleared still leaves the Image empty. An Image never
added to the tree doesn't open its source, as on WinUI, and a test now
covers it.

6.8 differences:
- Arrange scaled the brush by the surface's image size, which a
  released surface, or an Image with no surface yet, doesn't have: it
  now keeps its last scale until the image is there.
- The SVG path is unchanged, as 6.8 keeps an SVG source's parsed
  document.
- 6.8 has no texture-backed surfaces, so 772c328 isn't needed.
- The size test runs on Skia desktop and the off-tree test on Skia;
  both pass on WinUI on master. The native renderers open their sources
  themselves.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-platform surface ownership and reentrant lifecycle changes warrant final human review of the implementation and runtime validation evidence.

No open issues 🎉

Review effort: Balanced

@mikernet

mikernet commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

@Xiaoy312 thanks for the probes, they made each point easy to act on. All seven are handled: fa4a240 replaces the last commit, and three commits are new.

  1. Trailer: fa4a240 ends with Refs instead of Fixes, and adds the cached case (5).
  2. ImageBrush: fix(imaging): Let a long-lived source drop the brushes it painted #25633, stacked on this PR, makes both references weak and closes [Skia] A long-lived ImageSource keeps every Image and ImageBrush that used it alive #25596.
    • The brush subscribes through a new SubscribeWeakly and holds the lease it returns. The source keeps only a weak reference to the callback. Once the brush is collected, the lease's finalizer ends the subscription on the UI thread, so the source can still release the pixels it owns. While the source opens, it keeps those callbacks alive, so an app waiting only for a brush's ImageOpened still gets it.
    • Invalidated becomes a weak RegisterInvalidated, built on WeakEventHelper like Brush.RegisterInvalidateRender.
    • Its tests check that the brushes and their Borders are collected (P06) for a stream bitmap, an SVG and a cached UriSource, and that the source then releases its pixels.
  3. Collapse: fixed rather than documented, in bfa20af. A source now keeps the pixel size of what it released until it opens again or changes, and an Image measures with it meanwhile. That covers the Image coming back, and another Image given the source, such as a recycled item. When_Source_Shown_Again_While_It_Opens_Again_Then_AutoSized_Image_Keeps_Its_Size reproduces P13 for both with the gated image server: both rows fail without the fix (0×0) and pass with it, and both pass on WinUI. fix(imaging): Release cached decodes, keep encoded bytes instead #25559 keeps the size of a released shared decode the same way.
  4. Off-tree Image: confirmed on WinUI. When_Image_Never_In_Tree_Then_Source_Not_Opened passes there and here (129365e), and the change is now under "Behavior to be aware of".
  5. Cached UriSource: When_Shared_Source_Outlives_Unloaded_Images_Then_Images_Collected now has that case.
  6. Texture-backed surfaces: IsReleasable now excludes them, and ReleaseFrames ignores them (772c328).
  7. Single owner: agreed, that's left to fix: Stop shared dependency objects retaining discarded elements and their DataContext #25102.

Runs: Skia desktop, 23 image-related classes, 298 passed; WebAssembly, 9 image classes, 104 passed. On both, the one failure is When_WriteableBitmap_Assigned_With_Data_Present, which fails with master's code at this machine's 125% scaling.

@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-25124/docs/index.html

mikernet added a commit to Singulink/uno that referenced this pull request Oct 21, 2026
Port of unoplatform#25633 (9f71cb9 and 9fcac22), with the
shared-source Image test of unoplatform#25124's fa4a240, whose helpers its
tests use.

A source that outlives its brushes, such as an icon in a static field
or in the app resources, kept every ImageBrush that ever painted it
alive, and with each one the element it painted. The source held the
brush's subscription and its Invalidated handler strongly. It now
holds both weakly: the brush keeps a lease on its subscription, whose
finalizer ends the subscription once the brush is collected, so the
source can release the pixels it owns; an open in flight keeps the
brushes waiting for it alive, but not one that left the source; and
Invalidated becomes a weak RegisterInvalidated registration.

6.8 differences:
- The open's completion also ends a shared decode's open use (the
  unoplatform#25559 port), so the keep-alive release sits inside that try.
- The native Image's WriteableBitmap handler (Image.cs) uses the weak
  registration too. The native WebAssembly brush keeps its own
  per-element subscription, unchanged.
- The test surface type is SkiaCompositionSurface.

@Xiaoy312 Xiaoy312 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @mikernet. I re-ran the same probes, plus the new and updated tests, on 772c328 (Skia desktop, Release, one attempt per test). All seven points are addressed, and I have nothing blocking left on this PR.

  • 1, 5: fa4a240 says Refs #25596, and When_Shared_Source_Outlives_Unloaded_Images_Then_Images_Collected passes for all three sources: Bitmap, Svg and CachedUri.
  • 3: P13 now stays 100×100 when it loads (it was 0×0), still with the second request. Both rows of When_Source_Shown_Again_While_It_Opens_Again_Then_AutoSized_Image_Keeps_Its_Size pass. With bfa20af's Image/ImageSource changes reverted, both rows fail and P13 is back to 0×0, so the test catches the regression. Reading the code:
    • ReleaseUnused is the only path that releases the current data while the source itself is unchanged. OnOpened, including a failed or empty open, and InvalidateSource both clear the size. So a stale size only lasts while the source opens again.
    • The measure that follows delivery also invalidates arrange, so the brush's scale transform is set once the pixels are back.
  • 4: When_Image_Never_In_Tree_Then_Source_Not_Opened passes, and P11 now fails as expected. I didn't check WinUI myself.
  • 6: 772c328 covers it: a texture-backed surface is now neither counted nor released by the source, and ReleaseFrames ignores it as well.
  • 2, 7: P06–P08 and P04 still leak here, as expected, since those parts are left to #25633 and #25102. I haven't reviewed #25633.

All 50 tests in Given_Image_Release pass.

Nit, only if the commits are kept as they are when merging: bfa20af strips the UTF-8 BOM from Given_Image.cs, and 129365e adds it back. The net diff is clean, but each of the two commits shows a line-1 change.

@Xiaoy312 Xiaoy312 closed this pull request by merging all changes into unoplatform:master in 1d33309 Oct 28, 2026
@Xiaoy312

Copy link
Copy Markdown
Contributor

this is automagically closed because this is a subset of #25633

MartinZikmund pushed a commit that referenced this pull request Nov 2, 2026
An ImageSource kept its subscribers in a strong list, and the Skia
Image subscribed to the SourceLoaded and OpenFailed events of an
SvgImageSource. ImageBrush did the same with its subscription and with
a handler on ImageSource.Invalidated. All of these were only released
when the consumer's source changed, so a source that outlives its
consumers, such as an icon in a static field or in the application
resources, kept every Image and ImageBrush that ever used it alive,
along with everything they reference.

Hold the subscribers weakly instead: the disposable returned by
Subscribe keeps the callback alive for as long as the consumer holds
it, and an open in progress keeps the current subscribers alive until
it completes, as a SetSourceAsync awaiting it may be referenced by
nothing else. The SVG events and Invalidated are forwarded through
handlers that hold the consumer weakly and unsubscribe once it is gone.
When sources open and when ImageOpened and ImageFailed are raised is
unchanged.

This is a narrow servicing fix: on master, Image subscribing only while
loaded (#25124) fixes this as part of a much larger change. A source
used by a single element still references it as its inheritance
context parent until another element uses it, which is addressed
separately (#25102) and not covered here.

Fixes #25596
mikernet added a commit to mikernet/uno that referenced this pull request Nov 12, 2026
With the bitmap cache on, the decode shared by the BitmapImages that
load the same uri and decode size was owned by an LRU of
MaxBitmapImageCacheCount (100) entries. No source released it, and the
LRU kept it whether or not anything displayed it. An entry the LRU
evicted, or a shared load whose requesters were all cancelled, was left
to finalization (unoplatform#25128).

A shared decode now counts its uses: the opens waiting for it, and the
sources whose subscribers draw it or were given it to draw. Once it has
none, it waits on the same pending-release list as the decodes sources
own (1 s and MaxUndisplayedImageBytes, oldest first). When released, its
frames are freed and it leaves the cache, and a source that still holds
it opens again on its next subscriber. It keeps its pixel size, which an
Image showing that source measures with until then, as for the decodes
sources own.

An open hands its use over to its source once the source counts its own
use. Any other way out of the open (cancelled, failed, superseded, a
handler that throws) ends it, so nothing can release the result in
between, and nothing keeps it for good. A load whose requesters were all
cancelled keeps running for later ones, as since unoplatform#25101, and its result
then waits like any other.

The sharing follows WinUI's ImageCache, which leaves the provider's
store once the last BitmapImage using it lets it go (MUX Reference:
ImageProvider::EnsureCacheEntry, ImageProvider::RemoveImageCache). Like
that store, the cache does not own its decodes. An ImageBrush keeps its
subscription for its lifetime, so a source can be collected while it
still counts as a user. Sources are therefore held weakly, and a
collected one is dropped the next time the decode's uses change.

A subscriber that comes back still drawing a shared decode its source
replaced while it was away uses it again until it draws what replaced
it, as unoplatform#25124 lets it take back replaced pixels it owns.

The LRUCache helper had no other user and is removed.
MaxBitmapImageCacheCount no longer bounds anything; the next commit
marks it obsolete.
mikernet added a commit to mikernet/uno that referenced this pull request Nov 12, 2026
Shared decodes are kept while something draws them or was given them to
draw: while either of two sharing sources shows one, and for a moved
Image, an Image a re-entrant change skipped, one that comes back while
the replacement downloads, a drawn brush, an Image that left between
delivery and measure, and a collapsed Image. Once nothing does, they are
released on the same list and limit as owned decodes, and opened again
from one new decode.

An open superseded while taking its pixel size, or whose ImageOpened
handler throws, gives up its shared result, as does a shared load whose
requesters were all cancelled, a brush given another ImageSource, and a
source an ImageOpened handler switched away from. A source collected
while subscribed keeps no decode in the cache, alone or shared with a
live one, and does not hide the live one's use.

A released download is decoded again from its encoded bytes, unless the
response was unsuccessful, redirected (desktop), or said no-store,
no-cache or max-age=0. The encoded cache drops the least recently used
image once full, bytes unused past the expiry, and what a lowered bound
or a disabled cache no longer allows. On WebAssembly, where a test
cannot listen, a stand-in for fetch serves the image and counts its
requests.

Two tests from unoplatform#25124 assumed a shared decode is never released: the
within-delay one now waits with a long delay instead of a limit of 0,
and the cache-off one also checks that the shared decode goes, on a
decode size of its own. The failed-source test also checks the release
with the cache on.
mikernet added a commit to Singulink/uno that referenced this pull request Nov 14, 2026
Takes Uno's servicing/6.8 from 6951c2c up to 5254ace (42 pull
requests). Conflicts, by area:

IME and TextBox (unoplatform#25984, backport of unoplatform#25804; unoplatform#25952, backport of unoplatform#25619;
unoplatform#25976, backport of unoplatform#25818)
- Takes upstream's composition design (unoplatform#25804): the browser's input is
  the source of truth for the text, compositions included. This drops
  the composition parts of the unoplatform#24502 port (90746b8): its
  TypeScript composition handling, the StartIndex/TextChangePending
  event arguments, the IsReadOnly forwarding to the hidden input, and
  its Given_TextBox.BrowserSoftKeyboard tests.
- Keeps the non-IME browser fixes on top of it, in the form their
  master pull requests have on unoplatform#25804: Enter raised once per press and
  tracked by key code (unoplatform#25174, with Given_TextBox.BrowserEnterKey from
  master in place of the Enter tests that lived in the dropped file),
  the iOS refocus on tap (unoplatform#25185), pastes without text and isIOS from
  BrowserInputHelper.
- Moves the unoplatform#26005 MaxLength fixes onto unoplatform#25804's native input path, as
  master has them: a refused browser edit keeps the TextBox's selection,
  TextBoxView.UpdateTextFromNative reports whether the edit went in, and
  the IME keeps its range on the applied text (master be85699, which
  d226897 had ported onto the unoplatform#24502 IME). Adds that commit's browser
  and platform-applied IME tests.
- TextBox.pointers.skia.cs: upstream's file (unoplatform#25619 iOS touch editing,
  unoplatform#25818, unoplatform#25804), with the unoplatform#25126 port (the hold acts when it starts,
  a touch right-tap keeps the caret) in the form master's unoplatform#25126 branch
  has it on unoplatform#25619, including focusing before the iOS hold timer acts
  and its iOS not-focusable test.

Pointers (unoplatform#25976, backport of unoplatform#25818)
- Tapped and RightTapped are raised once the PointerReleased bubbled
  (unoplatform#25818), with the unoplatform#25732 port's window coordinates (2-argument
  routed event args, RawPosition for ContextRequested), as master's
  unoplatform#25732 branch has them.
- The release wrapper of unoplatform#25818 keeps the unoplatform#25126 port's
  StopContextMenuTimer at the start of the release.

DependencyObject (unoplatform#26142, backport of unoplatform#26123)
- Takes upstream, then the code of unoplatform#25839 (97a2f17), which replaces
  this branch's unoplatform#25102 ports (800247f, be41da6, fd40bf9,
  7546379). DependencyObjectStore.Binder.cs, .alc.cs,
  Application.Alc.cs, ResourceDictionary.alc.cs and their tests now
  match unoplatform#25839; this drops 800247f's own
  SingleParentNonFE_Parent_Removes_Value_Then_Next_Parent_Starts_Clean
  test, which unoplatform#25839 doesn't have.

Imaging (unoplatform#25597, unoplatform#25915, unoplatform#25911, unoplatform#25897)
- Keeps this branch's side: it has the later forms of these fixes
  (unoplatform#25124, unoplatform#25559, unoplatform#25633 and unoplatform#26370 ports), where upstream has the
  6.8-only unoplatform#25597 design and earlier snapshots. Upstream's shared
  download, IgnoreImageCache and failed-load fixes are covered by
  SharedImageDecode. Adds upstream's When_Source_Outlives_Brushes test.

ToolTip (unoplatform#26098, backport of unoplatform#25137 and unoplatform#26096)
- Keeps this branch's unoplatform#25137 port and adds unoplatform#26096 in master's form: the
  service releases only a ToolTip it opened, and leaves one the app
  opened during the show delay to the app. Keeps both sides' tests.

Flyout (unoplatform#26062, backport of unoplatform#26060)
- This branch had the same commits already; keeps the "Keep
  non-resizable flyouts in window" port and its tests.

Also removes a helper pair in Given_AccessibleTextBox that both sides
added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/skia ✏️ Categorizes an issue or PR as relevant to Skia kind/documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Skia] A long-lived ImageSource keeps every Image and ImageBrush that used it alive [Skia] A decoded image's pixels are released only through a finalizer chain, never when the image leaves the screen

8 participants