GL: new "angle-instanced-attributes-always-draw-instanced" workaround.

Co-authored-by: Pablo Escobar <mail@rvrs.in>

doc/changelog.dox

@@ -139,6 +139,11 @@ See also:
 -   A new @cpp "android-generic-hostname-might-be-swiftshader" @ce workaround
     needed for testing against Android Emulator SwiftShader GPU. See
     @ref opengl-workarounds for more information.
+-   A new @cpp "angle-instanced-attributes-always-draw-instanced" @ce
+    workaround for ANGLE's draw validation bug, where it complained about
+    instanced buffers being too small when drawing such mesh as non-instanced.
+    See @ref opengl-workarounds for more information, see also
+    [mosra/magnum#539](https://github.com/mosra/magnum/pull/539).
 
 @subsubsection changelog-latest-new-math Math library
 

src/Magnum/GL/Implementation/MeshState.cpp

@@ -42,6 +42,7 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S
     , maxElementIndex{0}, maxElementsIndices{0}, maxElementsVertices{0}
     #endif
 {
+    /* Vertex array object implementation */
     #ifndef MAGNUM_TARGET_GLES
     if(context.isExtensionSupported<Extensions::ARB::vertex_array_object>())
     #elif defined(MAGNUM_TARGET_GLES2)
@@ -87,7 +88,18 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S
         #endif
         {
             createImplementation = &Mesh::createImplementationVAO;
-            attributePointerImplementation = &Mesh::attributePointerImplementationVAO;
+
+            /* ANGLE is ... also special! Equivalently below for the non-VAO
+               case. */
+            #ifdef MAGNUM_TARGET_GLES
+            if((context.detectedDriver() & Context::DetectedDriver::Angle) && !context.isDriverWorkaroundDisabled("angle-instanced-attributes-always-draw-instanced"_s)) {
+                attributePointerImplementation = &Mesh::attributePointerImplementationVAOAngleAlwaysInstanced;
+            } else
+            #endif
+            {
+                attributePointerImplementation = &Mesh::attributePointerImplementationVAO;
+            }
+
             bindIndexBufferImplementation = &Mesh::bindIndexBufferImplementationVAO;
         }
 
@@ -105,7 +117,17 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S
         moveConstructImplementation = &Mesh::moveConstructImplementationDefault;
         moveAssignImplementation = &Mesh::moveAssignImplementationDefault;
         destroyImplementation = &Mesh::destroyImplementationDefault;
-        attributePointerImplementation = &Mesh::attributePointerImplementationDefault;
+
+        /* ANGLE is ... also special! Equivalently above for the VAO case. */
+        #ifdef MAGNUM_TARGET_GLES
+        if((context.detectedDriver() & Context::DetectedDriver::Angle) && !context.isDriverWorkaroundDisabled("angle-instanced-attributes-always-draw-instanced"_s)) {
+            attributePointerImplementation = &Mesh::attributePointerImplementationDefaultAngleAlwaysInstanced;
+        } else
+        #endif
+        {
+            attributePointerImplementation = &Mesh::attributePointerImplementationDefault;
+        }
+
         acquireVertexBufferImplementation = &Mesh::acquireVertexBufferImplementationDefault;
         bindIndexBufferImplementation = &Mesh::bindIndexBufferImplementationDefault;
         bindVAOImplementation = &Mesh::bindVAOImplementationDefault;

src/Magnum/GL/Implementation/driverSpecific.cpp

@@ -64,6 +64,18 @@ constexpr Containers::StringView KnownWorkarounds[]{
 "angle-chatty-shader-compiler"_s,
 #endif
 
+#ifdef MAGNUM_TARGET_GLES
+/* ANGLE has a buggy bounds validation when drawing a mesh with instanced
+   attributes added (with divisor set) using non-instanced glDraw*() APIs (in
+   particular, when instance count is 1). This should be allowed according to
+   the GL spec, which describes e.g. glDrawElements() as a special case of
+   the "virtual" glDrawElementsOneInstance(). To work around the validation
+   bug, gl*Draw*Instanced() is used unconditionally for all meshes that have
+   instanced attributes. A test that triggers this issue is in
+   MeshGLTest::drawInstancedAttributeSingleInstance(). */
+"angle-instanced-attributes-always-draw-instanced"_s,
+#endif
+
 #if defined(CORRADE_TARGET_APPLE) && !defined(MAGNUM_TARGET_GLES)
 /* Calling glBufferData(), glMapBuffer(), glMapBufferRange() or glUnmapBuffer()
    on ANY buffer when ANY buffer is attached to a currently bound

src/Magnum/GL/Mesh.cpp

@@ -792,7 +792,13 @@ void Mesh::drawInternal(Int count, Int baseVertex, Int instanceCount, GLintptr i
     (this->*state.bindImplementation)();
 
     /* Non-instanced mesh */
-    if(instanceCount == 1) {
+    if(instanceCount == 1
+        #ifdef MAGNUM_TARGET_GLES
+        /* See the "angle-instanced-attributes-always-draw-instanced"
+           workaround */
+        && !_instanced
+        #endif
+    ) {
         /* Non-indexed mesh */
         if(!_indexBuffer.id()) {
             glDrawArrays(GLenum(_primitive), baseVertex, count);
@@ -1170,6 +1176,19 @@ void Mesh::attributePointerImplementationVAODSAIntelWindows(AttributeLayout&& at
 #endif
 #endif
 
+#ifdef MAGNUM_TARGET_GLES
+/* See the "angle-instanced-attributes-always-draw-instanced" workaround for
+   these two */
+void Mesh::attributePointerImplementationDefaultAngleAlwaysInstanced(AttributeLayout&& attribute) {
+    if(attribute.divisor) _instanced = true;
+    return attributePointerImplementationDefault(std::move(attribute));
+}
+void Mesh::attributePointerImplementationVAOAngleAlwaysInstanced(AttributeLayout&& attribute) {
+    if(attribute.divisor) _instanced = true;
+    return attributePointerImplementationVAO(std::move(attribute));
+}
+#endif
+
 void Mesh::vertexAttribPointer(AttributeLayout& attribute) {
     glEnableVertexAttribArray(attribute.location);
     attribute.buffer.bindInternal(Buffer::TargetHint::Array);

src/Magnum/GL/Mesh.h

@@ -1278,6 +1278,10 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject {
         void MAGNUM_GL_LOCAL attributePointerImplementationVAODSAIntelWindows(AttributeLayout&& attribute);
         #endif
         #endif
+        #ifdef MAGNUM_TARGET_GLES
+        void MAGNUM_GL_LOCAL attributePointerImplementationDefaultAngleAlwaysInstanced(AttributeLayout&& attribute);
+        void MAGNUM_GL_LOCAL attributePointerImplementationVAOAngleAlwaysInstanced(AttributeLayout&& attribute);
+        #endif
         void MAGNUM_GL_LOCAL vertexAttribPointer(AttributeLayout& attribute);
 
         #ifndef MAGNUM_TARGET_GLES
@@ -1358,6 +1362,10 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject {
         /* Whether the _attributes storage was constructed (it's not when the
            object is constructed using NoCreate). Also fits in the gap. */
         bool _constructed{};
+        #ifdef MAGNUM_TARGET_GLES
+        /* See the "angle-instanced-attributes-always-draw-instanced" workaround */
+        bool _instanced{};
+        #endif
         Int _count{}, _baseVertex{}, _instanceCount{1};
         #ifndef MAGNUM_TARGET_GLES2
         UnsignedInt _baseInstance{};

src/Magnum/GL/Test/MeshGLTest.cpp

@@ -3,6 +3,7 @@
 
     Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019,
                 2020, 2021 Vladimír Vondruš <mosra@centrum.cz>
+    Copyright © 2022 Pablo Escobar <mail@rvrs.in>
 
     Permission is hereby granted, free of charge, to any person obtaining a
     copy of this software and associated documentation files (the "Software"),
@@ -182,6 +183,7 @@ struct MeshGLTest: OpenGLTester {
     void addVertexBufferInstancedDouble();
     #endif
     void resetDivisorAfterInstancedDraw();
+    void drawInstancedAttributeSingleInstance();
 
     void multiDraw();
     void multiDrawSparseArrays();
@@ -670,7 +672,8 @@ MeshGLTest::MeshGLTest() {
               #ifndef MAGNUM_TARGET_GLES
               &MeshGLTest::addVertexBufferInstancedDouble,
               #endif
-              &MeshGLTest::resetDivisorAfterInstancedDraw});
+              &MeshGLTest::resetDivisorAfterInstancedDraw,
+              &MeshGLTest::drawInstancedAttributeSingleInstance});
 
     addInstancedTests({&MeshGLTest::multiDraw,
                        &MeshGLTest::multiDrawSparseArrays,
@@ -3704,6 +3707,66 @@ void MeshGLTest::resetDivisorAfterInstancedDraw() {
     }
 }
 
+void MeshGLTest::drawInstancedAttributeSingleInstance() {
+    #ifndef MAGNUM_TARGET_GLES
+    if(!Context::current().isExtensionSupported<Extensions::ARB::draw_instanced>())
+        CORRADE_SKIP(Extensions::ARB::draw_instanced::string() << "is not supported.");
+    if(!Context::current().isExtensionSupported<Extensions::ARB::instanced_arrays>())
+        CORRADE_SKIP(Extensions::ARB::instanced_arrays::string() << "is not supported.");
+    #elif defined(MAGNUM_TARGET_GLES2)
+    #ifndef MAGNUM_TARGET_WEBGL
+    if(!Context::current().isExtensionSupported<Extensions::ANGLE::instanced_arrays>() &&
+       !Context::current().isExtensionSupported<Extensions::EXT::instanced_arrays>() &&
+       !Context::current().isExtensionSupported<Extensions::NV::instanced_arrays>())
+        CORRADE_SKIP("Required extension is not supported.");
+    #else
+    if(!Context::current().isExtensionSupported<Extensions::ANGLE::instanced_arrays>())
+        CORRADE_SKIP(Extensions::ANGLE::instanced_arrays::string() << "is not supported.");
+    #endif
+    #endif
+
+    typedef Attribute<0, Float> Attribute;
+
+    const Float data[]{
+        Math::unpack<Float, UnsignedByte>(96)
+    };
+    Buffer buffer;
+    /* The ANGLE validation error can be only reproduced with DynamicDraw used
+       here, not StaticDraw. Interesting. */
+    buffer.setData(data, BufferUsage::DynamicDraw);
+
+    Renderbuffer renderbuffer;
+    renderbuffer.setStorage(
+        #ifndef MAGNUM_TARGET_GLES2
+        RenderbufferFormat::RGBA8,
+        #else
+        RenderbufferFormat::RGBA4,
+        #endif
+        Vector2i(1));
+    Framebuffer framebuffer{{{}, Vector2i(1)}};
+    framebuffer.attachRenderbuffer(Framebuffer::ColorAttachment{0}, renderbuffer)
+               .bind();
+
+    FloatShader shader{"float", "vec4(valueInterpolated, 0.0, 0.0, 0.0)"};
+
+    MAGNUM_VERIFY_NO_GL_ERROR();
+
+    /* Create a mesh with (implicitly) one instance and the buffer added as
+       instanced. Drawing it 16 times should always draw 96 with no error.
+       On ANGLE w/o the "angle-instanced-attributes-always-draw-instanced"
+       workaround this would trigger a validation error where it would complain
+       that the 4-byte buffer is not large enough to draw 16 vertices. */
+    Mesh mesh;
+    mesh.addVertexBufferInstanced(buffer, 1, 0, Attribute{})
+        .setPrimitive(MeshPrimitive::Points)
+        .setCount(16);
+    shader.draw(mesh);
+
+    MAGNUM_VERIFY_NO_GL_ERROR();
+
+    CORRADE_COMPARE(Containers::arrayCast<UnsignedByte>(framebuffer.read({{}, Vector2i{1}}, {PixelFormat::RGBA, PixelType::UnsignedByte}).data())[0], 96);
+}
+
 struct MultiDrawShader: AbstractShaderProgram {
     typedef Attribute<0, Vector2> Position;
     typedef Attribute<1, Vector4> Value;