[Fix, Test] (MG_State, MG_Backend, MG_Impl, MG_Test): review wave - hand the mapping back instead of renewing it, retire an immutable ES store on respecify, and tag glTexStorage2D levels too

MobileGL/MG_Backend/DirectGLES/Managers.cpp

@@ -358,8 +358,9 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 // Require working fences: recycling is gated on the frame-completion
                 // watermark, which only advances if Present can insert/poll fences.
                 return g_GLESFuncs.glFenceSync != nullptr && g_GLESFuncs.glGetSynciv != nullptr &&
-                       r.id != 0 && !r.persistentMapped && r.contextGeneration == g_bufferContextGeneration &&
-                       r.storageInitialized && r.storageSize > 0 && r.storageSize <= kMaxPoolableBufferBytes;
+                       r.id != 0 && !r.persistentMapped && !r.immutableStorage &&
+                       r.contextGeneration == g_bufferContextGeneration && r.storageInitialized &&
+                       r.storageSize > 0 && r.storageSize <= kMaxPoolableBufferBytes;
             }
 
             // Retire a buffer id into the pool (owning thread; caller verified IsPoolable).
@@ -555,6 +556,17 @@ namespace MobileGL::MG_Backend::DirectGLES {
                     resource = created.get();
                     bufferObject.SetBackendResource(std::move(created));
                 }
