diff --git a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp index fa712790..1623d4bc 100644 --- a/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp +++ b/MobileGL/MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.cpp @@ -474,6 +474,75 @@ namespace MobileGL::MG_Impl::GLImpl { } } + // GL 4.6 core 9.2.8 conditions that depend only on the framebuffer and the attachment + // point. Shared, because glFramebufferTexture / 1D / 2D / 3D / TextureLayer are aliases of + // one another in that section and a CTS case that walks the family must not get five + // different answers - which is exactly what happened when these lived in one helper that + // only two of the five went through. + Bool ValidateFramebufferTextureAttachmentPoint(const char* functionName, + const SharedPtr& + framebufferObject, + FramebufferAttachmentType attachmentType) { + // "An INVALID_OPERATION error is generated if COLOR_ATTACHMENTm is used with m greater + // than or equal to MAX_COLOR_ATTACHMENTS." + if (!FramebufferImpl::ValidateColorAttachmentInRange(attachmentType, functionName)) return false; + // "An INVALID_OPERATION error is generated if zero is bound to target." MobileGL keeps + // a real FramebufferObject for framebuffer 0, so a null test can never see this - the + // object is always there, and framebuffer 0 has to be recognised by identity instead, + // the same comparison DrawBuffers_State makes. Without this an attach onto the default + // framebuffer silently REPLACED its colour attachment, permanently desynchronising it + // from what the swapchain keeps publishing. + const auto& defaultFramebufferInfo = FramebufferImpl::pDefaultFramebufferInfo; + if (!framebufferObject || + (defaultFramebufferInfo && framebufferObject == defaultFramebufferInfo->defaultFBO)) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidOperation, + MakeUnique( + "MG_Impl/GLImpl", functionName, + "No framebuffer object is bound to the target; the default framebuffer's attachments " + "cannot be named.")); + return false; + } + return true; + } + + // The other half of 9.2.8: "level must be greater than or equal to zero", and for a + // texture with immutable storage it "must be smaller than the number of levels the texture + // has". Split from the attachment-point half because the caller only has a texture object + // once the detach (texture == 0) case is behind it. + Bool ValidateFramebufferTextureLevel(const char* functionName, + const SharedPtr& textureObject, + GLint level) { + if (level < 0) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique("MG_Impl/GLImpl", functionName, + "Texture level must be non-negative.")); + return false; + } + if (!textureObject || !textureObject->IsImmutable()) { + // A mutable texture has no level bound here: a level it has not specified yet is + // not an error, it just leaves the framebuffer incomplete. + return true; + } + // GetAddressableLevelCount(), NOT GetImmutableLevels(): for a VIEW the latter is + // deliberately the ORIGINAL texture's count (GL 4.6 core 8.18 defines + // TEXTURE_IMMUTABLE_LEVELS on a view that way), which is far too large a bound - a + // two-level view onto a ten-level texture would accept level 5 and attach an image + // nothing can draw into. + const Uint levelBound = textureObject->GetAddressableLevelCount(); + if (static_cast(level) >= levelBound) { + MG_State::pGLContext->RecordError( + ErrorCode::InvalidValue, + MakeUnique( + "MG_Impl/GLImpl", functionName, + std::format("Texture level {} is beyond the {} level(s) this texture has.", level, + levelBound))); + return false; + } + return true; + } + void AttachFramebufferTextureWithUploadTarget(const char* functionName, GLenum target, GLenum attachment, GLuint texture, GLint level, TextureUploadTarget textureUploadTarget, Bool layered = false) { @@ -492,49 +561,18 @@ 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(); - // 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, - "No framebuffer object is bound to the target; the default framebuffer's attachments " - "cannot be named.")); - return; - } + if (!ValidateFramebufferTextureAttachmentPoint(functionName, framebufferObject, attachmentType)) return; if (texture == 0) { framebufferObject->Detach(attachmentType); 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( @@ -543,17 +581,7 @@ namespace MobileGL::MG_Impl::GLImpl { std::format("Texture object {} is not valid.", texture))); 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; - } + if (!ValidateFramebufferTextureLevel(functionName, textureObject, level)) return; const auto expectedTextureTarget = MG_Util::ConvertTextureUploadTargetToTextureTarget(textureUploadTarget); if (expectedTextureTarget == TextureTarget::Unknown || @@ -1084,13 +1112,7 @@ namespace MobileGL::MG_Impl::GLImpl { auto& bindingSlot = MG_State::pGLContext->GetFramebufferBindingSlot(framebufferTarget); auto& framebufferObject = bindingSlot.GetBoundObject(); - if (!framebufferObject) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidOperation, - MakeUnique("MG_Impl/GLImpl", functionName, - "Framebuffer target is bound to no framebuffer object.")); - return; - } + if (!ValidateFramebufferTextureAttachmentPoint(functionName, framebufferObject, attachmentType)) return; if (texture == 0) { framebufferObject->Detach(attachmentType); @@ -1105,6 +1127,7 @@ namespace MobileGL::MG_Impl::GLImpl { std::format("Texture object {} is not valid.", texture))); return; } + if (!ValidateFramebufferTextureLevel(functionName, textureObject, level)) return; if (layer < 0) { MG_State::pGLContext->RecordError( ErrorCode::InvalidValue, @@ -1227,6 +1250,13 @@ namespace MobileGL::MG_Impl::GLImpl { "Framebuffer target is bound to no framebuffer object.")); return; } + // glFramebufferTexture2D is by far the most-used member of the family and the only one + // that inlines its own logic instead of going through the shared helper, so the 9.2.8 + // conditions have to be asked here explicitly. + if (!ValidateFramebufferTextureAttachmentPoint("FramebufferTexture2D_State", framebufferObject, + attachmentType)) { + return; + } if (texture == 0) { framebufferObject->Detach(attachmentType); @@ -1241,6 +1271,7 @@ namespace MobileGL::MG_Impl::GLImpl { std::format("Texture object {} is not valid.", texture))); return; } + if (!ValidateFramebufferTextureLevel("FramebufferTexture2D_State", textureObject, level)) return; const auto expectedTextureTarget = MG_Util::ConvertTextureUploadTargetToTextureTarget(textureUploadTarget); if (expectedTextureTarget == TextureTarget::Unknown || @@ -1333,13 +1364,10 @@ namespace MobileGL::MG_Impl::GLImpl { std::format("Texture object {} is not valid.", texture))); return; } - if (level < 0) { - MG_State::pGLContext->RecordError( - ErrorCode::InvalidValue, - MakeUnique("MG_Impl/GLImpl", "NamedFramebufferTexture_State", - "Texture level must be non-negative.")); - return; - } + // The whole level condition, not just its negative half: glNamedFramebufferTexture and + // glFramebufferTexture are equivalent in 9.2.8, so an out-of-range immutable level has to + // be rejected on both or a CTS case gets two answers for one rule. + if (!ValidateFramebufferTextureLevel("NamedFramebufferTexture_State", textureObject, level)) return; TextureUploadTarget textureUploadTarget = TextureUploadTarget::Unknown; Bool layered = false; diff --git a/MobileGL/MG_State/GLState/TextureState/TextureObject.h b/MobileGL/MG_State/GLState/TextureState/TextureObject.h index 5954c682..ddae84a5 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureObject.h +++ b/MobileGL/MG_State/GLState/TextureState/TextureObject.h @@ -49,6 +49,12 @@ namespace MobileGL::MG_State::GLState { virtual void SetMaxLevel(Uint maxLevel) = 0; virtual Bool IsImmutable() const = 0; virtual Uint GetImmutableLevels() const = 0; + // How many levels THIS object can address, i.e. the bound a level argument has to + // stay under. The same number as GetImmutableLevels() for an ordinary immutable + // texture, but NOT for a view: GL 4.6 core 8.18 defines TEXTURE_IMMUTABLE_LEVELS on a + // view as the ORIGINAL texture's value, which says nothing about what the view itself + // can reach, and bounding by it lets a level the view does not have through. + virtual Uint GetAddressableLevelCount() const = 0; virtual void SetImmutableLevels(Uint levels) = 0; virtual Uint16 GetTextureParamsVersion() const = 0; // Monotonic counter bumped on every CPU-side pixel mutation (see MarkStorageDirty). @@ -132,6 +138,10 @@ namespace MobileGL::MG_State::GLState { void SetMaxLevel(Uint maxLevel) override; Bool IsImmutable() const override; Uint GetImmutableLevels() const override; + // m_immutableLevels is already the VIEW-relative count for a view (its constructor + // stores there so the level-range clamp works in view coordinates), so + // this one accessor is correct for both and needs no override. + Uint GetAddressableLevelCount() const override { return m_immutableLevels; } void SetImmutableLevels(Uint levels) override; Uint16 GetTextureParamsVersion() const override; Uint64 GetContentVersion() const override; diff --git a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp index 1e4caf5d..9ddf20e8 100644 --- a/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp +++ b/MobileGL/MG_Test/Framebuffer/FramebufferTest.cpp @@ -1340,3 +1340,106 @@ TEST_F(FramebufferTest, FramebufferTextureRejectsALevelTheTextureDoesNotHave) { EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); DrainPendingGlErrors(); } + +// The four conditions above are stated once in GL 4.6 core 9.2.8 for the WHOLE family, and +// glFramebufferTexture2D / 3D / TextureLayer reach the attachment through their own code rather +// than through the shared helper - so each of them has to be asked separately or one entry point +// answers differently from its aliases. glFramebufferTexture2D is the most-used of the five, and +// the default-framebuffer case is the damaging one: the attach used to succeed and replace +// framebuffer 0's colour attachment, which nothing ever puts back. + +TEST_F(FramebufferTest, FramebufferTexture2DRejectsTheDefaultFramebufferAndBadAttachments) { + GLuint texture = 0; + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D, 1, &texture); + MG_Impl::GLImpl::TextureStorage2D(texture, 2, GL_RGBA8, 64, 32); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + const auto defaultFramebuffer = MG_State::pGLContext->GetFramebufferObject(0); + ASSERT_NE(defaultFramebuffer, nullptr); + const auto& colorBefore = defaultFramebuffer->GetAttachment(FramebufferAttachmentType::Color0); + const Bool hadTextureBefore = colorBefore.IsTexture(); + + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); + MG_Impl::GLImpl::FramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + DrainPendingGlErrors(); + // ...and, more to the point, the default framebuffer still describes the surface. + const auto& colorAfter = defaultFramebuffer->GetAttachment(FramebufferAttachmentType::Color0); + EXPECT_EQ(colorAfter.IsTexture(), hadTextureBefore); + if (colorAfter.IsTexture() && hadTextureBefore) { + EXPECT_NE(colorAfter.GetTexture()->GetExternalIndex(), texture) + << "the refused attach must not have replaced framebuffer 0's colour attachment"; + } + + 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); + + const GLint limit = MG_Backend::pActiveBackendObject + ? static_cast( + MG_Backend::pActiveBackendObject->GetDynamicParameters().MaxColorAttachments) + : static_cast(MG_State::GLState::FramebufferObject::MAX_DRAW_BUFFERS); + MG_Impl::GLImpl::FramebufferTexture2D(GL_DRAW_FRAMEBUFFER, + static_cast(GL_COLOR_ATTACHMENT0 + limit), GL_TEXTURE_2D, + texture, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::FramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 2); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE) << "the texture has two levels, not three"; + DrainPendingGlErrors(); + + MG_Impl::GLImpl::FramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, -1); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); + DrainPendingGlErrors(); + + // The legal call still works, so the boundary is off-by-none. + MG_Impl::GLImpl::FramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 1); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + +TEST_F(FramebufferTest, FramebufferTextureLayerRejectsTheDefaultFramebufferAndBadLevels) { + GLuint texture = 0; + MG_Impl::GLImpl::CreateTextures(GL_TEXTURE_2D_ARRAY, 1, &texture); + MG_Impl::GLImpl::TextureStorage3D(texture, 2, GL_RGBA8, 16, 16, 4); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + // The attach path used to bypass every one of these while the DETACH path (texture == 0) went + // through the fixed helper, so one entry point answered two different ways. + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); + MG_Impl::GLImpl::FramebufferTextureLayer(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture, 0, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_OPERATION); + DrainPendingGlErrors(); + + 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); + + MG_Impl::GLImpl::FramebufferTextureLayer(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture, 2, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); + DrainPendingGlErrors(); + + MG_Impl::GLImpl::FramebufferTextureLayer(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, texture, 1, 0); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); +} + +// The DSA sibling is the entry point the bound-target family was aligned WITH, so an out-of-range +// immutable level has to be rejected there too - otherwise the alignment created a fresh +// asymmetry in the opposite direction. +TEST_F(FramebufferTest, NamedFramebufferTextureRejectsALevelTheTextureDoesNotHave) { + 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, 2, GL_RGBA8, 64, 32); + ASSERT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + MG_Impl::GLImpl::NamedFramebufferTexture(framebuffer, GL_COLOR_ATTACHMENT0, texture, 1); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + MG_Impl::GLImpl::NamedFramebufferTexture(framebuffer, GL_COLOR_ATTACHMENT0, texture, 2); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_INVALID_VALUE); + DrainPendingGlErrors(); +} diff --git a/MobileGL/MG_Test/Texture/TextureViewTest.cpp b/MobileGL/MG_Test/Texture/TextureViewTest.cpp index ee42e927..f3085249 100644 --- a/MobileGL/MG_Test/Texture/TextureViewTest.cpp +++ b/MobileGL/MG_Test/Texture/TextureViewTest.cpp @@ -25,6 +25,7 @@ #include "Init.h" #include #include +#include #include #include #include @@ -190,6 +191,36 @@ namespace { EXPECT_EQ(GetViewParameter(view, GL_TEXTURE_2D, GL_TEXTURE_IMMUTABLE_LEVELS), 3); } + // ...and the level a FRAMEBUFFER may attach is the view's own count, not the inherited + // TEXTURE_IMMUTABLE_LEVELS the test above pins. Bounding glFramebufferTexture by the latter + // accepted a level the view cannot reach, which attaches a 0x0 image: the framebuffer then + // reports COMPLETE and nothing can be drawn into it. + TEST_F(TextureViewTest, AFramebufferAttachIsBoundedByTheViewsOwnLevelCount) { + const GLuint storage = MakeImmutable2D(4, 32, 32); + const GLuint view = GenTexture(); + MG_Impl::GLImpl::TextureView(view, GL_TEXTURE_2D, storage, GL_RGBA8, /*minlevel=*/2, + /*numlevels=*/2, 0, 1); + ExpectSingleGlError(GL_NO_ERROR); + // The inherited query really does report the original's four levels... + ASSERT_EQ(GetViewParameter(view, GL_TEXTURE_2D, GL_TEXTURE_IMMUTABLE_LEVELS), 4); + // ...while the view itself has two. + ASSERT_EQ(GetViewParameter(view, GL_TEXTURE_2D, GL_TEXTURE_VIEW_NUM_LEVELS), 2); + + GLuint framebuffer = 0; + MG_Impl::GLImpl::CreateFramebuffers(1, &framebuffer); + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, framebuffer); + ExpectSingleGlError(GL_NO_ERROR); + + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, view, 1); + ExpectSingleGlError(GL_NO_ERROR); + + MG_Impl::GLImpl::FramebufferTexture(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, view, 2); + ExpectSingleGlError(GL_INVALID_VALUE); + + MG_Impl::GLImpl::BindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); + DrainPendingGlErrors(); + } + TEST_F(TextureViewTest, ViewClampsItsLevelCountToWhatRemains) { const GLuint storage = MakeImmutable2D(3, 16, 16); const GLuint view = GenTexture();