From db8b3479520862cd47c546e96506b7332158abcf Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 16 Sep 2026 13:39:50 -0400 Subject: [PATCH] [Fix] (Remote): preserve server ownership and wait for ring retirement --- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 16 +++ .../Scenarios/PixelStoreSweepScenario.cpp | 38 +++++ MobileGL/MG_Remote/Client/ClientSession.cpp | 32 +++-- MobileGL/MG_Remote/Client/EmitTables.cpp | 19 +++ .../MG_Remote/Client/PersistentMapTracker.cpp | 13 ++ .../MG_Remote/Client/PersistentMapTracker.h | 11 ++ .../GLState/BufferState/BufferObject.cpp | 29 ++-- .../MG_Test/Wire/RemoteClientControls.inc | 132 ++++++++++++++++++ MobileGL/MG_Test/Wire/RemoteClientTest.cpp | 4 + MobileGL/MG_Test/Wire/ServerLoopTest.cpp | 44 ++++++ 10 files changed, 319 insertions(+), 19 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index b1883c2c..4dcc85e9 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -20,6 +20,7 @@ // R-11's server-owned staging copy. Header-only and package v1's; see its own header block for // why GLESBufferResource does not simply gain a member. #include +#include #endif #include "Utils.h" @@ -204,6 +205,21 @@ namespace MobileGL::MG_Backend::DirectGLES { // garbage for a later collection - there is none on this arm. void OnFrontendStateObjectDestroyed(MG_Pipe::MGPipeKind kind, Uint64 lifetimeId) { if (InProcessTeardown()) return; +#if MOBILEGL_BUILD_DISAGGREGATED + // Death notices have no framebuffer wire opcode. Keep the lifetime/slot valid + // until the context owner has destroyed its twin and updated its binding cache. + if (MG_Config::Transport != MG_Config::TransportMode::Monolith && + !MG_Remote::Server::ServerLoop::OnApplyThread()) { + struct Death { MG_Pipe::MGPipeKind kind; Uint64 lifetimeId; } death{kind, lifetimeId}; + MG_Remote::Server::ServerLoopInstance().RunOnApplyThread( + +[](void* user) -> MobileGLResult { + const auto& death = *static_cast(user); + OnFrontendStateObjectDestroyed(death.kind, death.lifetimeId); + return MOBILEGL_OK; + }, &death); + return; + } +#endif switch (kind) { case MG_Pipe::MGPipeKind::Texture: TextureImpl::g_backendTextureObjects.DestroyByLifetimeId(lifetimeId); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp index efc850fa..120a4fe0 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PixelStoreSweepScenario.cpp @@ -219,6 +219,44 @@ namespace MGITest { EXPECT_EQ(FirstGLError(), 0u); } + TEST_F(PixelStoreSweepScenario, ReadPixelsSwapsUnsignedShortComponentsAndPreservesPackGaps) { + if (!Ready()) return; + ResetAllPixelStoreModes(); + GLuint texture = 0, framebuffer = 0; + glGenTextures(1, &texture); + glBindTexture(GL_TEXTURE_2D, texture); + // RGBA8 expands to repeated bytes in a ushort and cannot expose a missing swap. + const GLfloat color[] = {0.1f, 0.2f, 0.3f, 0.4f}; + glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA16F, 1, 1, 0, GL_RGBA, GL_FLOAT, color); + glGenFramebuffers(1, &framebuffer); + glBindFramebuffer(GL_FRAMEBUFFER, framebuffer); + glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0); + ASSERT_EQ(glCheckFramebufferStatus(GL_FRAMEBUFFER), GLenum(GL_FRAMEBUFFER_COMPLETE)); + std::uint8_t native[8]{}; + glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_SHORT, native); + ASSERT_NE(native[0], native[1]) << "the source must contain a non-symmetric ushort"; + glPixelStorei(GL_PACK_SWAP_BYTES, GL_TRUE); + std::uint8_t tight[8]{}; + glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_SHORT, tight); + glPixelStorei(GL_PACK_ROW_LENGTH, 3); + glPixelStorei(GL_PACK_SKIP_ROWS, 1); + glPixelStorei(GL_PACK_SKIP_PIXELS, 1); + std::vector scattered(64, 0xCD); + glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_SHORT, scattered.data()); + for (std::size_t i = 0; i < sizeof(native); ++i) { + EXPECT_EQ(tight[i], native[i ^ 1]); + EXPECT_EQ(scattered[32 + i], native[i ^ 1]); + } + for (std::size_t i = 0; i < scattered.size(); ++i) { + if (i < 32 || i >= 40) EXPECT_EQ(scattered[i], 0xCD); + } + EXPECT_EQ(FirstGLError(), 0u); + ResetAllPixelStoreModes(); + glBindFramebuffer(GL_FRAMEBUFFER, 0); + glDeleteFramebuffers(1, &framebuffer); + glDeleteTextures(1, &texture); + } + // The leak regression. Each iteration is one complete CTS inner step, and every readback has // to be exactly the gradient THIS iteration uploaded - never the previous one's. Before the // missing destructors were added, the driver-side framebuffer count grew without bound here. diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 72c478fb..ec7bca66 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -714,17 +714,31 @@ namespace MobileGL::MG_Remote::Client { std::abort(); } - const Uint64 seq = + Uint64 seq = m_encoder.EncodeRecord(op, payload, payloadBytes, varTail, varTailBytes); if (seq == Wire::kInvalidSeq) { - // The ring refused it. NOT a silent drop and not a retry loop: R-10 says P5 does no - // chunking and must prove it needs none, so a refusal is the proof failing. - MGLOG_F("MGPipe: Fatal{RingOverrun, \"%s\"} - the command ring refused a %llu-byte " - "record. P5 does not chunk (R-10); this is the proof obligation failing, not " - "a back-pressure case", - Wire::WireOpName(op), - static_cast(payloadBytes + varTailBytes)); - std::abort(); + // EncodeRecord already refused individually oversized records. This one fits, + // but appliedSeq may be ahead of retiredTail: wait for actual reclaimable space, + // including the wrap pad. Prior EmitAndWait calls have published every record. + Wire::WireRecordLayout layout{}; + Wire::MGPipeWireRecordLayout(op, payload, layout); + const Uint64 toEnd = m_cmd.Capacity() - m_cmd.LocalHead() % m_cmd.Capacity(); + const Uint64 needed = layout.TotalBytes + (toEnd < layout.TotalBytes ? toEnd : 0); + const BarrierWaitScope waiting; + const auto wait = m_producer.WaitForCmdSpace(needed, kBarrierTimeoutMs); + if (wait != Transport::SessionWait::Reached) { + MGLOG_F("MGPipe: Fatal{RetirementWaitFailed, \"SEG_CMD\"} - %s needs %llu " + "reclaimable bytes; the retirement wait ended on %s", + Wire::WireOpName(op), static_cast(needed), + wait == Transport::SessionWait::ShutDown ? "shutdown" : "timeout"); + std::abort(); + } + seq = m_encoder.EncodeRecord(op, payload, payloadBytes, varTail, varTailBytes); + if (seq == Wire::kInvalidSeq) { + MGLOG_F("MGPipe: Fatal{RingOverrun, \"SEG_CMD\"} - %s refused after sufficient " + "space retired", Wire::WireOpName(op)); + std::abort(); + } } // Publish the head, record submittedSeq, THEN ring - in that order, which is diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index a0cf7b3a..96407cc1 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -344,6 +344,23 @@ namespace MobileGL::MG_Remote::Client { ReadbackBytesPerPixel(format, type); } + void ApplyReadbackByteSwap(void* pixels, Uint64 bytes, GLenum type, + const PixelStoreParameters& pack) { + if (!pack.SwapBytes || pixels == nullptr) return; + const auto dataType = MG_Util::ConvertGLEnumToTexturePixelDataType(type); + SizeT group = MG_Util::GetSizedTexturePixelDataTypeSize(dataType); + if (group == 0) group = MG_Util::GetBaseTexturePixelDataTypeSize(dataType); + // This packed depth/stencil type contains two independent 32-bit words. + if (type == GL_FLOAT_32_UNSIGNED_INT_24_8_REV) group = 4; + if (group <= 1) return; + auto* data = static_cast(pixels); + for (Uint64 at = 0; at + group <= bytes; at += group) { + for (SizeT i = 0; i < group / 2; ++i) { + std::swap(data[at + i], data[at + group - 1 - i]); + } + } + } + // M2 / codex 11: the reply the server posted is COMPLETE and OK. A short OK reply, and a // DECLINED or ERROR reply with a zero payload, both leave the destination full of stale // bytes; scattering or returning it is the silently truncated picture ID-47's own comment @@ -463,6 +480,7 @@ namespace MobileGL::MG_Remote::Client { // handed back as pixels. The Fatal aborts before the application reads the buffer, // so the bytes EmitAndWait already copied into `pixels` are never observed. RequireReadbackReplyComplete(status, replySize, tight); + ApplyReadbackByteSwap(pixels, tight, type, pack); return; } @@ -475,6 +493,7 @@ namespace MobileGL::MG_Remote::Client { // BEFORE THE SCATTER, so a short or non-OK reply never reaches the application's // pointer at all (the bounce is the only thing that held the partial bytes). RequireReadbackReplyComplete(status, replySize, tight); + ApplyReadbackByteSwap(bounce.data(), tight, type, pack); ScatterTightReadbackIntoPackState(bounce.data(), pixels, width, height, bytesPerPixel, pack); } diff --git a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp index 252d9a9b..a591e070 100644 --- a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp +++ b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp @@ -10,6 +10,7 @@ #include #include +#include #include @@ -34,6 +35,8 @@ namespace MobileGL::MG_Remote::Client { return MG_Config::Transport != MG_Config::TransportMode::Monolith; } + Bool PersistentMapTracker::OnServerRole() { return Server::ServerLoop::OnApplyThread(); } + // SyncPersistentMappedRange's early-out chain (BufferObject.cpp:341-353), in its order, // read as a membership test. Every line here has a line there; if one of them moves, the // unit case that drives both against each other is what says so. @@ -72,6 +75,11 @@ namespace MobileGL::MG_Remote::Client { void PersistentMapTracker::PushBlocksFor(BufferObject& buffer) { if (!PushIsArmed()) return; + if (OnServerRole()) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"PushBlocksFor\"} - the persistent-map " + "producer belongs to the client; the server consumes transported bytes"); + std::abort(); + } // Re-checked rather than trusted. The set is maintained at five events and a sixth // one arriving without a NoteMapStateChanged would otherwise push a buffer whose // shadow has been released - an adopted store's Bytes() is the GPU map, and reading @@ -120,6 +128,11 @@ namespace MobileGL::MG_Remote::Client { void PersistentMapTracker::PushAllMembers() { if (!PushIsArmed()) return; + if (OnServerRole()) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"PushAllMembers\"} - the persistent-map " + "producer belongs to the client; the server consumes transported bytes"); + std::abort(); + } if (m_livePersistentMaps.empty()) return; // Copied out first: PushBlocksFor can erase its own entry (a member that stopped // being one), and ska::flat_hash_map invalidates on erase. diff --git a/MobileGL/MG_Remote/Client/PersistentMapTracker.h b/MobileGL/MG_Remote/Client/PersistentMapTracker.h index cffca073..e712016b 100644 --- a/MobileGL/MG_Remote/Client/PersistentMapTracker.h +++ b/MobileGL/MG_Remote/Client/PersistentMapTracker.h @@ -72,6 +72,17 @@ namespace MobileGL::MG_Remote::Client { // resource_subdata record there would be new behaviour, which D-J forbids. static Bool PushIsArmed(); + // r1 (P5-close codex finding 1, ID-52): true on the SERVER role's thread - the apply thread + // of a running ServerLoop, which under inproc lives in this very process. This module is + // the CLIENT's producer: it reads the frontend object's MappedData() and pushes it as + // resource_subdata. On the apply thread that record is routed through the monolith adapter + // straight into the server's shadow (Ops_H_SubData), which REPLACES the transported bytes + // with client memory - and under an active transport the staged copy is the draw's ONLY + // base (ID-52 item 3). So the producer asks this before it runs: on the server role the + // answer is "there is nothing to sync" (SyncPersistentMappedRange) or a refusal by name + // (PushMappedSpanBlock). False in every monolith process, where no ServerLoop runs. + static Bool OnServerRole(); + // SyncPersistentMappedRange's early-out chain as a predicate. THE only spelling. static Bool IsLivePersistentMap(const MG_State::GLState::BufferObject& buffer); diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp index 942d572d..2cdd41cb 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp @@ -18,6 +18,9 @@ // edge exists only in a build that has the transport at all. #include #include +#include + +#include #include #endif @@ -375,17 +378,11 @@ namespace MobileGL::MG_State::GLState { void BufferObject::SyncPersistentMappedRange() { #if MOBILEGL_BUILD_DISAGGREGATED - // SPLIT: the same span, cut into MOBILEGL_IPC_PERSISTENT_BLOCK_KB blocks, and the - // membership test is this function's own early-out chain read by the tracker rather - // than re-derived there. The 21 sites that call this (CONTRACT-P5.md section 3: 9 - // Espryt + 12 Magma) therefore keep pushing at exactly the points monolith pushes - // at, which is what makes the split arm comparable to the monolith one at all; they - // retire into MG_Remote::Client::PushPersistentMapsBeforeVerb at P8. - // - // A block size of 0 disables the push (E3(a)'s negative control) and this is where - // that is felt: the bytes the application wrote through the pointer never leave, the - // frame draws the last uploaded ones, and PersistentCoherentMapScenario goes red. + // Split's client pre-verb hook publishes these bytes. The retained backend sync + // sites must do nothing on the apply thread: re-entering this producer there would + // overwrite the server shadow through the monolith adapter without crossing the wire. if (MG_Remote::Client::PersistentMapTracker::PushIsArmed()) { + if (MG_Remote::Client::PersistentMapTracker::OnServerRole()) return; MG_Remote::Client::PersistentMapTracker::Instance().PushBlocksFor(*this); return; } @@ -409,6 +406,16 @@ namespace MobileGL::MG_State::GLState { // defined-content promotion and the legacy-ops fallback are what the monolith span // push does, and a second route to the same record is a second thing to keep in step. if (size == 0) return; + // Assert ownership at the final producer entry too, before serial/aggregate updates. + if (MG_Remote::Client::PersistentMapTracker::OnServerRole()) { + MGLOG_F("MGPipe: Fatal{RoleViolation, \"PushMappedSpanBlock\"} - the server role (the apply " + "thread) reached the CLIENT persistent-map producer for buffer lifetime %llu " + "[%zu, +%zu). Under an active transport the server's staged copy is the draw's " + "only base (ID-52); a push from here would replace the transported bytes with " + "client memory through the monolith adapter", + static_cast(m_lifetimeId), offset, size); + std::abort(); + } NotifySubData(offset, size); if (MG_Util::PipeStats::Enabled()) { // THE persistent-map-push SITE: the bytes an application wrote through a map with @@ -429,6 +436,8 @@ namespace MobileGL::MG_State::GLState { void BufferObject::NotePersistentMapStateChanged() { if (!MG_Remote::Client::PersistentMapTracker::PushIsArmed()) return; MG_Remote::Client::PersistentMapTracker::Instance().NoteMapStateChanged(*this); + // Teardown must retire membership, but only the client publishes live-host-write state. + if (MG_Remote::Client::PersistentMapTracker::OnServerRole()) return; // THE LIVE-HOST-WRITES BIT (ARCHITECTURE.md 12, CONTRACT-P5.md section 3). A live // WRITE map - persistent or not - mutates the shadow with no call, no serial and no diff --git a/MobileGL/MG_Test/Wire/RemoteClientControls.inc b/MobileGL/MG_Test/Wire/RemoteClientControls.inc index 8bc21336..1f6c4d52 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientControls.inc +++ b/MobileGL/MG_Test/Wire/RemoteClientControls.inc @@ -393,4 +393,136 @@ int RunPackGpuControl(bool split) { ok ? "PASS" : "FAIL", DescribeStatus(child).c_str(), child.Log.c_str()); return ok ? 0 : 1; } +namespace { +struct PatternReadPeer : ReadPeer { + Bool OnReadPixels(const MGPReadbackInfo& info, Uint64 seq, Codec::ReplySink* replies) override { + Uint8 bytes[32]; + for (Uint8 i = 0; i < sizeof(bytes); ++i) bytes[i] = i + 1; + replies->PostReply(seq, 0, bytes, info.DstSize); + return true; + } +}; + +std::atomic retirementRetryStarted{false}; +std::atomic retirementWaitObserved{false}; +void HoldRetirementUntilProducerWaits() { + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(2); + while (std::chrono::steady_clock::now() < deadline) { + if (retirementRetryStarted.load() && ClientSessionInstance().Control()->producerParked.load()) { + retirementWaitObserved.store(true); + break; + } + std::this_thread::yield(); + } + Srv::ServerLoopInstance().SetBeforeRetireHookForTesting(nullptr); +} + +struct DrawPeer : Codec::WireVerbSink { + Bool OnDrawVbo(const MGPDrawInfo&, const MGPDrawRange*, const MGHostSpan*, + const MGPDrawIndirect*) override { return true; } + void Install() { + Srv::ServerLoopInstance().RunOnApplyThread([](void* self) { + auto& decoder = Srv::ServerSessionInstance().Applier().*PeerMember(DecoderTag{}); + decoder.SetVerbSink(static_cast(self)); + return MOBILEGL_OK; + }, this); + } +}; +} + +TEST(RemoteClientControls, ReadPixelsSwapsComponentsInTightAndScatteredReplies) { + const auto child = RunInChild([] { + StartControlSession(); + MG_State::pGLContext = MakeUnique(); + PatternReadPeer peer; + peer.Install(); + struct Pair { GLenum format; GLenum type; Uint32 bpp; Uint32 group; }; + for (const Pair pair : {Pair{GL_RGBA, GL_UNSIGNED_SHORT, 8, 2}, + Pair{GL_RGBA, GL_FLOAT, 16, 4}, + Pair{GL_RGBA, GL_UNSIGNED_INT_8_8_8_8, 4, 4}, + Pair{GL_DEPTH_STENCIL, GL_FLOAT_32_UNSIGNED_INT_24_8_REV, 8, 4}, + Pair{GL_RGBA, GL_UNSIGNED_BYTE, 4, 1}}) { + for (int scatter : {0, 1}) for (int swap : {0, 1}) { + auto& context = *MG_State::pGLContext; + context.SetPixelStoreParam(PixelStoreParam::PackSwapBytes, swap); + context.SetPixelStoreParam(PixelStoreParam::PackRowLength, scatter ? 3 : 0); + context.SetPixelStoreParam(PixelStoreParam::PackSkipPixels, scatter); + Uint8 result[160]; + std::memset(result, 0xCD, sizeof(result)); + RemoteEmitTable().GL.ReadPixels(0, 0, 1, 2, pair.format, pair.type, result); + for (Uint32 at = 0; at < sizeof(result); ++at) { + Uint8 expected = 0xCD; + for (Uint32 row = 0; row < 2; ++row) { + const Uint32 begin = (scatter ? 1 + 3 * row : row) * pair.bpp; + if (at >= begin && at < begin + pair.bpp) { + Uint32 i = at - begin; + if (swap) i = (i / pair.group) * pair.group + pair.group - 1 - i % pair.group; + expected = row * pair.bpp + i + 1; + } + } + if (result[at] != expected) ::_exit(110); + } + } + } + ClientSessionInstance().Stop(); + }); + ExpectChildSuccess(child); +} + +TEST(RemoteClientControls, PersistentMapProducerRefusesTheApplyThread) { + const auto child = RunInChild([] { + StartControlSession(); + Srv::ServerLoopInstance().RunOnApplyThread([](void*) { + PersistentMapTracker::Instance().PushAllMembers(); + return MOBILEGL_OK; + }, nullptr); + }); + ExpectNamedAbort(child, "Fatal{RoleViolation, \"PushAllMembers\"}"); +} + +TEST(RemoteClientControls, CommandWrapWaitsForRetirementAfterThePriorRecordApplied) { + const auto child = RunInChild([] { + MG_Config::Ipc.RingMb = 1; + StartControlSession(); + DrawPeer peer; + peer.Install(); + Vector ranges(32768); + MGPDrawInfo info{}; + auto emit = [&](Uint32 count) { + info.NumDraws = count; + return ClientSessionInstance().EmitAndWait(MGPWireOp::DrawVbo, &info, sizeof(info), + ranges.data(), count * sizeof(MGPDrawRange), nullptr, 0, nullptr); + }; + emit(28000); // Position the next record so that its successor needs a wrap pad. + peer.Install(); // A mailbox round trip also lets the previous drain retire. + Srv::ServerLoopInstance().SetBeforeRetireHookForTesting(&HoldRetirementUntilProducerWaits); + const auto prior = emit(32768); + if (ClientSessionInstance().Control()->retiredSeq.load() >= prior) ::_exit(111); + retirementRetryStarted.store(true); + const auto next = emit(32768); + if (next != prior + 1 || !retirementWaitObserved.load()) ::_exit(112); + ClientSessionInstance().Stop(); + }); + ExpectChildSuccess(child); +} + +TEST(RemoteClientControls, StagingWaitsForRetirementAfterThePriorRecordApplied) { + const auto child = RunInChild([] { + MG_Config::Ipc.StageMb = 1; + StartControlSession(); + Vector bytes(768 * 1024, 0x57); + auto& session = ClientSessionInstance(); + session.Encoder().StageBytes(bytes.data(), bytes.size()); + Srv::ServerLoopInstance().SetBeforeRetireHookForTesting(&HoldRetirementUntilProducerWaits); + MGPBindRenderState bind{}; + const auto prior = session.EmitAndWait(MGPWireOp::BindRenderState, &bind, sizeof(bind), + nullptr, 0, nullptr, 0, nullptr); + if (session.Control()->retiredSeq.load() >= prior) ::_exit(113); + retirementRetryStarted.store(true); + session.Encoder().StageBytes(bytes.data(), bytes.size()); + if (!retirementWaitObserved.load() || session.Encoder().StageReclaimWaits() != 1) ::_exit(114); + session.Stop(); + }); + ExpectChildSuccess(child); +} #endif diff --git a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp index acef2348..0cf679e9 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp +++ b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -51,6 +52,9 @@ #include #include #include +#include +#include +#include #if !defined(_WIN32) #include diff --git a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp index 10bbe4b9..d68a05e1 100644 --- a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp @@ -1629,6 +1629,44 @@ TEST(StagedShadowProductionTest, ATwinSurvivingContextLossDropsItsFreedShadowBas // shows it": B is the corruption, the upload is the draw, the mapped read-back is the picture. Red // once by restoring the MappedData() fallback in liveHostBase(): the store then holds A, the // client's bytes, and the R-2.5 audit could never reach a draw again. +TEST(ServerLoopEglTest, FrontendFramebufferDeathDeletesOnTheContextOwner) { + MG_Config::Features.PipePush |= MG_Pipe::kMGPipeSubsystemEsprytSlots; + EglServerFixture fixture; + MGL_EGL_BRING_UP_OR_BAIL(fixture); + ASSERT_TRUE(fixture.MakeCurrent()); + namespace GLES = MG_Backend::DirectGLES; + static auto nativeDelete = GLES::g_GLESFuncs.glDeleteFramebuffers; + static std::atomic deleted{0}; + static std::atomic wrongThread{false}; + nativeDelete = GLES::g_GLESFuncs.glDeleteFramebuffers; + deleted.store(0); + wrongThread.store(false); + auto framebuffer = MakeShared(901u); + GLuint driverId = 0; + ASSERT_EQ(OnApply([&] { + auto& twin = GLES::FramebufferImpl::g_backendFramebufferObjects.GetOrCreate(framebuffer); + twin = MakeShared(); + driverId = twin->GetBackendFramebufferId(); + twin->Bind(FramebufferTarget::Draw); + GLES::g_GLESFuncs.glDeleteFramebuffers = +[](GLsizei count, const GLuint* names) { + if (!Server::ServerLoop::OnApplyThread()) wrongThread.store(true); + deleted.fetch_add(count); + nativeDelete(count, names); + }; + }), MOBILEGL_OK); + ASSERT_NE(driverId, 0u); + framebuffer.reset(); // Frontend destructor runs on this client thread. + EXPECT_EQ(deleted.load(), 1u); + EXPECT_FALSE(wrongThread.load()); + ASSERT_EQ(OnApply([&] { + GLES::g_GLESFuncs.glDeleteFramebuffers = nativeDelete; + GLint bound = -1; + GLES::g_GLESFuncs.glGetIntegerv(GL_DRAW_FRAMEBUFFER_BINDING, &bound); + EXPECT_EQ(bound, 0) << "native deletion must unbind the framebuffer in the owner context"; + }), MOBILEGL_OK); + fixture.TearDown(); +} + TEST(StagedShadowProductionTest, TheEnsurePathUploadsTheServerShadowNotTheClientObjectsBytes) { EglServerFixture fixture; MGL_EGL_BRING_UP_OR_BAIL(fixture); @@ -1642,6 +1680,12 @@ TEST(StagedShadowProductionTest, TheEnsurePathUploadsTheServerShadowNotTheClient ASSERT_EQ(buffer->MappedData()[0], 0xA5); ASSERT_TRUE(buffer->HasDefinedContent()); + // Keep this object coherently mapped: Ensure's retained backend sync site must not + // re-enter the client producer and replace the staged 0x5B bytes with this 0xA5 map. + ASSERT_NE(buffer->AcquireMemoryRange({0, 64}, BufferMappingAccessBit::Write | + BufferMappingAccessBit::Persistent | BufferMappingAccessBit::Coherent), nullptr); + ASSERT_TRUE(buffer->IsMapped()); + // The server shadow for the twin the draw will use: B, staged the way the wire delivers it. const MG_Pipe::MGPipeHandle res = DeclareBuffer(BufferDesc(26, 64, true)); StageBytes(res, 0, 64, 0x5B);