From afebf38e90cf514366688a3b7bf02adc9d0a06df Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 10:38:29 -0400 Subject: [PATCH] [Fix, Test] (GLImpl): require only the requested level to exist in glGetTexImage --- .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 69 ++++++++++++------- MobileGL/MG_Test/Texture/TextureTest.cpp | 50 ++++++++++++++ 2 files changed, 95 insertions(+), 24 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 3f6a021b..3ffea8c3 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -4200,8 +4200,14 @@ namespace MobileGL::MG_Impl::GLImpl { return false; } - // For a cube map this is exactly cube completeness: IsComplete() wants all six faces. - if (!textureObject->IsComplete()) { + // GL 4.6 core 8.11.4 names cube completeness as the only completeness a readback requires, + // and for a cube map that is exactly what IsComplete() answers (all six faces defined at + // every level). It must not speak for any other target: on a mip chain it also rejects + // "level N defined, the levels below it not", which is a perfectly readable texture at + // level N - and the shape glClearTexImage's conformance cases build, since they define + // only the level they clear. The requested level's own existence is checked below. + if ((target == TextureTarget::TextureCubeMap || target == TextureTarget::TextureCubeMapArray) && + !textureObject->IsComplete()) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, MakeUnique("MG_Impl/GLImpl", caller, "Texture is incomplete")); @@ -4260,33 +4266,48 @@ namespace MobileGL::MG_Impl::GLImpl { const auto* textureMipmapObject = static_cast(textureObject.get()); const auto& uploadTargets = textureObject->GetUploadTargets(); - if (!uploadTargets.empty() && static_cast(level) < textureMipmapObject->GetMipmapLevelCount()) { - // Tightly packed, and summed over every face because a cube map query returns all - // six. Pack pixel-store state only ever grows this, so a request rejected here - // could not have fit under any packing. - const auto texelSize = textureMipmapObject->GetMipmapTexelSize(uploadTargets[0], level); - const SizeT required = MG_Util::CalculateInputTextureImageSize(textureInputFormat, - texturePixelDataType, texelSize) * - uploadTargets.size(); + // The half of the completeness gate above that GL does keep: the REQUESTED level has + // to hold an image. A name that was never given one carries no levels at all (which is + // also what an Unknown internal format answers), and a chain grown to reach level N + // leaves every level below it at {0, 0, 0}. + if (uploadTargets.empty() || static_cast(level) >= textureMipmapObject->GetMipmapLevelCount()) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", caller, "Texture level has no image to read back.")); + return false; + } + const auto texelSize = textureMipmapObject->GetMipmapTexelSize(uploadTargets[0], level); + if (texelSize.x() <= 0 || texelSize.y() <= 0 || texelSize.z() <= 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", caller, "Texture level has no image to read back.")); + return false; + } - if (bufSize >= 0 && static_cast(bufSize) < required) { + // Tightly packed, and summed over every face because a cube map query returns all + // six. Pack pixel-store state only ever grows this, so a request rejected here + // could not have fit under any packing. + const SizeT required = MG_Util::CalculateInputTextureImageSize(textureInputFormat, + texturePixelDataType, texelSize) * + uploadTargets.size(); + + if (bufSize >= 0 && static_cast(bufSize) < required) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", caller, "Destination buffer is too small.")); + return false; + } + + if (pixelPackBufferObject) { + const SizeT bufferSize = pixelPackBufferObject->GetSize(); + const SizeT offset = reinterpret_cast(pixels); + if (offset > bufferSize || required > bufferSize - offset) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", caller, "Destination buffer is too small.")); + MakeUnique("MG_Impl/GLImpl", caller, + "Packing would write past the end of the pixel pack buffer.")); return false; } - - if (pixelPackBufferObject) { - const SizeT bufferSize = pixelPackBufferObject->GetSize(); - const SizeT offset = reinterpret_cast(pixels); - if (offset > bufferSize || required > bufferSize - offset) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", caller, - "Packing would write past the end of the pixel pack buffer.")); - return false; - } - } } } diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index 2182f8dc..de58330d 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureTest.cpp @@ -1317,6 +1317,56 @@ TEST_F(TextureTest, GetTextureImageReadsNamedObjectWithoutBinding) { EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); } +// GL 4.6 core 8.11.4 asks a readback for cube completeness and nothing else, so a mip chain whose +// levels BELOW the requested one were never defined is still readable at that level - which is +// exactly the shape ARB_clear_texture's conformance cases build (they define only the level they +// clear). The whole-chain completeness gate used to answer INVALID_OPERATION here. +TEST_F(TextureTest, GetTexImageReadsALevelWhoseLowerLevelsWereNeverDefined) { + GLuint texture = 0; + MG_Impl::GLImpl::GenTextures(1, &texture); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, texture); + + const Uint8 pixels[] = { + 61, 62, 63, 64, + 71, 72, 73, 74, + }; + MG_Impl::GLImpl::TexImage2D(GL_TEXTURE_2D, 2, GL_RGBA8, 2, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, pixels); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + Uint8 output[sizeof(pixels)] = {}; + MG_Impl::GLImpl::GetTexImage(GL_TEXTURE_2D, 2, GL_RGBA, GL_UNSIGNED_BYTE, output); + + EXPECT_EQ(std::memcmp(output, pixels, sizeof(pixels)), 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + +// The other half of the same rule: loosening the chain-wide check must not let a level that holds +// no image at all through. Level 0 exists as a chain slot once level 2 is defined, but nothing ever +// gave it an image, so it stays INVALID_OPERATION - as does a level past the end of the chain and a +// texture that was never given any image whatsoever. +TEST_F(TextureTest, GetTexImageStillRejectsALevelThatHoldsNoImage) { + GLuint texture = 0; + MG_Impl::GLImpl::GenTextures(1, &texture); + MG_Impl::GLImpl::BindTexture(GL_TEXTURE_2D, texture); + + Uint8 output[4] = {}; + + // No image at all yet: the chain carries no levels. + MG_Impl::GLImpl::GetTexImage(GL_TEXTURE_2D, 0, GL_RGBA, GL_UNSIGNED_BYTE, output); + ExpectSingleGlError(GL_INVALID_OPERATION); + + MG_Impl::GLImpl::TexImage2D(GL_TEXTURE_2D, 2, GL_RGBA8, 1, 1, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // Inside the chain, but never defined. + MG_Impl::GLImpl::GetTexImage(GL_TEXTURE_2D, 0, GL_RGBA, GL_UNSIGNED_BYTE, output); + ExpectSingleGlError(GL_INVALID_OPERATION); + + // Past the end of the chain. + MG_Impl::GLImpl::GetTexImage(GL_TEXTURE_2D, 3, GL_RGBA, GL_UNSIGNED_BYTE, output); + ExpectSingleGlError(GL_INVALID_OPERATION); +} + TEST_F(TextureTest, GetTextureSubImageReadsFullNamedLevelWithoutBinding) { GLuint texture = 0; GLuint boundTexture = 0;