diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp index 8bd69f80..fa712790 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp @@ -492,16 +492,29 @@ namespace MobileGL::MG_Impl::GLImpl { const FramebufferAttachmentType attachmentType = MG_Util::ConvertGLEnumToFramebufferAttachmentType(attachment); const FramebufferTarget framebufferTarget = MG_Util::ConvertGLEnumToFramebufferTarget(target); if (!FramebufferImpl::ValidateFramebufferAttachmentType(attachmentType)) return; + // GL 4.6 core 9.2.8: COLOR_ATTACHMENTm with m >= MAX_COLOR_ATTACHMENTS is an + // INVALID_OPERATION. The DSA sibling (NamedFramebufferTexture_State) has always asked + // this; the bound-target family did not, so an attachment one past the limit was + // accepted and then silently ignored. + if (!FramebufferImpl::ValidateColorAttachmentInRange(attachmentType, functionName)) return; if (!FramebufferImpl::ValidateFramebufferTarget(framebufferTarget)) return; if (!TextureImpl::ValidateTextureName(texture, true)) return; auto& bindingSlot = MG_State::pGLContext->GetFramebufferBindingSlot(framebufferTarget); auto& framebufferObject = bindingSlot.GetBoundObject(); - if (!framebufferObject) { + // GL 4.6 core 9.2.8: an INVALID_OPERATION error is generated if ZERO is bound to + // target. MobileGL keeps a real FramebufferObject for framebuffer 0, so the null test + // this replaced could never fire - the object is always there. Framebuffer 0 has to be + // recognised by identity instead, the same comparison DrawBuffers_State makes. + const auto& defaultFramebufferInfo = FramebufferImpl::pDefaultFramebufferInfo; + if (!framebufferObject || + (defaultFramebufferInfo && framebufferObject == defaultFramebufferInfo->defaultFBO)) { MG_State::pGLContext->RecordError( ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", functionName, - "Framebuffer target is bound to no framebuffer object.")); + MakeUnique( + "MG_Impl/GLImpl", functionName, + "No framebuffer object is bound to the target; the default framebuffer's attachments " + "cannot be named.")); return; } @@ -510,6 +523,18 @@ namespace MobileGL::MG_Impl::GLImpl { return; } + // GL 4.6 core 9.2.8: a negative level is INVALID_VALUE, and so is a level a texture + // with immutable storage does not have. Asked here rather than in each entry point so + // the whole glFramebufferTexture* family answers the same way, which is what the DSA + // sibling already did for the negative half. + if (level < 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", functionName, + "Texture level must be non-negative.")); + return; + } + auto& textureObject = MG_State::pGLContext->GetTextureObject(texture); if (!textureObject) { MG_State::pGLContext->RecordError( @@ -519,6 +544,17 @@ namespace MobileGL::MG_Impl::GLImpl { return; } + if (textureObject->IsImmutable() && + level >= static_cast(textureObject->GetImmutableLevels())) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique( + "MG_Impl/GLImpl", functionName, + std::format("Texture level {} is beyond the {} level(s) immutable texture {} has.", level, + textureObject->GetImmutableLevels(), texture))); + return; + } + const auto expectedTextureTarget = MG_Util::ConvertTextureUploadTargetToTextureTarget(textureUploadTarget); if (expectedTextureTarget == TextureTarget::Unknown || textureObject->GetTarget() != expectedTextureTarget) { @@ -1241,6 +1277,12 @@ namespace MobileGL::MG_Impl::GLImpl { return; } + // The name's validity is an INVALID_VALUE condition (GL 4.6 core 9.2.8), and it has to be + // asked BEFORE the object is resolved: reporting the miss as the INVALID_OPERATION below + // pre-empted the shared helper's ValidateTextureName and answered the wrong error code for + // every texture name that was never generated. + if (!TextureImpl::ValidateTextureName(texture, true)) return; + auto& textureObject = MG_State::pGLContext->GetTextureObject(texture); if (!textureObject) { MG_State::pGLContext->RecordError( diff --git a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp index 1d68cb26..1e4caf5d 100644 --- a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp +++ b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp @@ -1252,3 +1252,91 @@ TEST_F(FramebufferTest, ApplicationAlphaMaskOffIsStillHonouredOnANativeDrawBuffe EXPECT_EQ(g_driverIndexedColorMasks[2].a, GL_TRUE) << "a native buffer keeps its alpha writes"; EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); } + +// --- glFramebufferTexture error conditions (GL 4.6 core 9.2.8) --------------------------------- +// +// Four of them were missing from the bound-target path while its DSA sibling +// (glNamedFramebufferTexture) implemented all four, which is what KHR-GL4x.geometry_shader. +// layered_fbo.fb_texture_* fails on. Two of them - the attachment-range check and the +// default-framebuffer rejection - newly REFUSE calls that used to succeed, so they are pinned +// here rather than left to the conformance suite. + +TEST_F(FramebufferTest, FramebufferTextureRejectsTheDefaultFramebuffer) { + GLuint texture = 0; + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D, 1, &texture); + MG_Impl::GLImpl::TextureStorage2D(texture, 1, GL_RGBA8, 64, 32); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // MobileGL models framebuffer 0 as a real FramebufferObject, so the null test that used to + // stand in for this could never fire and the attach silently "succeeded". + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + DrainPendingGlErrors(); +} + +TEST_F(FramebufferTest, FramebufferTextureRejectsAColourAttachmentPastTheLimit) { + GLuint framebuffer = 0; + GLuint texture = 0; + MG_Impl::GLImpl::CreateFramebuffers(1, &framebuffer); + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D, 1, &texture); + MG_Impl::GLImpl::TextureStorage2D(texture, 1, GL_RGBA8, 64, 32); + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, framebuffer); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // The same limit ValidateColorAttachmentInRange reads, so the test cannot disagree with the + // implementation about where the boundary is. + const GLint limit = MG_Backend::pActiveBackendObject + ? static_cast( + MG_Backend::pActiveBackendObject->GetDynamicParameters().MaxColorAttachments) + : static_cast(MG_State::GLState::FramebufferObject::MAX_DRAW_BUFFERS); + ASSERT_GT(limit, 0); + ASSERT_LT(limit, 32) << "the test needs a colour attachment enum past the limit to exist"; + + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, + static_cast(GL_COLOR_ATTACHMENT0 + limit), texture, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + DrainPendingGlErrors(); + + // The last legal one still attaches, so the boundary is off-by-none. + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, + static_cast(GL_COLOR_ATTACHMENT0 + limit - 1), texture, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + +TEST_F(FramebufferTest, FramebufferTextureReportsInvalidValueForANameThatWasNeverGenerated) { + GLuint framebuffer = 0; + MG_Impl::GLImpl::CreateFramebuffers(1, &framebuffer); + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, framebuffer); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // INVALID_VALUE, not INVALID_OPERATION: the entry point used to resolve the texture object + // first and report the miss with the wrong code, pre-empting ValidateTextureName. + const GLuint neverGenerated = std::numeric_limits::max(); + ASSERT_FALSE(MG_State::pGLContext->ValidateTextureName(neverGenerated)); + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, neverGenerated, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); + DrainPendingGlErrors(); +} + +TEST_F(FramebufferTest, FramebufferTextureRejectsALevelTheTextureDoesNotHave) { + GLuint framebuffer = 0; + GLuint texture = 0; + MG_Impl::GLImpl::CreateFramebuffers(1, &framebuffer); + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D, 1, &texture); + // Two levels of immutable storage: level 1 is legal, level 2 is not. + MG_Impl::GLImpl::TextureStorage2D(texture, 2, GL_RGBA8, 64, 32); + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, framebuffer); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture, 1); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR) << "the last level the texture has is legal"; + + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture, 2); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture, -1); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); + DrainPendingGlErrors(); +}