From 54e00bf9fd2412e8b31115273d34da915128635e Mon Sep 17 00:00:00 2001 From: thmo Date: Fri, 14 Aug 2026 12:56:40 +0200 Subject: [PATCH] fix: stop callers disposing the surface everyone shares The transport tests got a surface with no size and no planes, and it was never about the test host: macOS bindings keep one managed peer per native object, and a server publishes into one recycled surface. So every frame a client delivers is the same managed instance the server writes through - and TryGetFrame told callers to dispose it. The first dispose zeroed that shared handle, and every later read came back empty. The polling loops in the tests and the sample disposed the previous frame each iteration, which made whether a run passed a matter of how the parity fell. Frames and acquired surfaces now belong to the library: both hold the peer so it is not finalized between calls, and both document that callers read the pixels rather than take the surface. Ownership is unchanged - owns:true still consumes the shim's retain, and the count stays flat across a frame loop. Also fix the plane helpers the second failure exposed. IOSurface reports no planes for a packed surface and its per-plane accessors raise NSGenericException there, so PlaneCount now answers 1 as it documents and PlaneInfo describes plane 0 from the surface itself. Closes #3 --- README.md | 7 +- samples/Syphon.NET.Peer/Program.cs | 8 +- src/Syphon.NET/IOSurfaceExtensions.cs | 24 ++++- src/Syphon.NET/SyphonClient.cs | 23 ++++- src/Syphon.NET/SyphonServer.cs | 17 +++- tests/Syphon.NET.Tests/SyphonTests.cs | 127 +++++++++++++++++++++++++- 6 files changed, 187 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index a09fdca..c104e12 100644 --- a/README.md +++ b/README.md @@ -54,8 +54,10 @@ directory.PumpEvents(TimeSpan.FromMilliseconds(200)); using var client = directory.CreateClient(index: 0); -// Frames are Microsoft's IOSurface binding directly - dispose when done (it releases the retain). -using IOSurface.IOSurface? frame = client.TryGetFrame(); +// Frames are Microsoft's IOSurface binding directly. The surface belongs to the client - read it and +// let it go, never dispose it: a server recycles one surface, and the bindings keep a single managed +// peer per native object, so disposing would zero the handle every later frame comes back through. +IOSurface.IOSurface? frame = client.TryGetFrame(); if (frame is not null) { (int w, int h) = frame.PixelSize(); @@ -78,6 +80,7 @@ using Syphon.NET; (int w, int h) = surface.PixelSize(); // int-typed dimensions bool bgra = surface.IsBgra(); // format predicates (IsBgra / IsNv12) +int planes = surface.PlaneCount(); // 1 for packed BGRA, 2 for NV12 (int cw, int ch, int stride) = surface.PlaneInfo(1); // per-plane dims + row stride // Scoped CPU access yielding a Span; unlocks on dispose: diff --git a/samples/Syphon.NET.Peer/Program.cs b/samples/Syphon.NET.Peer/Program.cs index 3477609..ee68c02 100644 --- a/samples/Syphon.NET.Peer/Program.cs +++ b/samples/Syphon.NET.Peer/Program.cs @@ -74,7 +74,7 @@ static bool Loopback(int w, int h, out string detail) using SyphonClient client = server.CreateLoopbackClient(); // Publish repeatedly and keep the most recent delivered frame; discard an initial stale one - // by requiring a few publishes before accepting. + // by requiring a few publishes before accepting. Frames belong to the client - never dispose one. IOSurface.IOSurface? frame = null; var sw = Stopwatch.StartNew(); int published = 0; @@ -85,7 +85,6 @@ static bool Loopback(int w, int h, out string detail) IOSurface.IOSurface? f = client.TryGetFrame(); if (f is not null) { - frame?.Dispose(); frame = f; if (published >= 4) break; } @@ -94,7 +93,6 @@ static bool Loopback(int w, int h, out string detail) if (frame is null) { detail = "no frame delivered"; return false; } - using (frame) { (int gotW, int gotH) = frame.PixelSize(); if (gotW != w || gotH != h) @@ -242,12 +240,11 @@ static int CrossTest() while (psw.Elapsed < TimeSpan.FromSeconds(8)) { IOSurface.IOSurface? f = client.TryGetFrame(); - if (f is not null) { frame?.Dispose(); frame = f; if (++got >= 3) break; } + if (f is not null) { frame = f; if (++got >= 3) break; } Thread.Sleep(16); } if (frame is null) { Log("CROSS: no frame delivered across processes"); return 1; } - using (frame) { (int gotW, int gotH) = frame.PixelSize(); if (gotW != w || gotH != h) @@ -308,7 +305,6 @@ static int ForeignClient(string[] cmdArgs) } if (frame is null) { Log("[cs-client] no frame received from foreign server"); return 4; } - using (frame) { (int w, int h) = frame.PixelSize(); byte[] got = new byte[w * h * 4]; diff --git a/src/Syphon.NET/IOSurfaceExtensions.cs b/src/Syphon.NET/IOSurfaceExtensions.cs index bb58b3d..3828bd3 100644 --- a/src/Syphon.NET/IOSurfaceExtensions.cs +++ b/src/Syphon.NET/IOSurfaceExtensions.cs @@ -29,11 +29,29 @@ public static bool IsNv12(this IOSurface.IOSurface surface) => surface.PixelFormat == FourCcNv12VideoRange || surface.PixelFormat == FourCcNv12FullRange; /// Number of planes (1 for packed BGRA, 2 for NV12). - public static int PlaneCount(this IOSurface.IOSurface surface) => (int)surface.PlaneCount; + /// + /// IOSurface itself reports 0 for a non-planar surface and rejects its per-plane accessors + /// outright. A packed surface is one plane everywhere it matters (it is what + /// describes at index 0), so it is reported as such here - matching CoreVideo's plane model. + /// + public static int PlaneCount(this IOSurface.IOSurface surface) => Math.Max(1, (int)surface.PlaneCount); /// Dimensions and row stride of a plane (plane 0 = luma/BGRA, plane 1 = NV12 CbCr), as ints. - public static (int Width, int Height, int BytesPerRow) PlaneInfo(this IOSurface.IOSurface surface, int plane) => - ((int)surface.GetWidth((nuint)plane), (int)surface.GetHeight((nuint)plane), (int)surface.GetBytesPerRow((nuint)plane)); + /// + /// On a non-planar surface the per-plane accessors raise NSGenericException ("surface is not + /// planar"), so plane 0 is answered from the surface itself. + /// + /// The surface has no such plane. + public static (int Width, int Height, int BytesPerRow) PlaneInfo(this IOSurface.IOSurface surface, int plane) + { + ArgumentOutOfRangeException.ThrowIfNegative(plane); + ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(plane, surface.PlaneCount()); + + if (surface.PlaneCount == 0) + return ((int)surface.Width, (int)surface.Height, (int)surface.BytesPerRow); + + return ((int)surface.GetWidth((nuint)plane), (int)surface.GetHeight((nuint)plane), (int)surface.GetBytesPerRow((nuint)plane)); + } // ---- Scoped CPU access ------------------------------------------------------------------------------- diff --git a/src/Syphon.NET/SyphonClient.cs b/src/Syphon.NET/SyphonClient.cs index b6907a2..3b83971 100644 --- a/src/Syphon.NET/SyphonClient.cs +++ b/src/Syphon.NET/SyphonClient.cs @@ -18,6 +18,9 @@ public sealed partial class SyphonClient : IDisposable private nint _handle; private GCHandle _self; private bool _firstFrameLogged; + // Strong reference to the most recent frame so the peer that callers hold is not finalized (and the + // surface released) between calls. + private IOSurface.IOSurface? _lastFrame; /// Raised on an arbitrary thread when a new frame becomes available. public event Action? FrameReady; @@ -70,16 +73,27 @@ public static SyphonClient Connect(ReadOnlySpan description, Action? onFra /// /// Return the latest frame's backing , or null if no new frame is available - /// since the last call. The surface is returned retained; dispose it once consumed (it releases the - /// retain). Zero-copy - the bytes live in shared GPU memory. + /// since the last call. Zero-copy - the bytes live in shared GPU memory. /// + /// + /// The surface belongs to the client: read it (for example with ) + /// and let it go - do not dispose it. A server publishes frames into one recycled surface, so + /// successive calls hand back the very same managed instance, and macOS bindings keep exactly one + /// managed peer per native object: disposing it would zero the handle of an instance the caller, the + /// publishing server and every later call still share, which then reports a surface with no size, no + /// planes and no pixels. The client holds the frame retained until the next call or until it is + /// disposed; keep the pixels, not the surface, if you need them longer. + /// public IOSurface.IOSurface? TryGetFrame() { ObjectDisposedException.ThrowIf(_handle == 0, this); nint surface = SyphonNative.sy_client_copy_new_frame(_handle); if (surface == 0) return null; if (!_firstFrameLogged) { _firstFrameLogged = true; LogFirstFrame(); } - return Runtime.GetINativeObject(surface, owns: true); + // owns: true consumes the shim's retain. When a peer for this surface already exists the runtime + // returns that instance and drops the extra retain, so the count stays flat across a frame loop. + _lastFrame = Runtime.GetINativeObject(surface, owns: true); + return _lastFrame; } [UnmanagedCallersOnly] @@ -102,6 +116,9 @@ public void Dispose() nint h = Interlocked.Exchange(ref _handle, 0); if (h != 0) SyphonNative.sy_client_destroy(h); if (_self.IsAllocated) _self.Free(); + // Drop the frame reference rather than disposing it: the peer may be shared with the publishing + // server (loopback) and with callers still holding it. + _lastFrame = null; } [LoggerMessage(Level = LogLevel.Debug, Message = "client created")] diff --git a/src/Syphon.NET/SyphonServer.cs b/src/Syphon.NET/SyphonServer.cs index 0bde053..4148016 100644 --- a/src/Syphon.NET/SyphonServer.cs +++ b/src/Syphon.NET/SyphonServer.cs @@ -26,6 +26,8 @@ public sealed partial class SyphonServer : IDisposable private readonly ILogger _logger; private nint _handle; private bool _firstPublishLogged; + // Strong reference to the surface most recently handed out by AcquireSurface; see the note there. + private IOSurface.IOSurface? _currentSurface; /// Create a server advertised to other applications under . /// Server name advertised to clients. @@ -68,6 +70,12 @@ public void Publish(IOSurface.IOSurface surface, bool flipped = false) /// Get a server-owned writable surface of the given size and format, recreated when the /// dimensions or format change. Write pixels into it, then call . /// + /// + /// The surface belongs to the server - do not dispose it. It is recycled while the size and + /// format hold, so successive calls hand back the very same managed instance (macOS bindings keep one + /// managed peer per native object); disposing it would zero the handle of an instance the server and + /// every later call still share, which then reports a surface with no size, no planes and no pixels. + /// public IOSurface.IOSurface AcquireSurface(int width, int height, CVPixelFormatType format = CVPixelFormatType.CV32BGRA) { ObjectDisposedException.ThrowIf(_handle == 0, this); @@ -75,9 +83,11 @@ public IOSurface.IOSurface AcquireSurface(int width, int height, CVPixelFormatTy ArgumentOutOfRangeException.ThrowIfNegativeOrZero(height); nint surface = SyphonNative.sy_server_acquire_surface(_handle, (uint)width, (uint)height, (uint)format); if (surface == 0) throw new InvalidOperationException("Failed to acquire a surface."); - // The server owns the surface (it recreates/releases it), so wrap it non-owning. - return Runtime.GetINativeObject(surface, owns: false) + // The server owns the surface (it recreates/releases it), so wrap it non-owning. Held onto so the + // peer callers write through is not finalized between acquire and publish. + _currentSurface = Runtime.GetINativeObject(surface, owns: false) ?? throw new InvalidOperationException("Failed to wrap the acquired surface."); + return _currentSurface; } /// Publish the surface most recently returned by . @@ -159,6 +169,9 @@ public void Dispose() { nint h = Interlocked.Exchange(ref _handle, 0); if (h != 0) SyphonNative.sy_server_destroy(h); + // Drop the reference rather than disposing: the peer is shared with any loopback client that + // received this surface, and with callers still holding it. + _currentSurface = null; } [LoggerMessage(Level = LogLevel.Debug, Message = "server created")] diff --git a/tests/Syphon.NET.Tests/SyphonTests.cs b/tests/Syphon.NET.Tests/SyphonTests.cs index 97d6c0c..9898420 100644 --- a/tests/Syphon.NET.Tests/SyphonTests.cs +++ b/tests/Syphon.NET.Tests/SyphonTests.cs @@ -58,7 +58,8 @@ private static void AssertLoopback(int w, int h) byte[] expected = Pattern(w, h); using SyphonClient client = server.CreateLoopbackClient(); - using IOSurface.IOSurface? surface = PollLatest(server, client, expected, w, h); + // Not disposed: delivered frames belong to the client (see SyphonClient.TryGetFrame). + IOSurface.IOSurface? surface = PollLatest(server, client, expected, w, h); Assert.IsNotNull(surface, "a published frame should be delivered to the loopback client"); (int gotW, int gotH) = surface.PixelSize(); @@ -76,7 +77,9 @@ private static void AssertLoopback(int w, int h) SyphonServer server, SyphonClient client, byte[] src, int w, int h) { // Publish repeatedly, keeping the most recent delivered frame and discarding an initial - // stale one by requiring a few publishes before accepting. + // stale one by requiring a few publishes before accepting. The frames are the client's to + // own - never dispose one here, or every later frame (the same managed peer) comes back with + // a zeroed handle. IOSurface.IOSurface? frame = null; var sw = Stopwatch.StartNew(); int published = 0; @@ -87,7 +90,6 @@ private static void AssertLoopback(int w, int h) IOSurface.IOSurface? f = client.TryGetFrame(); if (f is not null) { - frame?.Dispose(); frame = f; if (published >= 4) break; } @@ -96,6 +98,57 @@ private static void AssertLoopback(int w, int h) return frame; } + /// + /// A server recycles one surface, so every delivered frame is the same native object and the + /// bindings hand back the same managed peer. Polling a long run of frames has to keep yielding a + /// readable surface - it did not while the loop disposed each frame, which zeroed that shared peer. + /// + [TestMethod] + [TestCategory("Transport")] + public void RepeatedFrames_StayReadable() + { + const int w = 32, h = 16; + SyphonServer server = null!; + try + { + server = new SyphonServer("Syphon.NET Repeat Test"); + } + catch (DllNotFoundException) + { + Assert.Inconclusive("Native Syphon shim not present on this host."); + } + catch (PlatformNotSupportedException) + { + Assert.Inconclusive("No Metal device available on this host."); + } + + using (server) + { + byte[] expected = Pattern(w, h); + using SyphonClient client = server.CreateLoopbackClient(); + + int received = 0; + var sw = Stopwatch.StartNew(); + while (received < 10 && sw.Elapsed < TimeSpan.FromSeconds(10)) + { + server.PublishPixels(expected, w, h, CVPixelFormatType.CV32BGRA); + IOSurface.IOSurface? frame = client.TryGetFrame(); + if (frame is null) { Thread.Sleep(16); continue; } + + received++; + (int gotW, int gotH) = frame.PixelSize(); + Assert.AreEqual(w, gotW, $"frame {received} should still report its width"); + Assert.AreEqual(h, gotH, $"frame {received} should still report its height"); + + byte[] got = new byte[w * h * 4]; + frame.CopyTightlyPacked(got); + CollectionAssert.AreEqual(expected, got, $"frame {received} must round-trip"); + } + + Assert.AreEqual(10, received, "ten published frames should have been delivered"); + } + } + private static byte[] Pattern(int w, int h) { byte[] p = new byte[w * h * 4]; @@ -160,6 +213,74 @@ public void WritePixels_ThenCopyTightlyPacked_RoundTripsByteExact() } } + /// + /// IOSurface reports no planes for a packed surface and raises an Objective-C exception from its + /// per-plane accessors; the helpers present that surface as the single plane it is. + /// + [TestMethod] + [TestCategory("Transport")] + public void PlaneInfo_OnPackedBgra_DescribesTheWholeSurface() + { + const int w = 48, h = 32; + SyphonServer server = null!; + try + { + server = new SyphonServer("Syphon.NET Plane Test"); + } + catch (DllNotFoundException) + { + Assert.Inconclusive("Native Syphon shim not present on this host."); + } + catch (PlatformNotSupportedException) + { + Assert.Inconclusive("No Metal device available on this host."); + } + + using (server) + { + IOSurface.IOSurface surface = server.AcquireSurface(w, h, CVPixelFormatType.CV32BGRA); + Assert.AreEqual(1, surface.PlaneCount()); + + (int pw, int ph, int stride) = surface.PlaneInfo(0); + Assert.AreEqual(w, pw); + Assert.AreEqual(h, ph); + Assert.IsTrue(stride >= w * 4, "the stride must cover a row of pixels"); + + Assert.ThrowsExactly(() => surface.PlaneInfo(1)); + } + } + + /// + /// The server recycles its surface, and the bindings keep one managed peer per native object, so + /// repeated acquires are the same instance - which is why callers must not dispose it. + /// + [TestMethod] + [TestCategory("Transport")] + public void AcquireSurface_ReturnsTheRecycledInstance() + { + SyphonServer server = null!; + try + { + server = new SyphonServer("Syphon.NET Recycle Test"); + } + catch (DllNotFoundException) + { + Assert.Inconclusive("Native Syphon shim not present on this host."); + } + catch (PlatformNotSupportedException) + { + Assert.Inconclusive("No Metal device available on this host."); + } + + using (server) + { + IOSurface.IOSurface first = server.AcquireSurface(64, 64); + IOSurface.IOSurface second = server.AcquireSurface(64, 64); + Assert.AreSame(first, second); + Assert.AreEqual((64, 64), second.PixelSize()); + } + } + private static byte[] Pattern(int w, int h) { byte[] p = new byte[w * h * 4];