From c090cd693deac968b04f04f0676f7a5d764cb181 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 27 Jul 2026 23:14:51 +0200 Subject: [PATCH] fix(diag): make the frame capture name the framebuffer it reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FrameScreenshotController.ReadDefaultFramebuffer called glReadPixels without binding a read framebuffer, so it captured whatever was bound to GL_READ_FRAMEBUFFER at that moment rather than the default framebuffer its name promises. The capture runs at the end of PrivatePresentationRenderer.Render, after PrivateEntityViewportRenderer has drawn the paperdoll and appraisal views into its own FBO — an FBO it clears to exactly RGBA(0,0,0,0). A capture that inherits that binding writes a fully transparent PNG, which the repeat-run connected gate scores as BLANK even though the backbuffer on screen was correct. This was latent for as long as something rebound framebuffer 0 often enough to mask it. Before Campaign V slice V4c, GL BeginPass bound framebuffer 0 on every pass with a null colour target; V4c deliberately stopped doing that (plan §5.4) so the offscreen viewport renderers could keep their own target across a dispatcher draw. Removing the wide path exposed the narrow bug underneath it — the same latent-bug-masked-by-a- fallback class the project recorded for #98. The read now binds framebuffer 0 to GL_READ_FRAMEBUFFER, reads, and restores the caller's binding, so a diagnostic capture states its own source and cannot perturb the frame it observes. The GL calls move behind IDefaultFramebufferSurface so the bind/read/restore order is assertable without a GL context; two tests cover the ordering and the restore on a throwing read. Gates: Release build green; App tests 3,864 passed / 3 skipped (3,862 baseline plus the two new tests), no #250 flakes; offline pixel gate against 8dec163f PASS at a differing fraction of 4.26e-05 against the 0.001 threshold, inside the documented same-commit noise band. Co-Authored-By: Claude Fable 5 --- .../Diagnostics/FrameScreenshotController.cs | 104 ++++++++++++++++-- ...WorldLifecycleAutomationControllerTests.cs | 67 +++++++++++ 2 files changed, 160 insertions(+), 11 deletions(-) diff --git a/src/AcDream.App/Diagnostics/FrameScreenshotController.cs b/src/AcDream.App/Diagnostics/FrameScreenshotController.cs index 0170a5e3..a11380be 100644 --- a/src/AcDream.App/Diagnostics/FrameScreenshotController.cs +++ b/src/AcDream.App/Diagnostics/FrameScreenshotController.cs @@ -31,9 +31,18 @@ internal sealed class FrameScreenshotController GL gl, string directory, Action? log = null) - : this((width, height) => ReadDefaultFramebuffer(gl, width, height), directory, log) + : this( + CreateReader(gl), + directory, + log) + { + } + + private static Func CreateReader(GL gl) { ArgumentNullException.ThrowIfNull(gl); + var surface = new GlDefaultFramebufferSurface(gl); + return (width, height) => ReadDefaultFramebuffer(surface, width, height); } internal FrameScreenshotController( @@ -126,20 +135,93 @@ internal sealed class FrameScreenshotController return flipped; } - private static unsafe byte[] ReadDefaultFramebuffer(GL gl, int width, int height) + /// + /// The GL surface state a default-framebuffer capture touches, isolated so + /// the bind/read/restore order is assertable without a GL context. + /// + internal interface IDefaultFramebufferSurface { + /// The name currently bound to GL_READ_FRAMEBUFFER. + uint ReadFramebufferBinding { get; } + + void BindReadFramebuffer(uint framebuffer); + + void ReadRgba(int width, int height, byte[] destination); + } + + /// + /// Reads the default framebuffer — framebuffer name 0, the backbuffer — + /// and nothing else. + /// + /// + /// + /// glReadPixels reads whatever is bound to GL_READ_FRAMEBUFFER, + /// so a capture that does not name its source silently captures whichever + /// offscreen target the previous renderer left bound. The frame this runs in + /// draws several: PrivateEntityViewportRenderer clears its paperdoll + /// and appraisal FBOs to exactly RGBA(0,0,0,0), which is what a leaked + /// binding writes to disk — a fully transparent PNG that reads as a + /// blank-world failure while the backbuffer on screen was correct. + /// + /// + /// This was latent for as long as something else rebound framebuffer 0 often + /// enough to mask it (before Campaign V slice V4c, GL BeginPass did so + /// on every pass — see plan §5.4). The capture states its own source instead + /// of inheriting one, and restores the caller's binding so a diagnostic + /// capture cannot perturb the frame it observes. + /// + /// + internal static byte[] ReadDefaultFramebuffer( + IDefaultFramebufferSurface surface, + int width, + int height) + { + ArgumentNullException.ThrowIfNull(surface); byte[] pixels = new byte[checked(width * height * 4)]; - fixed (byte* pointer = pixels) + uint previous = surface.ReadFramebufferBinding; + surface.BindReadFramebuffer(0u); + try { - gl.ReadPixels( - 0, - 0, - (uint)width, - (uint)height, - PixelFormat.Rgba, - PixelType.UnsignedByte, - pointer); + surface.ReadRgba(width, height, pixels); + } + finally + { + surface.BindReadFramebuffer(previous); } return pixels; } + + private sealed class GlDefaultFramebufferSurface : IDefaultFramebufferSurface + { + private readonly GL _gl; + + public GlDefaultFramebufferSurface(GL gl) => _gl = gl; + + public uint ReadFramebufferBinding + { + get + { + _gl.GetInteger(GetPName.ReadFramebufferBinding, out int binding); + return (uint)binding; + } + } + + public void BindReadFramebuffer(uint framebuffer) => + _gl.BindFramebuffer(FramebufferTarget.ReadFramebuffer, framebuffer); + + public unsafe void ReadRgba(int width, int height, byte[] destination) + { + fixed (byte* pointer = destination) + { + _gl.ReadPixels( + 0, + 0, + (uint)width, + (uint)height, + PixelFormat.Rgba, + PixelType.UnsignedByte, + pointer); + } + } + } } diff --git a/tests/AcDream.App.Tests/Diagnostics/WorldLifecycleAutomationControllerTests.cs b/tests/AcDream.App.Tests/Diagnostics/WorldLifecycleAutomationControllerTests.cs index 43be61a7..d5bd1509 100644 --- a/tests/AcDream.App.Tests/Diagnostics/WorldLifecycleAutomationControllerTests.cs +++ b/tests/AcDream.App.Tests/Diagnostics/WorldLifecycleAutomationControllerTests.cs @@ -93,6 +93,73 @@ public sealed class WorldLifecycleAutomationControllerTests Assert.NotEmpty(error); } + /// + /// glReadPixels reads GL_READ_FRAMEBUFFER. A capture that does not name + /// framebuffer 0 captures whichever offscreen target the previous renderer + /// left bound - the paperdoll/appraisal FBOs clear to RGBA(0,0,0,0), so the + /// artifact is a fully transparent PNG that reads as a blank-world failure + /// while the backbuffer on screen was correct. + /// + [Fact] + public void DefaultFramebufferRead_BindsFramebufferZeroAndRestoresTheCallersBinding() + { + var surface = new RecordingFramebufferSurface(boundOnEntry: 7u, fill: 0x42); + + byte[] pixels = FrameScreenshotController.ReadDefaultFramebuffer(surface, 2, 3); + + Assert.Equal( + ["bind 0", "read 2x3", "bind 7"], + surface.Calls); + Assert.Equal(0u, surface.ReadFramebufferBindingDuringRead); + Assert.Equal(2 * 3 * 4, pixels.Length); + Assert.All(pixels, value => Assert.Equal(0x42, value)); + Assert.Equal(7u, surface.ReadFramebufferBinding); + } + + [Fact] + public void DefaultFramebufferRead_RestoresTheCallersBindingWhenTheReadThrows() + { + var surface = new RecordingFramebufferSurface(boundOnEntry: 9u, fill: 0) + { + ReadFailure = new InvalidOperationException("GL_OUT_OF_MEMORY"), + }; + + Assert.Throws( + () => FrameScreenshotController.ReadDefaultFramebuffer(surface, 1, 1)); + + Assert.Equal(["bind 0", "read 1x1", "bind 9"], surface.Calls); + Assert.Equal(9u, surface.ReadFramebufferBinding); + } + + private sealed class RecordingFramebufferSurface(uint boundOnEntry, byte fill) + : FrameScreenshotController.IDefaultFramebufferSurface + { + private uint _binding = boundOnEntry; + + public List Calls { get; } = []; + + public Exception? ReadFailure { get; init; } + + public uint? ReadFramebufferBindingDuringRead { get; private set; } + + public uint ReadFramebufferBinding => _binding; + + public void BindReadFramebuffer(uint framebuffer) + { + _binding = framebuffer; + Calls.Add($"bind {framebuffer}"); + } + + public void ReadRgba(int width, int height, byte[] destination) + { + Calls.Add($"read {width}x{height}"); + ReadFramebufferBindingDuringRead = _binding; + if (ReadFailure is not null) + throw ReadFailure; + Array.Fill(destination, fill); + } + } + [Fact] public void Checkpoint_WritesCanonicalRevealAndResourceSnapshot() {