From 7ccb762936bbe8d742c825396e24337b13d5da51 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 12 Aug 2026 09:24:57 -0400 Subject: [PATCH] [Fix, Test] (MG_Impl, MG_Test): a persistently mapped unpack buffer is a legal source for the compressed uploads too --- .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 89 ++++++++++--------- MobileGL/MG_Test/Texture/TextureTest.cpp | 48 ++++++++++ 2 files changed, 93 insertions(+), 44 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index c0262acd..8a0a1766 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -1376,6 +1376,47 @@ namespace MobileGL::MG_Impl::GLImpl { return true; } + // The same rules for the COMPRESSED entry points, whose payload size is the imageSize the + // caller passed rather than something derived from a (format, type) pair - and which have no + // datum size, so the alignment rule above does not apply to them. Shared by + // glCompressedTexImage2D and glCompressedTexSubImage2D so the two cannot drift; the point + // that is easy to get wrong and that KHR-GL44.buffer_storage.map_persistent_texture exists to + // check is the first one: a PERSISTENT mapping stays a legal transfer source. + Bool ValidateCompressedUnpackBufferSource(const void* data, SizeT imageSize, const char* caller) { + const auto& unpackBuffer = + MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::PixelUnpack).GetBoundObject(); + if (!unpackBuffer) return true; + + if (unpackBuffer->IsMapped() && !(unpackBuffer->GetMappingAccess() & BufferMappingAccessBit::Persistent)) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", caller, "Pixel unpack buffer is currently mapped.")); + return false; + } + + const SizeT offset = reinterpret_cast(data); + const SizeT bufferSize = unpackBuffer->GetSize(); + if (offset > bufferSize || imageSize > bufferSize - offset) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", caller, + "Unpacking would read past the end of the pixel unpack buffer.")); + return false; + } + return true; + } + + // Where a compressed upload reads its blocks from: `data` is an offset into the bound unpack + // buffer when there is one, and a client pointer otherwise. Only meaningful once + // ValidateCompressedUnpackBufferSource has passed. Null means there is nothing to read, which + // GL leaves undefined and which callers must not dereference. + const void* CompressedUnpackSource(const void* data) { + const auto& unpackBuffer = + MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::PixelUnpack).GetBoundObject(); + if (!unpackBuffer) return data; + return reinterpret_cast(unpackBuffer->MappedData()) + reinterpret_cast(data); + } + void TexSubImage3D_State(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, GLenum format, GLenum type, const void* pixels) { TextureUploadTarget textureUploadTarget = MG_Util::ConvertGLEnumToTextureUploadTarget(target); @@ -3587,28 +3628,8 @@ namespace MobileGL::MG_Impl::GLImpl { } // ======================= Processing ================================ - const void* compressedBytes = data; - const auto& pixelUnpackBufferObject = - MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::PixelUnpack).GetBoundObject(); - if (pixelUnpackBufferObject) { - if (pixelUnpackBufferObject->IsMapped()) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", __func__, - "Pixel unpack buffer is currently mapped.")); - return; - } - const SizeT offset = reinterpret_cast(data); - const SizeT bufferSize = pixelUnpackBufferObject->GetSize(); - if (offset > bufferSize || expectedImageSize > bufferSize - offset) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", __func__, - "Unpacking would read past the end of the pixel unpack buffer.")); - return; - } - compressedBytes = reinterpret_cast(pixelUnpackBufferObject->MappedData()) + offset; - } + if (!ValidateCompressedUnpackBufferSource(data, expectedImageSize, __func__)) return; + const void* compressedBytes = CompressedUnpackSource(data); if (expectedImageSize == 0) return; // a zero-sized region is a legal no-op if (compressedBytes == nullptr) { // No unpack buffer and a null client pointer: there is nothing to read. GL leaves @@ -3746,28 +3767,8 @@ namespace MobileGL::MG_Impl::GLImpl { // SetMipmapCompressedImage re-arms it. textureMipmapObject->AllocateStorage(textureUploadTarget, level, {{width, height, 1}, internalBytes}); - const void* compressedBytes = data; - const auto& pixelUnpackBufferObject = - MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::PixelUnpack).GetBoundObject(); - if (pixelUnpackBufferObject) { - if (pixelUnpackBufferObject->IsMapped()) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", __func__, - "Pixel unpack buffer is currently mapped.")); - return; - } - const SizeT offset = reinterpret_cast(data); - const SizeT bufferSize = pixelUnpackBufferObject->GetSize(); - if (offset > bufferSize || expectedImageSize > bufferSize - offset) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", __func__, - "Unpacking would read past the end of the pixel unpack buffer.")); - return; - } - compressedBytes = reinterpret_cast(pixelUnpackBufferObject->MappedData()) + offset; - } + if (!ValidateCompressedUnpackBufferSource(data, expectedImageSize, __func__)) return; + const void* compressedBytes = CompressedUnpackSource(data); textureMipmapObject->SetMipmapCompressedImage(textureUploadTarget, level, internalformat, compressedBytes, expectedImageSize); textureMipmapObject->MarkStorageDirty(textureUploadTarget, level, true); diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index 6ad2fed3..bf8ac74d 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureTest.cpp @@ -1732,6 +1732,54 @@ TEST_F(TextureTest, CompressedTexSubImage2DUnpacksFromAPixelUnpackBuffer) { (void)texture; } +// ARB_buffer_storage's whole point: a PERSISTENTLY mapped buffer stays usable while the map is +// live, including as the source of a texture upload - which is what +// KHR-GL44.buffer_storage.map_persistent_texture checks. An ordinary map still disqualifies it. +// Both compressed entry points share one validator, so both are checked here. +TEST_F(TextureTest, CompressedUploadsAcceptAPersistentlyMappedUnpackBuffer) { + Uint8 source[256]; + for (Int i = 0; i < 256; ++i) source[i] = static_cast(255 - i); + GLuint buffer = 0; + MG_Impl::GLImpl::GenBuffers(1, &buffer); + MG_Impl::GLImpl::BindBuffer(GL_PIXEL_UNPACK_BUFFER, buffer); + MG_Impl::GLImpl::BufferStorage(GL_PIXEL_UNPACK_BUFFER, sizeof(source), source, + GL_MAP_PERSISTENT_BIT | GL_MAP_READ_BIT | GL_MAP_WRITE_BIT); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + void* mapped = MG_Impl::GLImpl::MapBufferRange(GL_PIXEL_UNPACK_BUFFER, 0, sizeof(source), + GL_MAP_PERSISTENT_BIT | GL_MAP_READ_BIT | GL_MAP_WRITE_BIT); + ASSERT_NE(mapped, nullptr); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + 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); + 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))); + 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); + + MG_Impl::GLImpl::UnmapBuffer(GL_PIXEL_UNPACK_BUFFER); + + // The negative control: an ORDINARY map is still an error, so the check above is not just + // "the mapped test was dropped". + GLuint plainBuffer = 0; + MG_Impl::GLImpl::GenBuffers(1, &plainBuffer); + MG_Impl::GLImpl::BindBuffer(GL_PIXEL_UNPACK_BUFFER, plainBuffer); + MG_Impl::GLImpl::BufferData(GL_PIXEL_UNPACK_BUFFER, sizeof(source), source, GL_STATIC_DRAW); + ASSERT_NE(MG_Impl::GLImpl::MapBuffer(GL_PIXEL_UNPACK_BUFFER, GL_READ_ONLY), nullptr); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + MG_Impl::GLImpl::CompressedTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, 8, 8, GL_COMPRESSED_RED_RGTC1, kRgtc1Size8x8, + reinterpret_cast(static_cast(0))); + ExpectSingleGlError(GL_INVALID_OPERATION); + MG_Impl::GLImpl::UnmapBuffer(GL_PIXEL_UNPACK_BUFFER); + MG_Impl::GLImpl::BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); +} + // glCompressedTextureSubImage2D was an exported no-op that raised no error at all, so an // application could not tell the write had not happened. It must reach the NAMED texture and leave // the binding it borrowed exactly as it found it.