[Fix] (Framebuffer): apply the glFramebufferTexture error conditions to the 2D/3D/Layer attach paths and bound a view by its own level count

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<MG_State::GLState::FramebufferObject>&
+                                                           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<GenericErrorInfo>(
+                        "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<MG_State::GLState::ITextureObject>& textureObject,
+                                             GLint level) {
+            if (level < 0) {
+                MG_State::pGLContext->RecordError(
+                    ErrorCode::InvalidValue,
+                    MakeUnique<GenericErrorInfo>("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<Uint>(level) >= levelBound) {
+                MG_State::pGLContext->RecordError(
+                    ErrorCode::InvalidValue,
+                    MakeUnique<GenericErrorInfo>(
+                        "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<GenericErrorInfo>(
-                        "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<GenericErrorInfo>("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<GLint>(textureObject->GetImmutableLevels())) {
-                MG_State::pGLContext->RecordError(
-                    ErrorCode::InvalidValue,
-                    MakeUnique<GenericErrorInfo>(
-                        "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<GenericErrorInfo>("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<GenericErrorInfo>("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;

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 <numlevels> 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;

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<GLint>(
+                                  MG_Backend::pActiveBackendObject->GetDynamicParameters().MaxColorAttachments)
+                            : static_cast<GLint>(MG_State::GLState::FramebufferObject::MAX_DRAW_BUFFERS);
+    MG_Impl::GLImpl::FramebufferTexture2D(GL_DRAW_FRAMEBUFFER,
+                                          static_cast<GLenum>(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();
+}

MobileGL/MG_Test/Texture/TextureViewTest.cpp

@@ -25,6 +25,7 @@
 #include "Init.h"
 #include <MG_Backend/BackendObjects.h>
 #include <MG_Impl/GLImpl/Getter/GL_Getter.h>
+#include <MG_Impl/GLImpl/Framebuffer/GL_Framebuffer.h>
 #include <MG_Impl/GLImpl/Texture/GL_Texture.h>
 #include <MG_State/GLState/Core.h>
 #include <MG_State/GLState/TextureState/TextureObject.h>
@@ -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();