From 7d68a17774cc32616e6b991ecb20e687ed6abf67 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sat, 22 Aug 2026 06:16:52 -0400 Subject: [PATCH] [Fix, Test] (DirectGLES): give a split buffer image its own view so the sampler still sees whole texels --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 19 ++- MobileGL/MG_Backend/DirectGLES/Managers.cpp | 53 ++++++- MobileGL/MG_Backend/DirectGLES/Managers.h | 23 +++ .../Scenarios/NonCoreImageFormatScenario.cpp | 135 ++++++++++++++++++ 4 files changed, 219 insertions(+), 11 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index ed802a5f..4b2ee6c5 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -1490,10 +1490,17 @@ namespace MobileGL::MG_Backend::DirectGLES { // Dim::Buffer guard there for the 32-byte GL_RG32F measurement that pinned it. // // What a buffer image takes instead is the SPLIT, which is the same three-layer move - // through a different door: the glTexBuffer view above and the bind below both name - // the single-channel base format, and the shader subscripts it two components per - // original texel. Same gate on both sides, so the two cannot disagree. + // through a different door: a private glTexBuffer view names the single-channel base + // format, the bind below names it too, and the shader subscripts it two components per + // original texel. Same gate on all three, so they cannot disagree. + // + // The split view is a SEPARATE texture name over the same buffer, and the bind has to + // name it rather than the application's own: the application's texture keeps the + // format it asked for so that a samplerBuffer reading the same buffer texture - which + // is NOT subscript-rewritten - still sees whole texels. See + // BackendTextureObject::m_bufferImageSplitViewId. GLenum bindFormat = imageBinding.Format; + GLuint bindTextureId = backendTexture->GetBackendTextureId(); if (imageBinding.Texture->GetTarget() == TextureTarget::TextureBuffer) { if (TextureImpl::GetImageBindableBufferSplitFormat(imageBinding.Texture->GetFormat()) != GL_UNKNOWN_MGL) { @@ -1501,6 +1508,10 @@ namespace MobileGL::MG_Backend::DirectGLES { MG_Util::ConvertGLEnumToTextureInternalFormat(imageBinding.Format)); boundFormatSplit != GL_UNKNOWN_MGL) { bindFormat = boundFormatSplit; + if (const Uint splitViewId = backendTexture->GetBufferImageSplitViewId(); + splitViewId != 0) { + bindTextureId = splitViewId; + } } } } else if (TextureImpl::GetImageBindableStorageWidening(imageBinding.Texture->GetFormat())) { @@ -1510,7 +1521,7 @@ namespace MobileGL::MG_Backend::DirectGLES { bindFormat = boundFormatWidening.InternalFormat; } } - g_GLESFuncs.glBindImageTexture(unit, backendTexture->GetBackendTextureId(), imageBinding.Level, + g_GLESFuncs.glBindImageTexture(unit, bindTextureId, imageBinding.Level, layered, layer, imageBinding.Access, bindFormat); } diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index cb579db3..f3cf9b05 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -2382,8 +2382,12 @@ namespace MobileGL::MG_Backend::DirectGLES { } if (m_contextGeneration == g_backendContextGeneration && g_GLESFuncs.glDeleteTextures) { g_GLESFuncs.glDeleteTextures(1, &m_backendTextureId); + if (m_bufferImageSplitViewId != 0) { + g_GLESFuncs.glDeleteTextures(1, &m_bufferImageSplitViewId); + } } m_backendTextureId = 0; + m_bufferImageSplitViewId = 0; } void BackendTextureObject::Bind(GLenum target, Uint unit) { @@ -3828,13 +3832,17 @@ namespace MobileGL::MG_Backend::DirectGLES { // and WidenImageFormatsPass rewrites every access to subscript it that way. Only // for a texture that is actually image-bound: a sampled-only buffer texture keeps // the format the application asked for (see GetImageBindableBufferSplitFormat). - if (m_imageBindableStorageRequired) { - if (const GLenum splitFormat = - TextureImpl::GetImageBindableBufferSplitFormat(textureBufferObject->GetFormat()); - splitFormat != GL_UNKNOWN_MGL) { - glInternalFormat = splitFormat; - } - } + // + // The split goes on a SEPARATE name (m_bufferImageSplitViewId), not on this one. + // Re-describing the application's own texture also re-describes what a + // samplerBuffer reading it sees, and the sampler side is not subscript-rewritten - + // so texelFetch(s, i) started returning component 2i of the base view instead of + // texel i. rg32f is a legal SAMPLED buffer-texture format in ES 3.2; only the + // IMAGE binding needs the split, so only the image binding's name carries it. + const GLenum bufferImageSplitFormat = + m_imageBindableStorageRequired + ? TextureImpl::GetImageBindableBufferSplitFormat(textureBufferObject->GetFormat()) + : GL_UNKNOWN_MGL; if (needsRegeneration) { // Desktop GL has had buffer textures core since 3.1 and MobileGL advertises a @@ -3890,6 +3898,37 @@ namespace MobileGL::MG_Backend::DirectGLES { func, file, line, MG_Util::ConvertGLEnumToString(glInternalFormat).c_str(), backendId, MG_Util::ConvertGLEnumToString(err).c_str()); }); + + // The image half of the SPLIT, on its own name over the same buffer. Minted + // lazily - only a texture that is both image-bound AND holds a format with no + // ESSL image spelling ever gets one - and re-pointed here, in the same + // regeneration gate as the view above, so the two never describe different + // buffers or different windows of one. + if (bufferImageSplitFormat != GL_UNKNOWN_MGL) { + if (m_bufferImageSplitViewId == 0) { + g_GLESFuncs.glGenTextures(1, &m_bufferImageSplitViewId); + } + if (m_bufferImageSplitViewId == 0) { + MGLOG_E_ONCE("Failed to generate the buffer-image split view for texture %u; " + "its image binding will read the unsplit view.", + stateTextureObject->GetExternalIndex()); + } else { + g_GLESFuncs.glBindTexture(GL_TEXTURE_BUFFER, m_bufferImageSplitViewId); + if (rangeOffset == 0 && rangeSize == buffer->GetSize()) { + CallTexBuffer(GL_TEXTURE_BUFFER, bufferImageSplitFormat, backendId); + } else if (!CallTexBufferRange(GL_TEXTURE_BUFFER, bufferImageSplitFormat, backendId, + static_cast(rangeOffset), + static_cast(rangeSize))) { + CallTexBuffer(GL_TEXTURE_BUFFER, bufferImageSplitFormat, backendId); + } + // The raw bind above went behind Bind()'s shadow, which tracks objects + // rather than names: leaving it claiming THIS object is bound would + // make the next Bind(GL_TEXTURE_BUFFER) a no-op and leave the split + // view bound in the application texture's place. + g_boundTexturesCache[g_activeTextureUnit][static_cast( + TextureTarget::TextureBuffer)] = nullptr; + } + } } break; } diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 6921c66d..a99609c0 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -808,6 +808,10 @@ namespace MobileGL::MG_Backend::DirectGLES { void Bind(GLenum target, Uint unit = TempTextureUnit); Uint GetBackendTextureId() const; + // The id to hand glBindImageTexture for a SPLIT buffer image, or 0 when this texture + // takes no split. See m_bufferImageSplitViewId. + Uint GetBufferImageSplitViewId() const { return m_bufferImageSplitViewId; } + // Aggregate first-level clean gate for the per-draw trio // SyncTextureParamsToBackend + SyncBuiltinSamplerToBackend + // SyncMipmapsToBackend: EXACTLY the conjunction of their own early-outs @@ -844,6 +848,25 @@ namespace MobileGL::MG_Backend::DirectGLES { void RecreateBackendTexture(); Uint m_backendTextureId = 0; + // A SECOND buffer-texture name over the SAME buffer object, viewed in the split's + // single-channel base format, used only as the glBindImageTexture target. + // + // The split needs the view to say r32f where the application said rg32f, but a buffer + // texture that is image-bound may ALSO be read through a samplerBuffer - and the + // sampler side is not subscript-rewritten, so re-describing the application's own + // texture broke it: texelFetch(s, i) returned component 2i of the base view instead of + // texel i's pair. That is exactly and only + // KHR-GL42/43.shader_image_load_store.advanced-sync-imageAccess, which image-stores + // into a GL_RG32F buffer texture and then reads the same texture through both an + // imageBuffer and a samplerBuffer in one shader, comparing the two. + // + // Two names over one buffer cost nothing and alias exactly: a buffer texture owns no + // storage, so both views are the application's bytes, and the split's whole premise is + // that the two describe the same memory. The application's own name therefore keeps + // the format it asked for - rg32f IS a legal SAMPLED buffer-texture format in ES 3.2, + // it is only the IMAGE binding ES cannot spell - and the private name below carries + // the split the shader was rewritten against. 0 when this texture takes no split. + Uint m_bufferImageSplitViewId = 0; // ES context generation the id was created under; a dtor running after // that context died must not delete a foreign (recycled) name. Uint m_contextGeneration = 0; diff --git a/MobileGL/MG_IntegrationTest/Scenarios/NonCoreImageFormatScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/NonCoreImageFormatScenario.cpp index 4a807fb6..77c461a3 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/NonCoreImageFormatScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/NonCoreImageFormatScenario.cpp @@ -1075,6 +1075,141 @@ void main() } } + // The SAME buffer texture read through BOTH doors at once, which is the shape the split + // originally broke. A buffer texture that is image-bound is split - the view is re-declared + // one component at a time and every image subscript is doubled to match - but the sampler + // side is NOT subscript-rewritten, so re-describing the APPLICATION's own texture made + // texelFetch(s, i) return component 2i of the base view instead of texel i's whole pair. + // The split therefore goes on a private second name over the same buffer + // (BackendTextureObject::m_bufferImageSplitViewId) and the application's name keeps the + // format it asked for: rg32f is a legal SAMPLED buffer-texture format in ES 3.2, it is only + // the IMAGE binding ES cannot spell. + // + // This is KHR-GL42/43.shader_image_load_store.advanced-sync-imageAccess reduced to one + // dispatch. That case image-stores into a GL_RG32F buffer texture and then, in one shader, + // reads the same texture through an imageBuffer AND a samplerBuffer and compares the two - + // so it went red on every pixel while its sibling -vertexArray, which never samples the + // buffer texture, passed. + // + // Like the case above this runs on every backend, and on a driver that can spell rg32f for + // an imageBuffer nothing is split at all - both doors then trivially agree, which is the + // other half of the claim: a split that fired where the driver needed none would show up + // here as the two disagreeing. + TEST_F(NonCoreImageFormatScenario, ASplitBufferImageStillSamplesWholeTexels) { + if (!Ready()) GTEST_SKIP() << "no GL context"; + if (!ImagesAreUsable()) GTEST_SKIP() << "no image load/store on this driver"; + GLint maxTextureBufferSize = 0; + glGetIntegerv(GL_MAX_TEXTURE_BUFFER_SIZE, &maxTextureBufferSize); + while (glGetError() != GL_NO_ERROR) { + } + if (maxTextureBufferSize <= 0) GTEST_SKIP() << "no buffer textures on this driver"; + + constexpr int kBufferTexels = 4; + // Both components of every texel distinct and non-zero, so a sampler that reads the + // SPLIT view cannot accidentally agree: texel i would come back as (2i-th component, + // 0, 0, 1) rather than (x, y, 0, 1), and every one of those is a value no texel holds. + std::vector seed(static_cast(kBufferTexels) * 2u, 0.0f); + for (int texel = 0; texel < kBufferTexels; ++texel) { + seed[static_cast(texel) * 2u + 0u] = static_cast(texel * 10 + 1); + seed[static_cast(texel) * 2u + 1u] = static_cast(texel * 10 + 2); + } + + GLuint buffer = 0; + glGenBuffers(1, &buffer); + glBindBuffer(GL_TEXTURE_BUFFER, buffer); + glBufferData(GL_TEXTURE_BUFFER, static_cast(seed.size() * sizeof(float)), seed.data(), + GL_DYNAMIC_DRAW); + GLuint texture = 0; + glGenTextures(1, &texture); + m_textures.push_back(texture); + glBindTexture(GL_TEXTURE_BUFFER, texture); + glTexBuffer(GL_TEXTURE_BUFFER, GL_RG32F, buffer); + if (const GLenum error = FirstGLError()) { + glDeleteBuffers(1, &buffer); + GTEST_SKIP() << "glTexBuffer(GL_RG32F) errored with " << GLErrorName(error); + } + + // The answer buffer is rgba32f, which IS core ESSL, so it is never split and cannot + // hide a mistake in the thing under test. + constexpr int kAnswers = kBufferTexels * 2; + const std::vector answerSeed(static_cast(kAnswers) * 4u, -12345.0f); + GLuint answerBuffer = 0; + glGenBuffers(1, &answerBuffer); + glBindBuffer(GL_TEXTURE_BUFFER, answerBuffer); + glBufferData(GL_TEXTURE_BUFFER, static_cast(answerSeed.size() * sizeof(float)), + answerSeed.data(), GL_DYNAMIC_DRAW); + GLuint answerTexture = 0; + glGenTextures(1, &answerTexture); + m_textures.push_back(answerTexture); + glBindTexture(GL_TEXTURE_BUFFER, answerTexture); + glTexBuffer(GL_TEXTURE_BUFFER, GL_RGBA32F, answerBuffer); + if (const GLenum error = FirstGLError()) { + glDeleteBuffers(1, &buffer); + glDeleteBuffers(1, &answerBuffer); + GTEST_SKIP() << "glTexBuffer(GL_RGBA32F) errored with " << GLErrorName(error); + } + + const GLuint program = MakeComputeProgram(R"(#version 430 core + +layout (local_size_x = 1, local_size_y = 1, local_size_z = 1) in; + +layout (rg32f, binding = 0) readonly uniform imageBuffer narrow; +layout (rgba32f, binding = 1) writeonly uniform imageBuffer answers; +uniform samplerBuffer sampled; + +void main() +{ + int texel = int(gl_GlobalInvocationID.x); + imageStore(answers, texel * 2 + 0, imageLoad(narrow, texel)); + imageStore(answers, texel * 2 + 1, texelFetch(sampled, texel)); +} +)"); + if (program == 0) { + glDeleteBuffers(1, &buffer); + glDeleteBuffers(1, &answerBuffer); + return; + } + + BindImage(kNarrowUnit, texture, GL_RG32F, GL_READ_ONLY); + BindImage(kWideUnit, answerTexture, GL_RGBA32F, GL_WRITE_ONLY); + glActiveTexture(GL_TEXTURE0); + glBindTexture(GL_TEXTURE_BUFFER, texture); + glUseProgram(program); + glUniform1i(glGetUniformLocation(program, "sampled"), 0); + glDispatchCompute(kBufferTexels, 1, 1); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + EXPECT_EQ(FirstGLError(), 0u) << "the dispatch leaked a GL error"; + glUseProgram(0); + + std::vector readback(answerSeed.size(), -54321.0f); + glBindBuffer(GL_TEXTURE_BUFFER, answerBuffer); + glGetBufferSubData(GL_TEXTURE_BUFFER, 0, + static_cast(readback.size() * sizeof(float)), readback.data()); + EXPECT_EQ(FirstGLError(), 0u) << "reading the answers back errored"; + + for (int texel = 0; texel < kBufferTexels; ++texel) { + const float red = static_cast(texel * 10 + 1); + const float green = static_cast(texel * 10 + 2); + const std::size_t viaImage = static_cast(texel) * 8u; + const std::size_t viaSampler = viaImage + 4u; + EXPECT_FLOAT_EQ(readback[viaImage + 0u], red) << "texel " << texel << " imageLoad red"; + EXPECT_FLOAT_EQ(readback[viaImage + 1u], green) << "texel " << texel << " imageLoad green"; + EXPECT_FLOAT_EQ(readback[viaSampler + 0u], red) << "texel " << texel << " texelFetch red"; + EXPECT_FLOAT_EQ(readback[viaSampler + 1u], green) + << "texel " << texel + << " texelFetch green: a samplerBuffer must see whole texels even where the " + "image side of the same texture was split"; + EXPECT_FLOAT_EQ(readback[viaSampler + 2u], 0.0f) << "texel " << texel << " texelFetch blue"; + EXPECT_FLOAT_EQ(readback[viaSampler + 3u], 1.0f) << "texel " << texel << " texelFetch alpha"; + } + + glBindBuffer(GL_TEXTURE_BUFFER, 0); + glDeleteBuffers(1, &buffer); + glDeleteBuffers(1, &answerBuffer); + while (glGetError() != GL_NO_ERROR) { + } + } + // The other consumer of the same texture. A widened texture's ES storage really does have // four channels, so a sampler reading it raw would see whatever the carrier holds; the // logical format's missing channels have to keep reading 0 and 1 (which Espryt arranges