diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 3aee6709..e6aa40a1 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/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(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(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 diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index a03ec03b..bd7fc955 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/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 diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp index a9467f41..c579c612 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp +++ b/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 } diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 8a0a1766..490278e7 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/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 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(1, width >> level); @@ -4286,6 +4307,13 @@ namespace MobileGL::MG_Impl::GLImpl { static_cast(levelWidth) * static_cast(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(level), internalformat, nullptr, + MG_Util::CalculateCompressedTextureImageSize(compressedInfo, + {levelWidth, levelHeight, 1})); + } } // See TextureStorage1D. textureMipmapObject->TruncateMipmapLevels(uploadTarget, static_cast(levels)); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/VertexArrayEnableDisableScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/VertexArrayEnableDisableScenario.cpp index 5028e744..938944b9 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/VertexArrayEnableDisableScenario.cpp +++ b/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; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/XfbCaptureBufferReuseScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/XfbCaptureBufferReuseScenario.cpp index 318f755f..7d44c3fb 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/XfbCaptureBufferReuseScenario.cpp +++ b/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 #include #include #include @@ -157,7 +158,11 @@ void main() { for (std::size_t i = 0; i < kCaptureFloats; ++i) { const float expected = value + static_cast(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 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); diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp index 41d4a2a7..6e6aa649 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp +++ b/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) { diff --git a/MobileGL/MG_Test/Buffer/BufferTest.cpp b/MobileGL/MG_Test/Buffer/BufferTest.cpp index 04084a20..a5312565 100644 --- a/MobileGL/MG_Test/Buffer/BufferTest.cpp +++ b/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(bufferObject->MappedData()), + static_cast(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(bufferObject->MappedData()), + static_cast(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; +} diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index bf8ac74d..fc07caf3 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/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(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(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(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(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(0xF0 + i); + MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 1, 4, 4, 2, 4, GL_COMPRESSED_RGBA_BPTC_UNORM, + static_cast(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(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(static_cast(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(static_cast(0))); + reinterpret_cast(static_cast(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(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(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); +}