+                // Before the generation is stamped, not after: everything on the resource
+                // describes a context that is gone, and the idempotency check below would
+                // otherwise hand the caller the dead context's mapped pointer.
+                if (resource->contextGeneration != g_bufferContextGeneration) {
+                    resource->id = 0;
+                    resource->persistentMapped = false;
+                    resource->persistentPtr = nullptr;
+                    resource->immutableStorage = false;
+                    resource->storageInitialized = false;
+                    resource->storageSize = 0;
+                }
                 resource->contextGeneration = g_bufferContextGeneration;
 
                 if (resource->persistentMapped && resource->persistentPtr && resource->storageSize == size) {
@@ -564,9 +576,10 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 // Need a fresh id: glBufferStorage fails on a buffer that already has
                 // immutable storage, and any prior mutable store is replaced anyway.
                 if (resource->id != 0) {
-                    ScrubBufferBindingShadowsForId(resource->id);
+                    NoteBufferIdDeleted(resource->id);
                     g_GLESFuncs.glDeleteBuffers(1, &resource->id);
                     resource->id = 0;
+                    resource->immutableStorage = false;
                 }
                 g_GLESFuncs.glGenBuffers(1, &resource->id);
                 if (resource->id == 0) return nullptr;
@@ -578,6 +591,10 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 g_GLESFuncs.glBufferStorageEXT(TempBufferTarget, static_cast<GLsizeiptr>(size), initial,
                                                GL_MAP_WRITE_BIT | kMapPersistentBit | kMapCoherentBit |
                                                    kDynamicStorageBit);
+                // Set as soon as the store exists, not once the map succeeds: the failure
+                // path below leaves this id holding immutable storage, and whoever touches
+                // it next has to know that glBufferData cannot redefine it.
+                resource->immutableStorage = true;
                 void* ptr = g_GLESFuncs.glMapBufferRange(TempBufferTarget, 0, static_cast<GLsizeiptr>(size),
                                                          GL_MAP_WRITE_BIT | kMapPersistentBit | kMapCoherentBit);
                 if (!ptr) {
@@ -603,29 +620,36 @@ namespace MobileGL::MG_Backend::DirectGLES {
             void Ops_Respecify(BufferObject& bufferObject) {
                 auto* resource = ResourceOf(bufferObject);
                 if (!resource) return; // lazy: EnsureBufferResource full-uploads on creation
-                if (resource->persistentMapped) {
-                    // The frontend writes straight into the storage mapped here, and it
-                    // renewed that mapping for the redefined store before writing to it
-                    // (BufferObject::RedefineStorage), so the new contents already are
-                    // where a respecification would put them.
-                    if (bufferObject.IsBackendPersistentMapped()) return;
-                    // Renewal declined (a zero-sized store, or the map could not be
-                    // retaken): the buffer is back on its CPU shadow and needs an ordinary
-                    // store again. Not this id's, though - it carries IMMUTABLE storage
-                    // (glBufferStorageEXT), which glBufferData below would refuse. Drop it
-                    // and let the lazy EnsureBufferResource path mint a mutable one with a
-                    // full upload; nothing points into the old mapping any more, because
-                    // the frontend has just given the adoption back.
+                // The frontend hands an adopted mapping back before it redefines the store
+                // (BufferObject::RedefineStorage), so a resource that still carries the
+                // persistent state here describes the OLD store - and its storage is
+                // IMMUTABLE (glBufferStorageEXT), which the glBufferData below cannot
+                // respecify and which the driver would refuse in silence. Retire the id so
+                // EnsureBufferResource mints a mutable one, with a full upload from the
+                // shadow the frontend has just filled.
+                //
+                // Keyed on the STORAGE, not on persistentMapped: a glMapBufferRange that
+                // failed after its glBufferStorageEXT succeeded clears persistentMapped and
+                // still leaves an immutable store behind, and that one reached glBufferData.
+                if (resource->immutableStorage) {
                     resource->persistentMapped = false;
                     resource->persistentPtr = nullptr;
                     if (resource->id != 0 && CanTouchGLNow() &&
                         resource->contextGeneration == g_bufferContextGeneration) {
-                        ScrubBufferBindingShadowsForId(resource->id);
+                        NoteBufferIdDeleted(resource->id);
                         g_GLESFuncs.glDeleteBuffers(1, &resource->id);
+                        resource->id = 0;
+                        resource->immutableStorage = false;
                     }
-                    resource->id = 0;
+                    // Off the context thread the id cannot be deleted here, and dropping it
+                    // would leak an immutable, persistently mapped store. It stays put, and
+                    // stays flagged, until EnsureBufferResource retires it on the thread
+                    // that owns the context.
                     resource->storageInitialized = false;
                     resource->storageSize = 0;
+                    resource->pendingRespecify = true;
+                    resource->pendingRanges.clear();
+                    return;
                 }
                 if (!CanTouchGLNow() || resource->id == 0 ||
                     resource->contextGeneration != g_bufferContextGeneration) {
@@ -922,6 +946,21 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 // frontend re-acquires a fresh one on its next map.
                 resource->persistentMapped = false;
                 resource->persistentPtr = nullptr;
+                resource->immutableStorage = false;
+            }
+
+            // An immutable store nothing maps any more: a respecification of a buffer that
+            // had been persistently mapped, which Ops_Respecify could not retire because it
+            // ran off the context thread. glBufferData cannot redefine it, so it is retired
+            // here, on the thread that can, and the id is re-minted below.
+            if (resource->immutableStorage && !resource->persistentMapped && resource->id != 0) {
+                NoteBufferIdDeleted(resource->id);
+                g_GLESFuncs.glDeleteBuffers(1, &resource->id);
+                resource->id = 0;
+                resource->immutableStorage = false;
+                resource->storageInitialized = false;
+                resource->storageSize = 0;
+                resource->pendingRespecify = true;
             }
 
             // Zero-copy coherent persistent buffer: the app writes straight into the

MobileGL/MG_Backend/DirectGLES/Managers.h

@@ -286,6 +286,14 @@ namespace MobileGL::MG_Backend::DirectGLES {
             // context loss.
             Bool persistentMapped = false;
             void* persistentPtr = nullptr;
+            // The GL store behind `id` was created with glBufferStorageEXT and is
+            // therefore IMMUTABLE - glBufferData cannot respecify it and it must never be
+            // recycled through the size-keyed buffer pool. Tracked separately from
+            // persistentMapped because the two come apart: a glMapBufferRange that fails
+            // after its glBufferStorageEXT succeeded leaves immutable storage behind with
+            // no map, and a respecification then has to retire the id rather than hand it
+            // to glBufferData, which the driver would silently refuse.
+            Bool immutableStorage = false;
         };
 
         // Registered as the frontend's BufferBackendOps at backend init and on

MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp

@@ -379,23 +379,12 @@ namespace MobileGL::MG_Backend::DirectVulkan {
         BumpSliceEpoch(*resource);
         // Any cached streaming slice refers to the previous contents.
         resource->transientFrameSerial = 0;
-        if (resource->persistentMapped) {
-            if (bufferObject.IsBackendPersistentMapped()) {
-                // The frontend renewed its adoption of this storage for the redefined
-                // store before writing a byte of it (BufferObject::RedefineStorage), so
-                // the new contents are already HERE and there is no second copy to
-                // update. Swapping the storage is what must not happen: the mapping the
-                // frontend holds, and every read that resolves through it, would keep
-                // addressing the storage being released - which is how a transform
-                // feedback capture came to be written to one buffer and read back out
-                // of another.
-                return;
-            }
-            // The renewal did not happen (a zero-sized store, or the storage could not
-            // be created): the frontend is back on its CPU shadow, so this is an
-            // ordinary resident buffer again and the handling below applies.
-            resource->persistentMapped = false;
-        }
+        // Redefining the store hands any adopted mapping back to the CPU shadow
+        // (BufferObject::RedefineStorage), so a buffer that reaches here persistent-mapped
+        // is an ordinary resident one again: it needs the busy-tracking and conditional
+        // orphan below, and the next AcquirePersistentMap has to mint storage for the new
+        // store rather than hand back a mapping of the old one.
+        resource->persistentMapped = false;
         if (!resource->buffer.IsValid()) {
             return; // streaming-only resource: shadow + serial are enough
         }

MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp

@@ -3639,6 +3639,20 @@ namespace MobileGL::MG_Impl::GLImpl {
             return;
         }
 
+        // Once per process: the call is about to succeed, and what it does is narrower than what
+        // an application has every right to expect from it. Before this existed the call answered
+        // GL_INVALID_ENUM, which was wrong but at least visible; a silent success that leaves the
+        // sampled texels untouched is the kind of thing that costs a day to find from the other
+        // end. MGLOG_I, not _W: warnings are compiled out at the level everything ships at.
+        static std::atomic<Bool> announcedNoCodec{false};
+        if (!announcedNoCodec.exchange(true)) {
+            MGLOG_I("%s: the compressed blocks are stored verbatim and returned by "
+                    "glGetCompressedTexImage, but there is no BC/ETC decoder here, so they do not "
+                    "reach the texels this level SAMPLES as. Upload through glTexSubImage2D for "
+                    "that.",
+                    __func__);
+        }
+
         // The level's compressed image is stored as one blob, so the rectangle is patched into
         // a copy of it and the whole thing handed back. Compressed sub-image uploads are not a
         // hot path, and this keeps the storage layer's compressed API to the two calls it has.
@@ -4278,6 +4292,13 @@ namespace MobileGL::MG_Impl::GLImpl {
         // core 8.19). Allocating only the primary one left the object cube-incomplete, so every
         // framebuffer it was attached to reported GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT. Every other
         // 2D target has exactly one upload target, so this loop is a no-op change for them.
+        // A specific compressed internalformat commits every level it allocates to that
+        // format, the same way glTexImage2D does - and here it matters twice over, because
+        // immutable storage plus glCompressedTexSubImage2D IS the modern way to upload a
+        // compressed texture: without the tag that sub-image call finds an uncompressed
+        // level and refuses it. Zero width means a generic (implementation's choice)
+        // format, which MobileGL answers with uncompressed storage, so it is not tagged.
+        const auto compressedInfo = MG_Util::GetCompressedFormatInfo(internalformat);
         for (const auto uploadTarget : textureObject->GetUploadTargets()) {
             for (GLsizei level = 0; level < levels; ++level) {
                 const GLsizei levelWidth = std::max<GLsizei>(1, width >> level);
@@ -4286,6 +4307,13 @@ namespace MobileGL::MG_Impl::GLImpl {
                     static_cast<SizeT>(levelWidth) * static_cast<SizeT>(levelHeight) * bytesPerPixel;
                 textureMipmapObject->AllocateStorage(uploadTarget, level, {{levelWidth, levelHeight, 1}, byteSize});
                 textureMipmapObject->MarkStorageDirty(uploadTarget, level, false);
+                if (compressedInfo.blockWidth != 0) {
+                    // After AllocateStorage, which clears the tag.
+                    textureMipmapObject->SetMipmapCompressedImage(
+                        uploadTarget, static_cast<Uint>(level), internalformat, nullptr,
+                        MG_Util::CalculateCompressedTextureImageSize(compressedInfo,
+                                                                     {levelWidth, levelHeight, 1}));
+                }
             }
             // See TextureStorage1D.
             textureMipmapObject->TruncateMipmapLevels(uploadTarget, static_cast<Uint>(levels));

MobileGL/MG_IntegrationTest/Scenarios/VertexArrayEnableDisableScenario.cpp

@@ -77,8 +77,12 @@ namespace MGITest {
                 declarations += "in int a_" + std::to_string(i) + ";\n";
                 copies += "  sum += a_" + std::to_string(i) + ";\n";
             }
-            const std::string vertexSource = "#version 450\n\n" + declarations + "out int sum;\n\nvoid main()\n{\n" +
-                                             copies + "}\n";
+            // `flat` where the CTS case has none: an integral shader output cannot be
+            // interpolated, so a driver is within its rights to reject the unqualified form
+            // even with no matching fragment input. The capture reads the same value either
+            // way, and the qualifier keeps this scenario portable off llvmpipe.
+            const std::string vertexSource = "#version 450\n\n" + declarations +
+                                             "flat out int sum;\n\nvoid main()\n{\n" + copies + "}\n";
             const std::string fragmentSource = R"(#version 450
 
 out vec4 color;

MobileGL/MG_IntegrationTest/Scenarios/XfbCaptureBufferReuseScenario.cpp

@@ -27,6 +27,7 @@
 // rather than a parameter: it is the configuration that already worked, so it
 // has to keep working for the others to mean anything.
 
+#include <cmath>
 #include <cstdio>
 #include <cstring>
 #include <string>
@@ -157,7 +158,11 @@ void main() {
                 for (std::size_t i = 0; i < kCaptureFloats; ++i) {
                     const float expected = value + static_cast<float>(i);
                     const float got = data[i];
-                    if (got - expected > 0.01f || expected - got > 0.01f) {
+                    // isfinite first: every ordered comparison against a NaN is false, so a
+                    // pair of one-sided range tests REPORTS SUCCESS for uninitialised
+                    // storage that happens to read as NaN - which is exactly the failure
+                    // these scenarios exist to catch.
+                    if (!std::isfinite(got) || std::fabs(got - expected) > 0.01f) {
                         return ::testing::AssertionFailure()
                                << "component " << i << " is " << got << ", expected " << expected
                                << (got == kPoison ? " (the capture never reached these bytes)" : "");
@@ -281,7 +286,12 @@ void main() {
             GLuint xfbBuffer = 0;
             glGenBuffers(1, &xfbBuffer);
             glBindBuffer(GL_TRANSFORM_FEEDBACK_BUFFER, xfbBuffer);
-            glBufferStorage(GL_TRANSFORM_FEEDBACK_BUFFER, kCaptureBytes, nullptr, GL_MAP_READ_BIT);
+            // Poisoned at creation - the storage is immutable, so this is the only chance to
+            // put a recognisable value there, and without it a span that captured nothing
+            // would be indistinguishable from one that captured the right thing whenever the
+            // untouched bytes happened to read back as the expected number.
+            const std::vector<float> poison(kCaptureFloats, kPoison);
+            glBufferStorage(GL_TRANSFORM_FEEDBACK_BUFFER, kCaptureBytes, poison.data(), GL_MAP_READ_BIT);
             ASSERT_EQ(glGetError(), GL_NO_ERROR) << "glBufferStorage on the capture buffer";
             glBindBufferBase(GL_TRANSFORM_FEEDBACK_BUFFER, 0, xfbBuffer);
 

MobileGL/MG_State/GLState/BufferState/BufferObject.cpp

@@ -76,33 +76,36 @@ namespace MobileGL::MG_State::GLState {
     }
 
     // A (re)definition of the store is about to write `size` bytes through Bytes().
-    // Sizing the shadow is all that takes for a shadow-backed buffer, but a buffer
-    // whose bytes were adopted into backend GPU memory needs the adoption renewed
-    // first: the mapping it holds describes exactly the OLD store. Writing the new
+    // Sizing the shadow is all that takes for a shadow-backed buffer. A buffer whose
+    // bytes were adopted into backend GPU memory has to give the adoption back first,
+    // because the mapping it holds describes exactly the OLD store: writing the new
     // contents through it runs past its end the moment the store grows, and a backend
-    // that replaces the storage for the new store (which is what an orphaning
-    // respecification asks for) would leave that mapping - and therefore every later
-    // read of this buffer - addressing storage nothing writes to any more.
+    // that replaces the storage for the new store - which is what an orphaning
+    // respecification asks for - would leave that mapping, and therefore every later
+    // read of this buffer, addressing storage nothing writes to any more. That was the
+    // transform feedback capture that wrote one buffer while the readback read another.
     //
-    // Renewing rather than simply dropping is what keeps the common case free: a
-    // redefinition at the same size gets the same mapping back without any storage
-    // being created, which is also what makes the backend's respecify a no-op (the
-    // new bytes are already in the storage it would otherwise upload to).
+    // Given back rather than renewed here, deliberately. Renewing in place would mean
+    // memcpying the new contents into storage that submitted-but-unretired draws may
+    // still be reading, which is precisely what the orphaning idiom exists to avoid;
+    // avoiding THAT would mean either stalling on a fence in the middle of a frame or
+    // teaching the persistent-map op to orphan, and the op must never orphan for the
+    // other kind of caller (an application-held GL_MAP_PERSISTENT_BIT mapping, whose
+    // pointer has to stay valid for the buffer's whole life). Handing the store back to
+    // the CPU shadow needs none of that: the backend's ordinary respecification path
+    // then does the busy-tracking and the conditional orphan it has always done, and the
+    // next binding that wants GPU residency takes a fresh mapping of the new store.
     void BufferObject::RedefineStorage(SizeT size) {
-        const Bool wasGpuResident = m_resource.IsGpuResident();
-        if (wasGpuResident) {
-            // A capture or a shader write may still be running against the very bytes
-            // that are about to be overwritten.
-            SyncGpuWrites();
+        if (m_resource.IsGpuResident()) {
             m_resource.ReleasePersistentMap();
+            // Whatever a shader or a capture wrote is in the store being replaced, so
+            // there is nothing left to reconcile - and leaving the flag set would make
+            // the next read of this buffer wait for GPU work on behalf of bytes the
+            // application has just thrown away.
+            m_gpuWritePending = false;
         }
         m_size = size;
         m_resource.ResizeShadow(size);
-        if (wasGpuResident) {
-            // Declining is allowed (a zero-sized store, a backend without the op): the
-            // buffer simply goes back to the CPU-shadow model it had before adoption.
-            EnsureGpuResidentStorage();
-        }
     }
 
     void BufferObject::Respecify(SizeT size, const void* data) {

MobileGL/MG_Test/Buffer/BufferTest.cpp

@@ -1610,3 +1610,188 @@ TEST_F(GeneralBufferTest, General_CoherentAsFlush_PersistentMapAdoptsZeroCopyBac
     EXPECT_EQ(GetError(), GL_NO_ERROR);
     g_zeroCopyMock = nullptr;
 }
+
+// A buffer whose bytes the backend adopted into its own GPU memory - which is what
+// EnsureGpuResidentStorage does for a transform-feedback capture target or a shader
+// storage binding, so that MapBuffer/GetBufferSubData read real GPU results - and which
+// the application then REDEFINES.
+//
+// The store the adopted mapping describes is the one being thrown away. Keeping that
+// mapping across the redefinition is what let a transform feedback capture be written to
+// one buffer and read back out of another: the backend replaced the storage (a
+// respecification is the orphaning point) while the frontend went on resolving every read
+// through a mapping of the storage it had just released. Two capture spans into one
+// re-specified buffer came back empty from the second one onwards.
+//
+// So the mapping is handed back and the buffer returns to the CPU-shadow model until
+// something asks for residency again. These pin all three parts of that: the adoption
+// really is dropped, the new contents really do land where later reads resolve, and the
+// backend really is told to respecify - it must not skip the storage, or its copy would
+// keep the old bytes.
+TEST_F(BufferTest, RedefiningAnAdoptedBufferHandsTheMappingBack) {
+    ZeroCopyMockBackend mock;
+    g_zeroCopyMock = &mock;
+    ScopedBackendOps scopedOps(&kZeroCopyMockOps);
+
+    GLuint buffer = 0;
+    GenBuffers(1, &buffer);
+    BindBuffer(GL_ARRAY_BUFFER, buffer);
+    const GLint before[4] = {1, 2, 3, 4};
+    BufferData(GL_ARRAY_BUFFER, sizeof(before), before, GL_DYNAMIC_DRAW);
+    ASSERT_EQ(GetError(), GL_NO_ERROR);
+
+    auto bufferObject = MG_State::pGLContext->GetBufferObject(buffer);
+    ASSERT_NE(bufferObject, nullptr);
+    // The backend adopts the bytes, exactly as a capture target or an SSBO binding does.
+    ASSERT_TRUE(bufferObject->EnsureGpuResidentStorage());
+    ASSERT_TRUE(bufferObject->IsBackendPersistentMapped());
+    ASSERT_EQ(static_cast<const void*>(bufferObject->MappedData()),
+              static_cast<const void*>(mock.gpu.data()));
+    mock.respecifyCalls = 0;
+
+    const GLint after[4] = {10, 20, 30, 40};
+    BufferData(GL_ARRAY_BUFFER, sizeof(after), after, GL_DYNAMIC_DRAW);
+    ASSERT_EQ(GetError(), GL_NO_ERROR);
+
+    EXPECT_FALSE(bufferObject->IsBackendPersistentMapped());
+    EXPECT_NE(static_cast<const void*>(bufferObject->MappedData()),
+              static_cast<const void*>(mock.gpu.data()));
+    EXPECT_EQ(std::memcmp(bufferObject->MappedData(), after, sizeof(after)), 0);
+    // The backend has a separate copy again, so it must have been told to refresh it.
+    EXPECT_EQ(mock.respecifyCalls, 1);
+
+    g_zeroCopyMock = nullptr;
+}
+
+// The same redefinition at a LARGER size, which is the case nothing could paper over: the
+// adopted mapping is exactly as big as the old store, so writing the new contents through
+// it ran past the end of the backend allocation.
+TEST_F(BufferTest, RedefiningAnAdoptedBufferAtANewSizeStaysInBounds) {
+    ZeroCopyMockBackend mock;
+    g_zeroCopyMock = &mock;
+    ScopedBackendOps scopedOps(&kZeroCopyMockOps);
+
+    GLuint buffer = 0;
+    GenBuffers(1, &buffer);
+    BindBuffer(GL_ARRAY_BUFFER, buffer);
+    const GLint small[2] = {1, 2};
+    BufferData(GL_ARRAY_BUFFER, sizeof(small), small, GL_DYNAMIC_DRAW);
+    auto bufferObject = MG_State::pGLContext->GetBufferObject(buffer);
+    ASSERT_NE(bufferObject, nullptr);
+    ASSERT_TRUE(bufferObject->EnsureGpuResidentStorage());
+    ASSERT_EQ(mock.gpu.size(), sizeof(small));
+
+    const GLint large[8] = {1, 2, 3, 4, 5, 6, 7, 8};
+    BufferData(GL_ARRAY_BUFFER, sizeof(large), large, GL_DYNAMIC_DRAW);
+    ASSERT_EQ(GetError(), GL_NO_ERROR);
+
+    EXPECT_EQ(bufferObject->GetSize(), sizeof(large));
+    EXPECT_FALSE(bufferObject->IsBackendPersistentMapped());
+    EXPECT_EQ(std::memcmp(bufferObject->MappedData(), large, sizeof(large)), 0);
+    // The old, smaller GPU block was not written through: still the old size, still the
+    // old bytes.
+    EXPECT_EQ(mock.gpu.size(), sizeof(small));
+    EXPECT_EQ(std::memcmp(mock.gpu.data(), small, sizeof(small)), 0);
+
+    // And residency can be taken again, now over the new store.
+    ASSERT_TRUE(bufferObject->EnsureGpuResidentStorage());
+    EXPECT_TRUE(bufferObject->IsBackendPersistentMapped());
+    EXPECT_EQ(mock.gpu.size(), sizeof(large));
+    EXPECT_EQ(std::memcmp(bufferObject->MappedData(), large, sizeof(large)), 0);
+
+    g_zeroCopyMock = nullptr;
+}
+
+// glBufferStorage is the other way into a redefinition, and an adopted buffer can reach
+// it: the adoption came from a binding rather than from an application map, so the buffer
+// is still mutable and glBufferStorage is still legal on it.
+TEST_F(BufferTest, ImmutableStorageOnAnAdoptedBufferHandsTheMappingBackToo) {
+    ZeroCopyMockBackend mock;
+    g_zeroCopyMock = &mock;
+    ScopedBackendOps scopedOps(&kZeroCopyMockOps);
+
+    GLuint buffer = 0;
+    GenBuffers(1, &buffer);
+    BindBuffer(GL_ARRAY_BUFFER, buffer);
+    const GLint before[4] = {1, 2, 3, 4};
+    BufferData(GL_ARRAY_BUFFER, sizeof(before), before, GL_DYNAMIC_DRAW);
+    auto bufferObject = MG_State::pGLContext->GetBufferObject(buffer);
+    ASSERT_NE(bufferObject, nullptr);
+    ASSERT_TRUE(bufferObject->EnsureGpuResidentStorage());
+    ASSERT_TRUE(bufferObject->IsBackendPersistentMapped());
+
+    const GLint after[6] = {9, 8, 7, 6, 5, 4};
+    BufferStorage(GL_ARRAY_BUFFER, sizeof(after), after, GL_MAP_READ_BIT);
+    ASSERT_EQ(GetError(), GL_NO_ERROR);
+
+    EXPECT_TRUE(bufferObject->IsImmutableStorage());
+    EXPECT_FALSE(bufferObject->IsBackendPersistentMapped());
+    EXPECT_EQ(bufferObject->GetSize(), sizeof(after));
+    EXPECT_EQ(std::memcmp(bufferObject->MappedData(), after, sizeof(after)), 0);
+
+    g_zeroCopyMock = nullptr;
+}
+
+// A redefinition to nothing. The backend declines residency for an empty store, so this
+// is also the path where the mapping is given back and never retaken.
+TEST_F(BufferTest, RedefiningAnAdoptedBufferToZeroBytesLeavesItOnTheShadow) {
+    ZeroCopyMockBackend mock;
+    g_zeroCopyMock = &mock;
+    ScopedBackendOps scopedOps(&kZeroCopyMockOps);
+
+    GLuint buffer = 0;
+    GenBuffers(1, &buffer);
+    BindBuffer(GL_ARRAY_BUFFER, buffer);
+    const GLint before[4] = {1, 2, 3, 4};
+    BufferData(GL_ARRAY_BUFFER, sizeof(before), before, GL_DYNAMIC_DRAW);
+    auto bufferObject = MG_State::pGLContext->GetBufferObject(buffer);
+    ASSERT_NE(bufferObject, nullptr);
+    ASSERT_TRUE(bufferObject->EnsureGpuResidentStorage());
+    ASSERT_TRUE(bufferObject->IsBackendPersistentMapped());
+
+    BufferData(GL_ARRAY_BUFFER, 0, nullptr, GL_DYNAMIC_DRAW);
+    ASSERT_EQ(GetError(), GL_NO_ERROR);
+
+    EXPECT_EQ(bufferObject->GetSize(), 0u);
+    EXPECT_FALSE(bufferObject->IsBackendPersistentMapped());
+    EXPECT_FALSE(bufferObject->EnsureGpuResidentStorage()); // nothing to make resident
+
+    // ...and it comes back to life on the next non-empty store.
+    const GLint again[3] = {5, 6, 7};
+    BufferData(GL_ARRAY_BUFFER, sizeof(again), again, GL_DYNAMIC_DRAW);
+    ASSERT_EQ(GetError(), GL_NO_ERROR);
+    EXPECT_TRUE(bufferObject->EnsureGpuResidentStorage());
+    EXPECT_EQ(std::memcmp(bufferObject->MappedData(), again, sizeof(again)), 0);
+
+    g_zeroCopyMock = nullptr;
+}
+
+// The negative control for the four above: a backend that DECLINES to hand out a mapping
+// leaves the buffer shadow-backed throughout, so a redefinition is just a redefinition -
+// no adoption to give back, and the backend still gets its Respecify.
+TEST_F(BufferTest, RedefiningANonAdoptedBufferIsUnchanged) {
+    ZeroCopyMockBackend mock;
+    mock.provideMap = false;
+    g_zeroCopyMock = &mock;
+    ScopedBackendOps scopedOps(&kZeroCopyMockOps);
+
+    GLuint buffer = 0;
+    GenBuffers(1, &buffer);
+    BindBuffer(GL_ARRAY_BUFFER, buffer);
+    const GLint before[4] = {1, 2, 3, 4};
+    BufferData(GL_ARRAY_BUFFER, sizeof(before), before, GL_DYNAMIC_DRAW);
+    auto bufferObject = MG_State::pGLContext->GetBufferObject(buffer);
+    ASSERT_NE(bufferObject, nullptr);
+    EXPECT_FALSE(bufferObject->EnsureGpuResidentStorage());
+    EXPECT_FALSE(bufferObject->IsBackendPersistentMapped());
+    mock.respecifyCalls = 0;
+
+    const GLint after[4] = {10, 20, 30, 40};
+    BufferData(GL_ARRAY_BUFFER, sizeof(after), after, GL_DYNAMIC_DRAW);
+    ASSERT_EQ(GetError(), GL_NO_ERROR);
+    EXPECT_FALSE(bufferObject->IsBackendPersistentMapped());
+    EXPECT_EQ(std::memcmp(bufferObject->MappedData(), after, sizeof(after)), 0);
+    EXPECT_EQ(mock.respecifyCalls, 1);
+
+    g_zeroCopyMock = nullptr;
+}

MobileGL/MG_Test/Texture/TextureTest.cpp

@@ -1702,6 +1702,94 @@ TEST_F(TextureTest, CompressedTexSubImage2DReplacesTheStoredBlocks) {
     EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
 }
 
+// The Y axis of the placement, which the whole-image and single-column cases above cannot see: an
+// implementation that dropped the first-block-row term, or that divided yoffset by the block WIDTH,
+// passes every one of them. The region here starts at block row 1, so its two blocks belong at
+// bytes 16 and 24 and nowhere else.
+TEST_F(TextureTest, CompressedTexSubImage2DPlacesTheFirstBlockRow) {
+    const GLuint texture = MakeCompressedRgtc1Texture8x8();
+    Uint8 whole[kRgtc1Size8x8];
+    for (Int i = 0; i < kRgtc1Size8x8; ++i) whole[i] = static_cast<Uint8>(i + 1);
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 8, 8, GL_COMPRESSED_RED_RGTC1, kRgtc1Size8x8,
+                                             whole);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    // The bottom block row only: 8 texels wide, 4 high, starting at y = 4.
+    const Uint8 bottom[16] = {0xC0, 0xC1, 0xC2, 0xC3, 0xC4, 0xC5, 0xC6, 0xC7,
+                              0xD0, 0xD1, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6, 0xD7};
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 0, 0, 4, 8, 4, GL_COMPRESSED_RED_RGTC1,
+                                             static_cast<GLsizei>(sizeof(bottom)), bottom);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    Uint8 expected[kRgtc1Size8x8];
+    std::memcpy(expected, whole, sizeof(expected));
+    std::memcpy(expected + 16, bottom, sizeof(bottom));
+
+    Uint8 stored[kRgtc1Size8x8] = {};
+    MG_Impl::GLImpl::GetCompressedTexImage(GL_TEXTURE_2D, 0, stored);
+    EXPECT_EQ(std::memcmp(stored, expected, sizeof(expected)), 0);
+
+    // And one block in the far corner, which needs both terms at once.
+    const Uint8 corner[8] = {0xE0, 0xE1, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7};
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 0, 4, 4, 4, 4, GL_COMPRESSED_RED_RGTC1,
+                                             static_cast<GLsizei>(sizeof(corner)), corner);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+    std::memcpy(expected + 24, corner, sizeof(corner));
+    std::memset(stored, 0, sizeof(stored));
+    MG_Impl::GLImpl::GetCompressedTexImage(GL_TEXTURE_2D, 0, stored);
+    EXPECT_EQ(std::memcmp(stored, expected, sizeof(expected)), 0);
+    EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+    (void)texture;
+}
+
+// A level whose size is neither square nor a multiple of the block size, at a level above the
+// base, in a format with SIXTEEN bytes per block. Between them these pin the row stride (which a
+// square level cannot distinguish from the column count), the rounding-up of a partial edge block,
+// the run-to-the-edge exemption from the whole-blocks rule, and the block size actually coming from
+// the format rather than from a constant.
+TEST_F(TextureTest, CompressedTexSubImage2DHandlesPartialBlocksAndAMipLevel) {
+    GLuint texture = 0;
+    MG_Impl::GLImpl::GenTextures(1, &texture);
+    MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, texture);
+    // 6x10 BPTC: 2 block columns x 3 block rows of 16 bytes = 96, one block row = 32.
+    constexpr GLsizei kBptcSize6x10 = 96;
+    MG_Impl::GLImpl::CompressedTexImage2D(GL_TEXTURE_2D, 1, GL_COMPRESSED_RGBA_BPTC_UNORM, 6, 10, 0, kBptcSize6x10,
+                                          nullptr);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    GLint imageSize = 0;
+    MG_Impl::GLImpl::GetTexLevelParameteriv(GL_TEXTURE_2D, 1, GL_TEXTURE_COMPRESSED_IMAGE_SIZE, &imageSize);
+    EXPECT_EQ(imageSize, kBptcSize6x10);
+
+    Uint8 whole[kBptcSize6x10];
+    for (Int i = 0; i < kBptcSize6x10; ++i) whole[i] = static_cast<Uint8>(i + 1);
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 1, 0, 0, 6, 10, GL_COMPRESSED_RGBA_BPTC_UNORM,
+                                             kBptcSize6x10, whole);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    // The right-hand column (2 texels wide - a partial block that runs to the edge) of the middle
+    // block row: one block, at byte 32 + 16.
+    Uint8 patch[16];
+    for (Int i = 0; i < 16; ++i) patch[i] = static_cast<Uint8>(0xF0 + i);
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 1, 4, 4, 2, 4, GL_COMPRESSED_RGBA_BPTC_UNORM,
+                                             static_cast<GLsizei>(sizeof(patch)), patch);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    Uint8 expected[kBptcSize6x10];
+    std::memcpy(expected, whole, sizeof(expected));
+    std::memcpy(expected + 48, patch, sizeof(patch));
+
+    Uint8 stored[kBptcSize6x10] = {};
+    MG_Impl::GLImpl::GetCompressedTexImage(GL_TEXTURE_2D, 1, stored);
+    EXPECT_EQ(std::memcmp(stored, expected, sizeof(expected)), 0);
+
+    // The partial edge block is only exempt from the whole-blocks rule AT the edge: the same
+    // 2-texel width one block to the left is not.
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 1, 0, 4, 2, 4, GL_COMPRESSED_RGBA_BPTC_UNORM,
+                                             static_cast<GLsizei>(sizeof(patch)), patch);
+    ExpectSingleGlError(GL_INVALID_OPERATION);
+}
+
 // The same call sourcing its blocks from a buffer bound to GL_PIXEL_UNPACK_BUFFER, where `data` is
 // an offset into that buffer rather than a client pointer - which is the form
 // KHR-GL44.buffer_storage.map_persistent_texture uses for every one of its operations.
@@ -1753,15 +1841,19 @@ TEST_F(TextureTest, CompressedUploadsAcceptAPersistentlyMappedUnpackBuffer) {
     GLuint texture = 0;
     MG_Impl::GLImpl::GenTextures(1, &texture);
     MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, texture);
-    MG_Impl::GLImpl::CompressedTexImage2D(GL_TEXTURE_2D, 0, GL_COMPRESSED_RED_RGTC1, 8, 8, 0, kRgtc1Size8x8, nullptr);
+    // The image call takes offset 0 and the sub-image call offset 128, so the readback can only
+    // match if the SUB-IMAGE call ran: were it refused (or a no-op), the level would still hold
+    // the image call's bytes.
+    MG_Impl::GLImpl::CompressedTexImage2D(GL_TEXTURE_2D, 0, GL_COMPRESSED_RED_RGTC1, 8, 8, 0, kRgtc1Size8x8,
+                                          reinterpret_cast<const void*>(static_cast<SizeT>(0)));
     EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR) << "glCompressedTexImage2D over a persistent map";
     MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 8, 8, GL_COMPRESSED_RED_RGTC1, kRgtc1Size8x8,
-                                             reinterpret_cast<const void*>(static_cast<SizeT>(0)));
+                                             reinterpret_cast<const void*>(static_cast<SizeT>(128)));
     EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR) << "glCompressedTexSubImage2D over a persistent map";
 
     Uint8 stored[kRgtc1Size8x8] = {};
     MG_Impl::GLImpl::GetCompressedTexImage(GL_TEXTURE_2D, 0, stored);
-    EXPECT_EQ(std::memcmp(stored, source, sizeof(stored)), 0);
+    EXPECT_EQ(std::memcmp(stored, source + 128, sizeof(stored)), 0);
 
     MG_Impl::GLImpl::UnmapBuffer(GL_PIXEL_UNPACK_BUFFER);
 
@@ -3569,3 +3661,57 @@ TEST_F(TextureTest, GetTexLevelParameterOnBufferStorageReportsErrorInsteadOfTerm
         ExpectSingleGlError(GL_INVALID_OPERATION);
     }
 }
+
+// Immutable storage plus glCompressedTexSubImage2D is the modern way to upload a compressed
+// texture, so glTexStorage2D has to commit its levels to a specific compressed internalformat
+// exactly as glTexImage2D does. When it did not, the sub-image call found an uncompressed level
+// and refused it, and glTexImage2D and glTexStorage2D disagreed about the same token.
+TEST_F(TextureTest, TexStorage2DTagsEveryLevelForASpecificCompressedFormat) {
+    GLuint texture = 0;
+    MG_Impl::GLImpl::GenTextures(1, &texture);
+    MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, texture);
+    MG_Impl::GLImpl::TexStorage2D(GL_TEXTURE_2D, 2, GL_COMPRESSED_RED_RGTC1, 8, 8);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    for (GLint level = 0; level < 2; ++level) {
+        GLint compressed = GL_FALSE;
+        MG_Impl::GLImpl::GetTexLevelParameteriv(GL_TEXTURE_2D, level, GL_TEXTURE_COMPRESSED, &compressed);
+        EXPECT_EQ(compressed, GL_TRUE) << "level " << level;
+        GLint internalFormat = 0;
+        MG_Impl::GLImpl::GetTexLevelParameteriv(GL_TEXTURE_2D, level, GL_TEXTURE_INTERNAL_FORMAT, &internalFormat);
+        EXPECT_EQ(internalFormat, static_cast<GLint>(GL_COMPRESSED_RED_RGTC1)) << "level " << level;
+        GLint imageSize = 0;
+        MG_Impl::GLImpl::GetTexLevelParameteriv(GL_TEXTURE_2D, level, GL_TEXTURE_COMPRESSED_IMAGE_SIZE, &imageSize);
+        // 8x8 -> 2x2 blocks -> 32 bytes; 4x4 -> 1 block -> 8 bytes.
+        EXPECT_EQ(imageSize, level == 0 ? 32 : 8) << "level " << level;
+    }
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    // ...and the sub-image call the whole arrangement exists for now reaches both levels.
+    Uint8 blocks[kRgtc1Size8x8];
+    for (Int i = 0; i < kRgtc1Size8x8; ++i) blocks[i] = static_cast<Uint8>(0x40 + i);
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 8, 8, GL_COMPRESSED_RED_RGTC1, kRgtc1Size8x8,
+                                             blocks);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+    Uint8 stored[kRgtc1Size8x8] = {};
+    MG_Impl::GLImpl::GetCompressedTexImage(GL_TEXTURE_2D, 0, stored);
+    EXPECT_EQ(std::memcmp(stored, blocks, sizeof(stored)), 0);
+
+    MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 1, 0, 0, 4, 4, GL_COMPRESSED_RED_RGTC1, 8, blocks);
+    EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+}
+
+// The negative control: a generic compressed token leaves glTexStorage2D's levels uncompressed,
+// because for those the implementation's choice IS the answer and MobileGL chooses uncompressed.
+TEST_F(TextureTest, TexStorage2DLeavesAGenericCompressedFormatUncompressed) {
+    GLuint texture = 0;
+    MG_Impl::GLImpl::GenTextures(1, &texture);
+    MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, texture);
+    MG_Impl::GLImpl::TexStorage2D(GL_TEXTURE_2D, 1, GL_COMPRESSED_RED, 8, 8);
+    ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+
+    GLint compressed = GL_TRUE;
+    MG_Impl::GLImpl::GetTexLevelParameteriv(GL_TEXTURE_2D, 0, GL_TEXTURE_COMPRESSED, &compressed);
+    EXPECT_EQ(compressed, GL_FALSE);
+    EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR);
+}