From 6cae26fbf4709a868bf403d5fb99c4d7d178903a Mon Sep 17 00:00:00 2001 From: Matthew Leibowitz Date: Sat, 28 Mar 2026 21:15:56 +0200 Subject: [PATCH] Refactor SKManagedStream: replace parent/child chain with SKData snapshot Replace the fragile parent/child weak-reference ownership chain in SKManagedStream with a lazy SKData snapshot approach for duplicate/fork. OnDuplicate() now creates a native SkMemoryStream backed by a cached, ref-counted SKData snapshot of the .NET stream. OnFork() duplicates and seeks to the current position. The original stream remains fully readable after duplication, multiple duplicates are allowed, and each duplicate is an independent native stream with no managed interop overhead. This is both simpler and faster than the old approach: - No parent/child weak references or ownership transfer logic - No VerifyOriginal() / InvalidOperationException on the original stream - Snapshot is cached (one read, shared by all duplicates) - Native SkMemoryStream duplicates expose getMemoryBase(), enabling zero-copy fast paths in Skia (e.g. CoreText font loading on macOS) Tests updated to reflect new semantics: - Removed SkipOnNonWindows() from 6 tests (no longer throws in native delegates, so all platforms can run these tests now) - Added NonSeekableStreamDuplicateAndForkReturnNull test - Renamed DotNetStreamIsClosedWhenSKManagedStreamIsDisposed to document that the .NET stream is closed when the parent is disposed (the snapshot keeps duplicates alive independently via native ref-counting) Based on the original SKManagedStream refactoring from PR #3560. Co-authored-by: Ramez Ragaa <66218781+ramezgerges@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 --- binding/SkiaSharp/SKManagedStream.cs | 99 ++++--------- tests/Tests/SkiaSharp/SKManagedStreamTest.cs | 148 +++++++++++++------ 2 files changed, 133 insertions(+), 114 deletions(-) diff --git a/binding/SkiaSharp/SKManagedStream.cs b/binding/SkiaSharp/SKManagedStream.cs index fcaa5f70bae..60ae9a86bac 100644 --- a/binding/SkiaSharp/SKManagedStream.cs +++ b/binding/SkiaSharp/SKManagedStream.cs @@ -11,10 +11,11 @@ public class SKManagedStream : SKAbstractManagedStream private bool isAsEnd; private bool disposeStream; - private bool wasCopied; - private WeakReference parent; - private WeakReference child; + // Lazily-created snapshot for duplicate/fork. Stored in native memory + // via SKData (ref-counted), so multiple duplicates share one native + // byte buffer with no additional managed byte-buffer allocations per duplicate. + private SKData snapshotData; public SKManagedStream (Stream managedStream) : this (managedStream, false) @@ -56,28 +57,11 @@ protected override void Dispose (bool disposing) => protected override void DisposeManaged () { - var childStream = child?.Target as SKManagedStream; - var parentStream = parent?.Target as SKManagedStream; - - if (childStream != null && parentStream != null) { - // remove this stream from the list by connecting the parent with the child - childStream.parent = parent; - parentStream.child = child; - } else if (childStream != null) { - // transfer ownership to child - childStream.parent = null; - } else if (parentStream != null) { - // transfer ownership back to parent - parentStream.child = null; - parentStream.wasCopied = false; - parentStream.disposeStream = disposeStream; - - disposeStream = false; + if (snapshotData != null) { + snapshotData.Dispose (); + snapshotData = null; } - parent = null; - child = null; - if (disposeStream && stream != null) { stream.Dispose (); stream = null; @@ -113,15 +97,11 @@ private IntPtr OnReadManagedStream (IntPtr buffer, IntPtr size) protected internal override IntPtr OnRead (IntPtr buffer, IntPtr size) { - VerifyOriginal (); - return OnReadManagedStream (buffer, size); } protected internal override IntPtr OnPeek (IntPtr buffer, IntPtr size) { - VerifyOriginal (); - if (!stream.CanSeek) { return (IntPtr)0; } @@ -133,8 +113,6 @@ protected internal override IntPtr OnPeek (IntPtr buffer, IntPtr size) protected internal override bool OnIsAtEnd () { - VerifyOriginal (); - if (!stream.CanSeek) { return isAsEnd; } @@ -143,22 +121,16 @@ protected internal override bool OnIsAtEnd () protected internal override bool OnHasPosition () { - VerifyOriginal (); - return stream.CanSeek; } protected internal override bool OnHasLength () { - VerifyOriginal (); - return stream.CanSeek; } protected internal override bool OnRewind () { - VerifyOriginal (); - if (!stream.CanSeek) { return false; } @@ -168,8 +140,6 @@ protected internal override bool OnRewind () protected internal override IntPtr OnGetPosition () { - VerifyOriginal (); - if (!stream.CanSeek) { return (IntPtr)0; } @@ -178,8 +148,6 @@ protected internal override IntPtr OnGetPosition () protected internal override IntPtr OnGetLength () { - VerifyOriginal (); - if (!stream.CanSeek) { return (IntPtr)0; } @@ -188,8 +156,6 @@ protected internal override IntPtr OnGetLength () protected internal override bool OnSeek (IntPtr position) { - VerifyOriginal (); - if (!stream.CanSeek) { return false; } @@ -199,8 +165,6 @@ protected internal override bool OnSeek (IntPtr position) protected internal override bool OnMove (int offset) { - VerifyOriginal (); - if (!stream.CanSeek) { return false; } @@ -210,46 +174,43 @@ protected internal override bool OnMove (int offset) protected internal override IntPtr OnCreateNew () { - VerifyOriginal (); - return IntPtr.Zero; } - protected internal override IntPtr OnDuplicate () + private SKData GetOrCreateSnapshot () { - VerifyOriginal (); + if (snapshotData != null) + return snapshotData; if (!stream.CanSeek) - return IntPtr.Zero; - - var newStream = new SKManagedStream (stream, disposeStream); - newStream.parent = new WeakReference (this); - - wasCopied = true; - disposeStream = false; - child = new WeakReference (newStream); - - stream.Position = 0; + return null; + + var pos = stream.Position; + try { + stream.Position = 0; + snapshotData = SKData.Create (stream, stream.Length); + } finally { + stream.Position = pos; + } - return newStream.Handle; + return snapshotData; } - protected internal override IntPtr OnFork () + protected internal override IntPtr OnDuplicate () { - VerifyOriginal (); - - var newStream = new SKManagedStream (stream, disposeStream); - - wasCopied = true; - disposeStream = false; + var data = GetOrCreateSnapshot (); + if (data == null) + return IntPtr.Zero; - return newStream.Handle; + return SkiaApi.sk_memorystream_new_with_skdata (data.Handle); } - private void VerifyOriginal () + protected internal override IntPtr OnFork () { - if (wasCopied) - throw new InvalidOperationException ("This stream was duplicated or forked and cannot be read anymore."); + var duplicate = OnDuplicate (); + if (duplicate != IntPtr.Zero) + SkiaApi.sk_stream_seek (duplicate, (IntPtr)stream.Position); + return duplicate; } } } diff --git a/tests/Tests/SkiaSharp/SKManagedStreamTest.cs b/tests/Tests/SkiaSharp/SKManagedStreamTest.cs index 87f70c17884..cdc466fae69 100644 --- a/tests/Tests/SkiaSharp/SKManagedStreamTest.cs +++ b/tests/Tests/SkiaSharp/SKManagedStreamTest.cs @@ -408,10 +408,8 @@ SKCodec CreateCodec(out IntPtr streamHandle) } [SkippableFact] - public void StreamCanBeDuplicatedButTheOriginalCannotBeRead() + public void StreamCanBeDuplicatedAndBothRemainReadable() { - SkipOnNonWindows(); - var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); var stream = new SKManagedStream(dotnet, true); Assert.Equal(1, stream.ReadByte()); @@ -420,18 +418,21 @@ public void StreamCanBeDuplicatedButTheOriginalCannotBeRead() var dupe = stream.Duplicate(); Assert.NotSame(stream, dupe); - Assert.IsType(dupe); + // duplicate starts at position 0 Assert.Equal(1, dupe.ReadByte()); Assert.Equal(2, dupe.ReadByte()); Assert.Equal(3, dupe.ReadByte()); - Assert.Throws(() => stream.ReadByte()); + // original remains readable at its previous position + Assert.Equal(4, stream.ReadByte()); + Assert.Equal(5, stream.ReadByte()); + + dupe.Dispose(); + stream.Dispose(); } [SkippableFact] - public void StreamCanBeForkedButTheOriginalCannotBeRead() + public void StreamCanBeForkedAndBothRemainReadable() { - SkipOnNonWindows(); - var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); var stream = new SKManagedStream(dotnet, true); Assert.Equal(1, stream.ReadByte()); @@ -439,33 +440,41 @@ public void StreamCanBeForkedButTheOriginalCannotBeRead() var dupe = stream.Fork(); Assert.NotSame(stream, dupe); - Assert.IsType(dupe); + // fork starts at the same position as the original Assert.Equal(3, dupe.ReadByte()); Assert.Equal(4, dupe.ReadByte()); - Assert.Throws(() => stream.ReadByte()); + // original remains readable at its previous position + Assert.Equal(3, stream.ReadByte()); + Assert.Equal(4, stream.ReadByte()); + + dupe.Dispose(); + stream.Dispose(); } [SkippableFact] - public void StreamCannotBeDuplicatedMultipleTimes() + public void StreamCanBeDuplicatedMultipleTimes() { - SkipOnNonWindows(); - var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); var stream = new SKManagedStream(dotnet, true); Assert.Equal(1, stream.ReadByte()); Assert.Equal(2, stream.ReadByte()); - var dupe = stream.Duplicate(); - Assert.Throws(() => stream.Duplicate()); + var dupe1 = stream.Duplicate(); + var dupe2 = stream.Duplicate(); - Assert.Equal(1, dupe.ReadByte()); + Assert.Equal(1, dupe1.ReadByte()); + Assert.Equal(1, dupe2.ReadByte()); + // original still works + Assert.Equal(3, stream.ReadByte()); + + dupe1.Dispose(); + dupe2.Dispose(); + stream.Dispose(); } [SkippableFact] - public void StreamCanBeDuplicatedMultipleTimesIfTheChildIsDestroyed() + public void StreamCanBeDuplicatedMultipleTimesWithChildDisposed() { - SkipOnNonWindows(); - var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); var stream = new SKManagedStream(dotnet, true); Assert.Equal(1, stream.ReadByte()); @@ -477,18 +486,23 @@ public void StreamCanBeDuplicatedMultipleTimesIfTheChildIsDestroyed() dupe1.Dispose(); + // can still duplicate after child is disposed var dupe2 = stream.Duplicate(); Assert.Equal(1, dupe2.ReadByte()); Assert.Equal(2, dupe2.ReadByte()); - Assert.Throws(() => stream.Duplicate()); + // can duplicate again — no limit + var dupe3 = stream.Duplicate(); + Assert.Equal(1, dupe3.ReadByte()); + + dupe2.Dispose(); + dupe3.Dispose(); + stream.Dispose(); } [SkippableFact] - public void FullOwnershipIsTransferredToTheChildIfTheParentIsDisposed() + public void DuplicatesAreIndependentAfterParentDisposed() { - SkipOnNonWindows(); - var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); var stream = new SKManagedStream(dotnet, true); Assert.Equal(1, stream.ReadByte()); @@ -498,32 +512,59 @@ public void FullOwnershipIsTransferredToTheChildIfTheParentIsDisposed() Assert.Equal(1, dupe1.ReadByte()); Assert.Equal(2, dupe1.ReadByte()); - Assert.Throws(() => stream.Duplicate()); - + // disposing parent does not affect the independent duplicate stream.Dispose(); + Assert.Equal(3, dupe1.ReadByte()); + var dupe2 = dupe1.Duplicate(); Assert.Equal(1, dupe2.ReadByte()); Assert.Equal(2, dupe2.ReadByte()); - Assert.Throws(() => dupe1.Duplicate()); - dupe1.Dispose(); dupe2.Dispose(); + // the original .NET stream should be disposed since disposeManagedStream was true Assert.Throws(() => dotnet.Position); } + [SkippableFact] + public void DuplicateStreamIsDisposed() + { + var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); + var stream = new SKManagedStream(dotnet, true); + Assert.Equal(1, stream.ReadByte()); + Assert.Equal(2, stream.ReadByte()); + + var dupe1 = stream.Duplicate(); + Assert.Equal(1, dupe1.ReadByte()); + Assert.Equal(2, dupe1.ReadByte()); + + stream.Dispose(); + + var dupe2 = dupe1.Duplicate(); + var handle = dupe2.Handle; + Assert.Equal(1, dupe2.ReadByte()); + Assert.Equal(2, dupe2.ReadByte()); + + Assert.True(SKObject.GetInstance(handle, out _)); + + dupe2.Dispose(); + Assert.False(SKObject.GetInstance(handle, out _)); + + dupe1.Dispose(); + } + [SkippableFact] public void DuplicateStreamIsCollected() { - SkipOnNonWindows(); + SkipOnMono(); var handle = DoWork(); CollectGarbage(); - Assert.False(SKObject.GetInstance(handle, out _)); + Assert.False(SKObject.GetInstance(handle, out _)); IntPtr DoWork() { @@ -536,8 +577,6 @@ IntPtr DoWork() Assert.Equal(1, dupe1.ReadByte()); Assert.Equal(2, dupe1.ReadByte()); - Assert.Throws(() => stream.Duplicate()); - stream.Dispose(); var dupe2 = dupe1.Duplicate(); @@ -549,10 +588,8 @@ IntPtr DoWork() } [SkippableFact] - public void MiddleDuplicateCanBeRemoved() + public void MultipleDuplicatesAreIndependent() { - SkipOnNonWindows(); - var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); var stream = new SKManagedStream(dotnet, true); Assert.Equal(1, stream.ReadByte()); @@ -561,26 +598,43 @@ public void MiddleDuplicateCanBeRemoved() var dupe1 = stream.Duplicate(); Assert.Equal(1, dupe1.ReadByte()); Assert.Equal(2, dupe1.ReadByte()); - Assert.Throws(() => stream.Duplicate()); - var dupe2 = dupe1.Duplicate(); + // can still duplicate the original + var dupe2 = stream.Duplicate(); Assert.Equal(1, dupe2.ReadByte()); Assert.Equal(2, dupe2.ReadByte()); - Assert.Throws(() => stream.Duplicate()); - Assert.Throws(() => dupe1.Duplicate()); + // can also duplicate the duplicate + var dupe3 = dupe1.Duplicate(); + Assert.Equal(1, dupe3.ReadByte()); + Assert.Equal(2, dupe3.ReadByte()); + + // disposing any one does not affect the others dupe1.Dispose(); - Assert.Throws(() => stream.Duplicate()); + Assert.Equal(3, stream.ReadByte()); + Assert.Equal(3, dupe2.ReadByte()); + Assert.Equal(3, dupe3.ReadByte()); dupe2.Dispose(); - var dupe3 = stream.Duplicate(); - Assert.Equal(1, dupe3.ReadByte()); - Assert.Equal(2, dupe3.ReadByte()); - Assert.Throws(() => stream.Duplicate()); + dupe3.Dispose(); + stream.Dispose(); + } + + [SkippableFact] + public void NonSeekableStreamDuplicateAndForkReturnNull() + { + var dotnet = new MemoryStream(new byte[] { 1, 2, 3, 4, 5 }); + var nonSeekable = new NonSeekableReadOnlyStream(dotnet); + var stream = new SKManagedStream(nonSeekable); + + Assert.Null(stream.Duplicate()); + Assert.Null(stream.Fork()); + + stream.Dispose(); } [SkippableFact] - public void DotNetStreamIsNotClosedPrematurely() + public void DotNetStreamIsClosedWhenSKManagedStreamIsDisposed() { var dotnet = CreateTestStream(); var stream = new SKManagedStream(dotnet, true); @@ -589,11 +643,15 @@ public void DotNetStreamIsNotClosedPrematurely() var dupe = stream.Duplicate(); Assert.Equal(0, dupe.Position); + // the snapshot has been taken, so the .NET stream is closed when + // the SKManagedStream is disposed (the duplicate is independent) stream.Dispose(); + Assert.Throws(() => dotnet.Position); + + // the duplicate is still usable after the parent is disposed Assert.Equal(0, dupe.Position); dupe.Dispose(); - Assert.Throws(() => dotnet.Position); } } }