[Fix] (SharedImage): uploads into a texture bound to a shared image are written into the image, and a producer's flush sends them first

MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp

@@ -18856,6 +18856,8 @@ namespace MobileGL::MG_Backend::DirectGLES {
         Bool PublishPendingAccesses() {
             ReadFrame& frame = CurrentReadFrame();
             frame.ImplicitSync = true;
+            // Sub-data waiting for a sync that nothing of this session would otherwise trigger.
+            if (t_boundNativeContext != EGL_NO_CONTEXT) TextureImpl::WritePendingSharedImageUploads();
             if (frame.Tracker.Pending() == 0) return true;
             int fence = -1;
             if (t_boundNativeContext != EGL_NO_CONTEXT && !ExportFence(&fence)) {

MobileGL/MG_Backend/DirectGLES/Managers.cpp

@@ -8858,27 +8858,12 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 return false;
             }
 
-            // Sub-data aimed at the image is not replayed into it - the image is written by the
-            // producer's present - but it must still leave the pending set, or the per-draw clean
-            // gate (IsDrawSyncCleanByRecord) would never close.
-            const auto dropPendingUploads = [&record] {
-                if (record.PendingUploads.empty()) return;
-                MGLOG_W_ONCE("Shared image texture %u: sub-data on an attached shared image is dropped",
-                             record.Desc.GlNameForDiag);
-                Vector<std::pair<Uint16, Uint16>> keys;
-                keys.reserve(record.PendingUploads.size());
-                for (const auto& pending : record.PendingUploads) {
-                    keys.emplace_back(MG_Pipe::MGPipeSubDataUploadTargetOf(pending.UploadTarget), pending.Level);
-                }
-                for (const auto& key : keys) ConsumePipeTextureUpload(record.Desc.Resource, key.first, key.second);
-            };
-
             // Steady state: bound to this image already (the serial moves with sub-data and with a
             // metadata respecify, neither of which changes what the name samples). A failed bind is
-            // retried only once the record moves.
+            // retried only once the record moves. Pending sub-data is the caller's
+            // (ReplaySharedImageUploads), after the image's sync state has been acquired.
             if (m_sharedImageId == imageId && m_isInitialized &&
                 (m_sharedImage != nullptr || m_syncedResourceSerial == record.Serial)) {
-                dropPendingUploads();
                 m_syncedResourceSerial = record.Serial;
                 return true;
             }
@@ -8893,7 +8878,6 @@ namespace MobileGL::MG_Backend::DirectGLES {
             // Whatever comes next redefines storage through a re-mint (see the imageId == 0 arm).
             m_backendStorageImmutable = true;
             m_syncedResourceSerial = record.Serial;
-            dropPendingUploads();
 
             const TextureTarget target = BufferImpl::StagedTextureTargetForPipeTarget(record.Desc.Target);
             if (ConvertTextureTargetToBackendGLEnum(target) != GL_TEXTURE_2D) {
@@ -8915,6 +8899,7 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 return true;
             }
             m_sharedImage = std::move(image);
+            m_sharedImageUploadsRgb = false;
             // The shape the name now has: the descriptor's level 0, and that one level only.
             m_prevTextureInfo = {static_cast<TextureInternalFormat>(record.Desc.InternalFormat),
                                  record.Desc.Width,
@@ -8926,6 +8911,83 @@ namespace MobileGL::MG_Backend::DirectGLES {
                                  record.Desc.FixedSampleLocations != 0};
             return true;
         }
+
+        // SUB-DATA ON A SHARED IMAGE IS WRITTEN INTO IT. An X server drawing with glamor uploads
+        // its clients' pixels (MIT-SHM / PutImage) into window pixmaps whose level 0 is a shared
+        // image, with glTexSubImage2D: the texels are in the staged store (the client defined the
+        // level without data, so only the uploaded boxes are covered), and they go into the
+        // EGLImage-backed name box by box. Every pending entry leaves the set either way, or the
+        // per-draw clean gate would never close. Called after AcquireForSampling, so the upload is
+        // ordered after the image's pending reads and writes like any other use.
+        void BackendTextureObject::ReplaySharedImageUploads(const MG_Pipe::MGPipeResourceRecord& record) {
+            if (record.PendingUploads.empty()) return;
+            Vector<std::pair<Uint16, Uint16>> keys;
+            Vector<std::pair<Uint16, MG_Pipe::MGPBox>> boxes;
+            keys.reserve(record.PendingUploads.size());
+            for (const auto& pending : record.PendingUploads) {
+                const Uint16 uploadHalf = MG_Pipe::MGPipeSubDataUploadTargetOf(pending.UploadTarget);
+                keys.emplace_back(uploadHalf, pending.Level);
+                if (pending.Level != 0) continue; // an image has one level
+                if (pending.Regions.empty()) {
+                    boxes.emplace_back(uploadHalf, pending.UnionBox);
+                } else {
+                    for (const auto& region : pending.Regions)
+                        boxes.emplace_back(uploadHalf, MG_Pipe::MGPBox{region.X, region.Y, region.Z, region.W, region.H,
+                                                                       region.D});
+                }
+            }
+            if (m_sharedImage != nullptr && !boxes.empty()) {
+                auto& store = MG_Remote::Server::ServerStagedTexture();
+                const Uint64 key = MG_Remote::Server::StagedTextureStore::KeyForHandle(record.Desc.Resource);
+                Bind(GL_TEXTURE_2D);
+                ScopedDefaultUnpackState unpackState;
+                BufferImpl::BindPixelUnpackBufferId(0);
+                for (const auto& [uploadHalf, box] : boxes) {
+                    Int rowTexels = 0;
+                    // A shared image is RGBA8 to every client that names it (the level the client
+                    // defined over it): four bytes a texel.
+                    const Uint8* bytes =
+                        store.CoveredBoxBytes(key, uploadHalf, 0, box.X, box.Y, box.W, box.H, 4, &rowTexels);
+                    if (bytes == nullptr) {
+                        MGLOG_E_ONCE("Shared image texture %u: an upload of %ux%u at (%d,%d) has no staged texels; "
+                                     "not written",
+                                     record.Desc.GlNameForDiag, box.W, box.H, box.X, box.Y);
+                        continue;
+                    }
+                    const Uint8* origin =
+                        bytes + (static_cast<SizeT>(box.Y) * static_cast<SizeT>(rowTexels) + static_cast<SizeT>(box.X)) * 4;
+                    if (!m_sharedImageUploadsRgb) {
+                        while (g_GLESFuncs.glGetError() != GL_NO_ERROR) {
+                        }
+                        g_GLESFuncs.glPixelStorei(GL_UNPACK_ROW_LENGTH, rowTexels);
+                        g_GLESFuncs.glTexSubImage2D(GL_TEXTURE_2D, 0, box.X, box.Y, static_cast<GLsizei>(box.W),
+                                                    static_cast<GLsizei>(box.H), GL_RGBA, GL_UNSIGNED_BYTE, origin);
+                        g_GLESFuncs.glPixelStorei(GL_UNPACK_ROW_LENGTH, 0);
+                        // An image of an X format (no alpha) is an RGB8 texture to the driver, which
+                        // takes only RGB texels: this image's uploads are converted from now on.
+                        if (g_GLESFuncs.glGetError() != GL_INVALID_OPERATION) continue;
+                        m_sharedImageUploadsRgb = true;
+                    }
+                    Vector<Uint8> rgb(static_cast<SizeT>(box.W) * box.H * 3);
+                    for (Uint32 row = 0; row < box.H; ++row) {
+                        const Uint8* src = origin + static_cast<SizeT>(row) * static_cast<SizeT>(rowTexels) * 4;
+                        Uint8* dst = rgb.data() + static_cast<SizeT>(row) * box.W * 3;
+                        for (Uint32 x = 0; x < box.W; ++x) {
+                            dst[x * 3 + 0] = src[x * 4 + 0];
+                            dst[x * 3 + 1] = src[x * 4 + 1];
+                            dst[x * 3 + 2] = src[x * 4 + 2];
+                        }
+                    }
+                    g_GLESFuncs.glPixelStorei(GL_UNPACK_ALIGNMENT, 1);
+                    g_GLESFuncs.glTexSubImage2D(GL_TEXTURE_2D, 0, box.X, box.Y, static_cast<GLsizei>(box.W),
+                                                static_cast<GLsizei>(box.H), GL_RGB, GL_UNSIGNED_BYTE, rgb.data());
+                    g_GLESFuncs.glPixelStorei(GL_UNPACK_ALIGNMENT, 4);
+                }
+                MGLOG_I_ONCE("Shared image texture %u: sub-data written into its image (%s texels)",
+                             record.Desc.GlNameForDiag, m_sharedImageUploadsRgb ? "RGB" : "RGBA");
+            }
+            for (const auto& [uploadHalf, level] : keys) ConsumePipeTextureUpload(record.Desc.Resource, uploadHalf, level);
+        }
 #endif
 
         void BackendTextureObject::SyncMipmapsToBackend(
@@ -8979,6 +9041,7 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 // a write that landed since the last one is waited for on this context's GPU
                 // before the command this sync is for, and the session's frame notes the read.
                 if (m_sharedImage != nullptr) SharedImageImpl::AcquireForSampling(m_sharedImage);
+                ReplaySharedImageUploads(*pushedStorage);
                 return;
             }
             // P5e (tx2), CONTRACT-P5E §5.2 (scout G-S2-2): THE VIEW TEST MOVES BEHIND THE
@@ -11248,6 +11311,20 @@ namespace MobileGL::MG_Backend::DirectGLES {
             g_boundTexturesCache;
         TwinRegistry<MG_State::GLState::ITextureObject, BackendTextureObject, MG_Pipe::MGPipeKind::Texture> g_backendTextureObjects;
 
+#if MOBILEGL_BUILD_DISAGGREGATED
+        void WritePendingSharedImageUploads() {
+            auto& applier = MG_Pipe::MGPipeApplier();
+            for (SizeT slot = 0; slot < applier.TextureResources.size(); ++slot) {
+                const auto& record = applier.TextureResources[slot];
+                if (!record.Live || record.SharedImageId == 0 || record.PendingUploads.empty()) continue;
+                const MG_Pipe::MGPipeHandle handle{static_cast<Uint32>(slot), record.Gen};
+                auto* twin = g_backendTextureObjects.FindByHandle(handle);
+                if (twin == nullptr || !*twin) continue;
+                (*twin)->SyncMipmapsToBackendByHandle(handle);
+            }
+        }
+#endif
+
         void NoteDriverSideTextureWrite(const SharedPtr<MG_State::GLState::ITextureObject>& textureObject) {
             SharedPtr<MG_State::GLState::ITextureObject> storage = textureObject;
             // Bounded: GL resolves a view of a view to the original storage, so this takes one step.

MobileGL/MG_Backend/DirectGLES/Managers.h

@@ -2111,6 +2111,10 @@ namespace MobileGL::MG_Backend::DirectGLES {
             // not go on to allocate or upload. False when the record names no image: a twin that
             // was bound to one has been told to re-mint, and the normal storage path takes over.
             Bool SyncSharedImageStorage(const MG_Pipe::MGPipeResourceRecord& record);
+            // Pending sub-data written into the attached shared image (Managers.cpp).
+            void ReplaySharedImageUploads(const MG_Pipe::MGPipeResourceRecord& record);
+            // The attached image takes RGB texels only (an X format): uploads are converted.
+            Bool m_sharedImageUploadsRgb = false;
 #endif
             // Whether this texture's ES storage was minted in an image carrier rather than in the
             // frontend format's own layout - the readback has to ask, because for a NORMALIZED
@@ -2483,6 +2487,12 @@ namespace MobileGL::MG_Backend::DirectGLES {
         // FBO's texture attachments - onto the backend, through the two borrowed-pair memos
         // documented at their definitions. Declared here so tests can drive those memos directly.
         void SyncNeccessaryTextures();
+#if MOBILEGL_BUILD_DISAGGREGATED
+        // Every texture of this session bound to a shared image with sub-data still pending is
+        // synced now, so the uploads are in the images before a flush publishes them (an X
+        // server's PutImage into a window pixmap no later draw of its own samples).
+        void WritePendingSharedImageUploads();
+#endif
         extern Array<Array<BackendTextureObject*, (SizeT)TextureTarget::TextureTargetCount>,
                      MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS>
             g_boundTexturesCache;

MobileGL/MG_Impl/EGLImpl/EGLImpl.cpp

@@ -1458,6 +1458,10 @@ namespace MobileGL::MG_Impl::EGLImpl {
                                                ownsServerWindow, std::getenv("MOBILEGL_SHARED_IMAGE_FLUSH_SYNC")))
             return;
         auto* backendObject = MG_Backend::pActiveBackendObject.get();
+#if MOBILEGL_BUILD_DISAGGREGATED
+        // Uploads still waiting for a validate point go first: they are part of what is published.
+        MG_Pipe::MGPipeClientDrainTextureUploads();
+#endif
         if (backendObject != nullptr && !backendObject->FlushSharedImageAccesses()) {
             MGLOG_E_ONCE("glFlush: this session's shared-image accesses could not be published; another session may "
                          "read them early");

MobileGL/MG_Impl/Pipe/PipeFill.cpp

@@ -3987,6 +3987,20 @@ namespace MobileGL::MG_Pipe {
 #endif
     }
 
+    void MGPipeClientDrainTextureUploads() {
+        if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return;
+        if (MG_Remote::Client::RunsAsTheServerRole()) return;
+        auto* ctx = LiveContext();
+        if (ctx == nullptr) return;
+        const Uint64 pushMask = MG_Config::Features.PipePush;
+        if ((pushMask & kMGPipeSubsystemTextureResources) != 0 &&
+            (kMGPipeWiredSubsystems & kMGPipeSubsystemTextureResources) != 0 &&
+            P4aFamilyHasItsConsumer(kMGPipeSubsystemTextureResources) &&
+            P4aFamilyDependenciesAreSet(kMGPipeSubsystemTextureResources, pushMask)) {
+            (void)MGPipeTextureEmitterInstance().DrainTextureSubData(*ctx);
+        }
+    }
+
 #if MOBILEGL_BUILD_DISAGGREGATED
     // Declared in PipeMutation.h, where the list is argued. THE ORDER: the latch goes first, so
     // everything after it - a pin's last owner dying, a deferred destroy replayed, an evicted

MobileGL/MG_Pipe/PipeApply.cpp

@@ -2371,7 +2371,12 @@ namespace MobileGL::MG_Pipe {
         // the mask arrives - and by construction only the non-storage fields differ.
         record->Desc = desc;
         // A shared image is the storage this call replaces - unless the call replaces none.
-        if (level != nullptr || !metadataOnly) record->SharedImageId = 0;
+        if ((level != nullptr || !metadataOnly) && record->SharedImageId != 0) {
+            MGLOG_D("MGPipe: texture %u leaves shared image %llu: a respecify (%s) redefined its storage",
+                    desc.GlNameForDiag, static_cast<unsigned long long>(record->SharedImageId),
+                    level != nullptr ? "one level" : "whole resource");
+            record->SharedImageId = 0;
+        }
 #if MOBILEGL_BUILD_DISAGGREGATED
         if (level != nullptr && desc.Target != static_cast<Uint8>(MGPipeResourceTarget::TexBuffer)) {
             // Scope and BufOffset/BufSize are transient respecify carriers, not resource state.

MobileGL/MG_Pipe/PipeMutation.h

@@ -266,6 +266,11 @@ namespace MobileGL::MG_Pipe {
     void MGPipeClientFinish();
     // Submit a stream link's published prefix; never wait for appliedSeq.
     void MGPipeClientFlush();
+    // The texture sub-data drain of a validate point, outside one: a shared-image producer's
+    // glFlush (an X server's PutImage into a window pixmap is followed by no draw of its own)
+    // has to get its uploads to the server before the flush publishes the images. Gated as the
+    // validate point's drain is; a no-op on a monolith and on the server role.
+    void MGPipeClientDrainTextureUploads();
 
     // A FRESH SESSION REPLACES AN ENDED ONE (a device loss recovered, or an eglTerminate /
     // eglInitialize): every client-side answer to "what does the server hold" describes the

MobileGL/MG_Remote/Server/StagedTextureStore.h

@@ -668,6 +668,29 @@ namespace MobileGL::MG_Remote::Server {
                     site, static_cast<Uint32>(uploadTarget), static_cast<Uint32>(level));
         }
 
+        // A box of a level for a replay into storage that is not this store's - a shared image,
+        // whose level was defined with no data and is written in pieces (an X server uploading a
+        // client's pixels into a window pixmap). The level image and its row length in texels when
+        // every row of the box is covered; null otherwise. Unlike RequireLevelBytes a partially
+        // covered level answers, and nothing is fatal: an uncovered box is the caller's to skip.
+        const Uint8* CoveredBoxBytes(Uint64 key, Uint16 uploadTarget, Uint16 level, Int x, Int y, Uint32 w, Uint32 h,
+                                     SizeT texel, Int* rowTexels) const {
+            if (!m_copies) return nullptr;
+            const std::lock_guard<std::mutex> lock(m_mutex);
+            const LevelShadow* shadow = FindLevel(key, uploadTarget, level);
+            if (shadow == nullptr || !shadow->Defined || x < 0 || y < 0 || w == 0 || h == 0 ||
+                x + static_cast<Int>(w) > shadow->Extent.x() || y + static_cast<Int>(h) > shadow->Extent.y())
+                return nullptr;
+            for (Uint32 row = 0; row < h; ++row) {
+                const SizeT begin = TexelOffset(shadow->Extent, x, y + static_cast<Int>(row), 0, texel);
+                const SizeT end = begin + static_cast<SizeT>(w) * texel;
+                if (end > shadow->Bytes.size() || !StagedShadowStore::CoverageHas(shadow->Covered, begin, end))
+                    return nullptr;
+            }
+            *rowTexels = shadow->Extent.x();
+            return shadow->Bytes.data();
+        }
+
         // Diagnostics the sync path and the unit cases read, so that a check can assert WHAT
         // HAPPENED rather than that nothing blew up.
         Bool IsCovered(Uint64 key, Uint16 uploadTarget, Uint16 level) const {