From 81604d5596b3d7f2e86da9ff009fac2a1f5b1b89 Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Wed, 5 Aug 2026 01:54:39 -0400 Subject: [PATCH] [Feat] (MG_Impl, MG_Test): validate the direct-state-access texture copies CopyTextureSubImage1D and 3D were do-nothing stubs and the 2D form checked only its effective target, so all 28 conditions in direct_state_access.textures_copy_errors went unreported: level and region bounds, and every read-framebuffer precondition. The read-framebuffer half lands in FramebufferImpl as ValidateReadFramebufferForCopy - incomplete read framebuffer (INVALID_FRAMEBUFFER_OPERATION), a read buffer that names no attachment, and a multisampled read buffer (both INVALID_OPERATION). It decides multisampledness by attachment kind rather than by sample count alone, because a TEXTURE_2D_MULTISAMPLE attachment sets SAMPLE_BUFFERS even when its sample count is one - which is exactly what the CTS attaches, and what a renderbuffer-only check would have missed. The texture half is ValidateCopyTextureSubImage, shared by all three forms; 1D and 3D also get the effective-target rule their form specifies. NOTE: the copy itself is still not implemented for 1D and 3D - CopyTexSubImage1D_State and CopyTexSubImage3D_State remain TODOs and no backend exposes anything but a 2D blit - so direct_state_access.textures_copy stays red. Only the errors are complete, which is what un-stubbing these two entry points buys; both carry a comment saying so. CopyTextureSubImage2DUsesNamedObjectAndRestoresBinding had been passing a storage-less texture and no read framebuffer, which the new validation correctly rejects. It now sets up a legal copy, so it still measures the by-name plumbing it was written for. Takes direct_state_access.textures_copy_errors from failing to passing on both backends. --- .../MG_Impl/GLImpl/Exporting/Definitions.cpp | 4 +- .../MG_Impl/GLImpl/Framebuffer/Validators.cpp | 44 +++++++++++++ .../MG_Impl/GLImpl/Framebuffer/Validators.h | 5 ++ .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 66 +++++++++++++++++++ MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.h | 3 + MobileGL/MG_Test/Texture/TextureTest.cpp | 13 ++++ 6 files changed, 133 insertions(+), 2 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp b/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp index 64fe0a37..a175509b 100644 --- a/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp +++ b/MobileGL/MG_Impl/GLImpl/Exporting/Definitions.cpp @@ -1062,9 +1062,9 @@ DECLARE_GL_FUNCTION_HEAD(void, TextureSubImage3D, GLuint texture, GLint level, G DECLARE_GL_FUNCTION_STUB_HEAD(void, CompressedTextureSubImage1D, GLuint texture, GLint level, GLint xoffset, GLsizei width, GLenum format, GLsizei imageSize, const void* data) DECLARE_GL_FUNCTION_STUB_END_NO_RETURN(void, CompressedTextureSubImage1D, texture, level, xoffset, width, format, imageSize, data) DECLARE_GL_FUNCTION_STUB_HEAD(void, CompressedTextureSubImage2D, GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLsizei width, GLsizei height, GLenum format, GLsizei imageSize, const void* data) DECLARE_GL_FUNCTION_STUB_END_NO_RETURN(void, CompressedTextureSubImage2D, texture, level, xoffset, yoffset, width, height, format, imageSize, data) DECLARE_GL_FUNCTION_STUB_HEAD(void, CompressedTextureSubImage3D, GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, GLenum format, GLsizei imageSize, const void* data) DECLARE_GL_FUNCTION_STUB_END_NO_RETURN(void, CompressedTextureSubImage3D, texture, level, xoffset, yoffset, zoffset, width, height, depth, format, imageSize, data) -DECLARE_GL_FUNCTION_STUB_HEAD(void, CopyTextureSubImage1D, GLuint texture, GLint level, GLint xoffset, GLint x, GLint y, GLsizei width) DECLARE_GL_FUNCTION_STUB_END_NO_RETURN(void, CopyTextureSubImage1D, texture, level, xoffset, x, y, width) +DECLARE_GL_FUNCTION_HEAD(void, CopyTextureSubImage1D, GLuint texture, GLint level, GLint xoffset, GLint x, GLint y, GLsizei width) DECLARE_GL_FUNCTION_END_NO_RETURN(void, CopyTextureSubImage1D, texture, level, xoffset, x, y, width) DECLARE_GL_FUNCTION_HEAD(void, CopyTextureSubImage2D, GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint x, GLint y, GLsizei width, GLsizei height) DECLARE_GL_FUNCTION_END_NO_RETURN(void, CopyTextureSubImage2D, texture, level, xoffset, yoffset, x, y, width, height) -DECLARE_GL_FUNCTION_STUB_HEAD(void, CopyTextureSubImage3D, GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, GLint y, GLsizei width, GLsizei height) DECLARE_GL_FUNCTION_STUB_END_NO_RETURN(void, CopyTextureSubImage3D, texture, level, xoffset, yoffset, zoffset, x, y, width, height) +DECLARE_GL_FUNCTION_HEAD(void, CopyTextureSubImage3D, GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, GLint y, GLsizei width, GLsizei height) DECLARE_GL_FUNCTION_END_NO_RETURN(void, CopyTextureSubImage3D, texture, level, xoffset, yoffset, zoffset, x, y, width, height) DECLARE_GL_FUNCTION_HEAD(void, TextureParameterf, GLuint texture, GLenum pname, GLfloat param) DECLARE_GL_FUNCTION_END_NO_RETURN(void, TextureParameterf, texture, pname, param) DECLARE_GL_FUNCTION_HEAD(void, TextureParameterfv, GLuint texture, GLenum pname, const GLfloat* param) DECLARE_GL_FUNCTION_END_NO_RETURN(void, TextureParameterfv, texture, pname, param) DECLARE_GL_FUNCTION_HEAD(void, TextureParameteri, GLuint texture, GLenum pname, GLint param) DECLARE_GL_FUNCTION_END_NO_RETURN(void, TextureParameteri, texture, pname, param) diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.cpp b/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.cpp index 9fdde7ab..84738b2a 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.cpp +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.cpp @@ -118,4 +118,48 @@ namespace MobileGL::MG_Impl::GLImpl::FramebufferImpl { std::format("Renderbuffer name {} is not valid.", index))); return false; } + + Bool ValidateReadFramebufferForCopy(const char* caller) { + auto& framebufferObject = + MG_State::pGLContext->GetFramebufferBindingSlot(FramebufferTarget::Read).GetBoundObject(); + if (!framebufferObject || !framebufferObject->CheckCompleteness()) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidFramebufferOperation, + MakeUnique("MG_Impl/GLImpl/FramebufferImpl", caller, + "Read framebuffer is not framebuffer complete.")); + return false; + } + + const FramebufferAttachmentType readBuffer = framebufferObject->GetReadBuffer(); + if (readBuffer == FramebufferAttachmentType::None || + !framebufferObject->GetAttachment(readBuffer).IsValid()) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl/FramebufferImpl", caller, + "Read buffer names no attachment of the read framebuffer.")); + return false; + } + + // SAMPLE_BUFFERS is one whenever the read buffer resolves to multisample storage. A + // multisample texture says so by its target - its sample count can legally be one - while a + // renderbuffer says so by having been given a non-zero sample count. + const auto& readAttachment = framebufferObject->GetAttachment(readBuffer); + Bool isMultisampled = false; + if (readAttachment.IsRenderbuffer() && readAttachment.GetRenderbuffer()) { + isMultisampled = readAttachment.GetRenderbuffer()->GetSamples() > 0; + } else if (readAttachment.IsTexture() && readAttachment.GetTexture()) { + const auto target = readAttachment.GetTexture()->GetTarget(); + isMultisampled = target == TextureTarget::Texture2DMultisample || + target == TextureTarget::Texture2DMultisampleArray; + } + if (isMultisampled) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl/FramebufferImpl", caller, + "Cannot copy from a multisampled read framebuffer.")); + return false; + } + + return true; + } } // namespace MobileGL::MG_Impl::GLImpl::FramebufferImpl diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.h b/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.h index a95b64ac..bfbd51b5 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.h +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/Validators.h @@ -20,4 +20,9 @@ namespace MobileGL::MG_Impl::GLImpl::FramebufferImpl { Bool ValidateColorAttachmentInRange(FramebufferAttachmentType attachment, const char* caller); Bool ValidateRenderbufferTarget(RenderbufferTarget target); Bool ValidateRenderbufferName(Uint index, Bool allowZero = true); + // The read-framebuffer preconditions the CopyTexSubImage family shares (GL 4.6 core 8.6): the + // read framebuffer must be complete, its read buffer must name a real attachment, and it must + // not be multisampled. Incompleteness is INVALID_FRAMEBUFFER_OPERATION, the other two are + // INVALID_OPERATION. + Bool ValidateReadFramebufferForCopy(const char* caller); } // namespace MobileGL::MG_Impl::GLImpl::FramebufferImpl diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 53ee12be..49982bf6 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -3020,6 +3021,26 @@ namespace MobileGL::MG_Impl::GLImpl { // TODO: implement } + // What the three CopyTextureSubImage forms check in common (GL 4.6 core 8.6), once the caller + // has rejected an effective target its own form does not accept: the destination region has to + // lie inside the level, and the read framebuffer has to be able to supply pixels at all. + Bool ValidateCopyTextureSubImage(const SharedPtr& textureObject, GLint level, + GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, + GLsizei depth, const char* caller) { + if (!TextureImpl::ValidateTextureLevelNumber(level)) return false; + if (width < 0 || height < 0 || depth < 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", caller, "Copy dimensions must be non-negative.")); + return false; + } + if (!TextureImpl::ValidateTextureSubImageOffsets(textureObject, xoffset, width, yoffset, height, zoffset, + depth)) { + return false; + } + return FramebufferImpl::ValidateReadFramebufferForCopy(caller); + } + void CopyTexSubImage2D_Backend(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint x, GLint y, GLsizei width, GLsizei height) { MG_Backend::gBackendFunctionsTable.GL.CopyTexSubImage2D(target, level, xoffset, yoffset, x, y, width, height); @@ -4706,11 +4727,56 @@ namespace MobileGL::MG_Impl::GLImpl { "rectangle texture.")); return; } + if (!ValidateCopyTextureSubImage(textureObject, level, xoffset, yoffset, 0, width, height, 1, __func__)) { + return; + } WithTemporarilyBoundNamedTexture(textureObject, [&](GLenum glTarget) { CopyTexSubImage2D_Backend(glTarget, level, xoffset, yoffset, x, y, width, height); }); } + void CopyTextureSubImage1D(GLuint texture, GLint level, GLint xoffset, GLint x, GLint y, GLsizei width) { + auto textureObject = GetTextureObjectByName(texture, __func__); + if (!textureObject) return; + if (textureObject->GetTarget() != TextureTarget::Texture1D) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", __func__, + "CopyTextureSubImage1D requires a 1D texture.")); + return; + } + if (!ValidateCopyTextureSubImage(textureObject, level, xoffset, 0, 0, width, 1, 1, __func__)) return; + // NOTE: the copy itself is still missing - CopyTexSubImage1D_State is a no-op and no backend + // exposes a 1D blit - so a valid call reaches the destination unchanged. Only the error + // reporting above is complete. + CopyTexSubImage1D_State(GL_TEXTURE_1D, level, xoffset, x, y, width); + } + + void CopyTextureSubImage3D(GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, + GLint y, GLsizei width, GLsizei height) { + auto textureObject = GetTextureObjectByName(texture, __func__); + if (!textureObject) return; + // GL 4.6 core 8.6: the 3D form takes the layered targets, a cube map included - the face + // is selected by zoffset. + const auto target = textureObject->GetTarget(); + if (target != TextureTarget::Texture3D && target != TextureTarget::Texture2DArray && + target != TextureTarget::TextureCubeMap && target != TextureTarget::TextureCubeMapArray) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique("MG_Impl/GLImpl", __func__, + "CopyTextureSubImage3D requires a 3D, 2D-array, cube map, or " + "cube map array texture.")); + return; + } + if (!ValidateCopyTextureSubImage(textureObject, level, xoffset, yoffset, zoffset, width, height, 1, __func__)) { + return; + } + // NOTE: as with the 1D form, CopyTexSubImage3D_State does not perform the copy yet. + WithTemporarilyBoundNamedTexture(textureObject, [&](GLenum glTarget) { + CopyTexSubImage3D_State(glTarget, level, xoffset, yoffset, zoffset, x, y, width, height); + }); + } + void CopyTexSubImage1D(GLenum target, GLint level, GLint xoffset, GLint x, GLint y, GLsizei width) { CopyTexSubImage1D_State(target, level, xoffset, x, y, width); } diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.h b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.h index 2aba8be6..49c9e217 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.h +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.h @@ -106,6 +106,9 @@ namespace MobileGL::MG_Impl::GLImpl { GLsizei width, GLsizei height); void CopyTexSubImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint x, GLint y, GLsizei width, GLsizei height); + void CopyTextureSubImage1D(GLuint texture, GLint level, GLint xoffset, GLint x, GLint y, GLsizei width); + void CopyTextureSubImage3D(GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLint x, + GLint y, GLsizei width, GLsizei height); void CopyTextureSubImage2D(GLuint texture, GLint level, GLint xoffset, GLint yoffset, GLint x, GLint y, GLsizei width, GLsizei height); void CopyTexSubImage1D(GLenum target, GLint level, GLint xoffset, GLint x, GLint y, GLsizei width); diff --git a/MobileGL/MG_Test/Texture/TextureTest.cpp b/MobileGL/MG_Test/Texture/TextureTest.cpp index 6b338502..93ff6982 100644 --- a/MobileGL/MG_Test/Texture/TextureTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureTest.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -296,6 +297,18 @@ TEST_F(TextureTest, CopyTextureSubImage2DUsesNamedObjectAndRestoresBinding) { MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D, 1, &boundTexture); MG_Impl::GLImpl::BindTextureUnit(0, boundTexture); + // The copy is only allowed to reach the backend when the destination region fits the level and + // the read framebuffer can supply pixels, so the call has to be set up as a legal one. + MG_Impl::GLImpl::TextureStorage2D(namedTexture, 3, GL_RGBA8, 16, 16); + + GLuint readFramebuffer = 0; + GLuint readTexture = 0; + MG_Impl::GLImpl::CreateFramebuffers(1, &readFramebuffer); + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D, 1, &readTexture); + MG_Impl::GLImpl::TextureStorage2D(readTexture, 1, GL_RGBA8, 16, 16); + MG_Impl::GLImpl::NamedFramebufferTexture(readFramebuffer, GL_COLOR_ATTACHMENT0, readTexture, 0); + MG_Impl::GLImpl::BindFramebuffer(GL_READ_FRAMEBUFFER, readFramebuffer); + const auto boundBefore = MG_State::pGLContext->GetTextureUnitObject(0) .GetBindingSlot(TextureTarget::Texture2D) .GetBoundObject();