GL: added *Framebuffer::implementationColorRead{Format,Type}().

Long overdue. Also wow, the drivers ARE SHIT.

doc/changelog.dox

@@ -38,6 +38,19 @@ See also:
 
 @section changelog-latest Changes since 2019.01
 
+@subsection changelog-latest-new New features
+
+@subsubsection changelog-latest-new-gl GL library
+
+-   New @ref GL::AbstractFramebuffer::implementationColorReadFormat() and
+    @ref GL::AbstractFramebuffer::implementationColorReadType() queries for
+    more robust checks when doing framebuffer readbacks; together with three
+    new workarounds @cpp "nv-implementation-color-read-format-dsa-broken" @ce,
+    @cpp "intel-windows-implementation-color-read-format-completely-broken" @ce
+    and @cpp "mesa-implementation-color-read-format-dsa-explicit-binding" @ce,
+    because it's 2019 and GL drivers are *still* awful. See
+    @ref opengl-workarounds for more information.
+
 @subsection changelog-latest-changes Changes and improvements
 
 @subsubsection changelog-latest-changes-audio Audio library

doc/opengl-mapping.dox

@@ -450,8 +450,8 @@ glGet() parameter                       | Matching API
 @def_gl{DRAW_BUFFERi}, \n @def_gl{DRAW_BUFFER}, \n @def_gl{READ_BUFFER} | not queryable, @ref GL::DefaultFramebuffer::mapForDraw(), \n @ref GL::DefaultFramebuffer::mapForRead(), \n @ref GL::Framebuffer::mapForDraw() and \n @ref GL::Framebuffer::mapForRead() setters only
 @def_gl{DRAW_FRAMEBUFFER_BINDING}, \n @def_gl{READ_FRAMEBUFFER_BINDING} | not queryable but tracked internally
 @def_gl{FRAGMENT_SHADER_DERIVATIVE_HINT}, \n @def_gl{LINE_SMOOTH_HINT}, \n @def_gl{POLYGON_SMOOTH_HINT}, \n @def_gl{TEXTURE_COMPRESSION_HINT} | not queryable
-@def_gl{IMPLEMENTATION_COLOR_READ_FORMAT} | |
-@def_gl{IMPLEMENTATION_COLOR_READ_TYPE} | |
+@def_gl{IMPLEMENTATION_COLOR_READ_FORMAT} | @ref GL::AbstractFramebuffer::implementationColorReadFormat()
+@def_gl{IMPLEMENTATION_COLOR_READ_TYPE} | @ref GL::AbstractFramebuffer::implementationColorReadType()
 @def_gl{LAYER_PROVOKING_VERTEX}         | |
 @def_gl{LINE_SMOOTH}, \n @def_gl{POLYGON_SMOOTH} | not supported (@ref opengl-unsupported "details")
 @def_gl{LINE_WIDTH}                     | not queryable, @ref GL::Renderer::setLineWidth() setter only

src/Magnum/GL/AbstractFramebuffer.cpp

@@ -213,6 +213,46 @@ FramebufferTarget AbstractFramebuffer::bindImplementationDefault() {
     return FramebufferTarget::Read;
 }
 
+PixelFormat AbstractFramebuffer::implementationColorReadFormat() {
+    return PixelFormat((this->*Context::current().state().framebuffer->implementationColorReadFormatTypeImplementation)(GL_IMPLEMENTATION_COLOR_READ_FORMAT));
+}
+
+PixelType AbstractFramebuffer::implementationColorReadType() {
+    return PixelType((this->*Context::current().state().framebuffer->implementationColorReadFormatTypeImplementation)(GL_IMPLEMENTATION_COLOR_READ_TYPE));
+}
+
+GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationDefault(const GLenum what) {
+    bindInternal(FramebufferTarget::Read);
+    GLint formatType;
+    glGetIntegerv(what, &formatType);
+    return formatType;
+}
+
+#if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL)
+GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31(const GLenum what) {
+    const FramebufferTarget target = bindInternal();
+    GLint formatType;
+    glGetFramebufferParameteriv(GLenum(target), what, &formatType);
+    return formatType;
+}
+#endif
+
+#ifndef MAGNUM_TARGET_GLES
+GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31DSA(const GLenum what) {
+    GLint formatType;
+    glGetNamedFramebufferParameteriv(_id, what, &formatType);
+    return formatType;
+}
+
+GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31DSAMesa(const GLenum what) {
+    /* Mesa needs the framebuffer bound for read even with DSA. See the
+       "mesa-implementation-color-read-format-dsa-explicit-binding" workaround
+       for details. */
+    bindInternal(FramebufferTarget::Read);
+    return implementationColorReadFormatTypeImplementationES31DSA(what);
+}
+#endif
+
 #if !(defined(MAGNUM_TARGET_WEBGL) && defined(MAGNUM_TARGET_GLES2))
 void AbstractFramebuffer::blit(AbstractFramebuffer& source, AbstractFramebuffer& destination, const Range2Di& sourceRectangle, const Range2Di& destinationRectangle, const FramebufferBlitMask mask, const FramebufferBlitFilter filter) {
     Context::current().state().framebuffer->blitImplementation(source, destination, sourceRectangle, destinationRectangle, mask, filter);

src/Magnum/GL/AbstractFramebuffer.h

@@ -263,6 +263,38 @@ class MAGNUM_GL_EXPORT AbstractFramebuffer {
         /** @brief Viewport rectangle */
         Range2Di viewport() const { return _viewport; }
 
+        /**
+         * @brief Implementation-specific color read format
+         *
+         * The result is not cached in any way. If
+         * @gl_extension{ARB,direct_state_access} (part of OpenGL 4.5) is not
+         * available, the framebuffer is bound to some target before the
+         * operation (if not already).
+         * @see @ref implementationColorReadType(),
+         *      @fn_gl{GetNamedFramebufferParameter} with
+         *      @def_gl_keyword{IMPLEMENTATION_COLOR_READ_FORMAT},
+         *      eventually @fn_gl{BindFramebuffer} and either
+         *      @fn_gl{GetFramebufferParameter} or @fn_gl{Get} with
+         *      @def_gl{IMPLEMENTATION_COLOR_READ_FORMAT}
+         */
+        PixelFormat implementationColorReadFormat();
+
+        /**
+         * @brief Implementation-specific color read type
+         *
+         * The result is not cached in any way. If
+         * @gl_extension{ARB,direct_state_access} (part of OpenGL 4.5) is not
+         * available, the framebuffer is bound to some target before the
+         * operation (if not already).
+         * @see @ref implementationColorReadFormat(),
+         *      @fn_gl{GetNamedFramebufferParameter} with
+         *      @def_gl_keyword{IMPLEMENTATION_COLOR_READ_TYPE}, eventually
+         *      @fn_gl{BindFramebuffer} and either
+         *      @fn_gl{GetFramebufferParameter} or @fn_gl{Get} with
+         *      @def_gl{IMPLEMENTATION_COLOR_READ_TYPE}
+         */
+        PixelType implementationColorReadType();
+
         /**
          * @brief Set viewport
          * @return Reference to self (for method chaining)
@@ -755,6 +787,15 @@ class MAGNUM_GL_EXPORT AbstractFramebuffer {
         FramebufferTarget MAGNUM_GL_LOCAL bindImplementationSingle();
         #endif
 
+        GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationDefault(GLenum what);
+        #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL)
+        GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationES31(GLenum what);
+        #endif
+        #ifndef MAGNUM_TARGET_GLES
+        GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationES31DSA(GLenum what);
+        GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationES31DSAMesa(GLenum what);
+        #endif
+
         GLenum MAGNUM_GL_LOCAL checkStatusImplementationDefault(FramebufferTarget target);
         #ifdef MAGNUM_TARGET_GLES2
         GLenum MAGNUM_GL_LOCAL checkStatusImplementationSingle(FramebufferTarget);

src/Magnum/GL/Implementation/FramebufferState.cpp

@@ -233,6 +233,54 @@ FramebufferState::FramebufferState(Context& context, std::vector<std::string>& e
     #endif
     #endif
 
+    /* Implementation-specific color read format/type implementation */
+    #ifndef MAGNUM_TARGET_GLES
+    if(context.isExtensionSupported<Extensions::ARB::direct_state_access>() && context.isExtensionSupported<Extensions::ARB::ES3_1_compatibility>()) {
+        #ifdef CORRADE_TARGET_WINDOWS
+        if((context.detectedDriver() & Context::DetectedDriver::IntelWindows) &&
+            !context.isDriverWorkaroundDisabled("intel-windows-implementation-color-read-format-completely-broken")) {
+            implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationDefault;
+
+        } else
+        #endif
+        {
+            /* DSA extension added above */
+            extensions.push_back(Extensions::ARB::ES3_1_compatibility::string());
+
+            if((context.detectedDriver() & Context::DetectedDriver::NVidia) &&
+                !context.isDriverWorkaroundDisabled("nv-implementation-color-read-format-dsa-broken")) {
+                implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31;
+            } else if((context.detectedDriver() & Context::DetectedDriver::Mesa) &&
+                !context.isDriverWorkaroundDisabled("mesa-implementation-color-read-format-dsa-explicit-binding")) {
+                implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31DSAMesa;
+            } else {
+                implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31DSA;
+            }
+        }
+    } else
+    #endif
+    #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL)
+    /* Checks for 3.1 on ES and for ARB_ES3_1_compatibility on desktop */
+    if(context.isVersionSupported(Version::GLES310)) {
+        #ifdef CORRADE_TARGET_WINDOWS
+        if((context.detectedDriver() & Context::DetectedDriver::IntelWindows) &&
+            !context.isDriverWorkaroundDisabled("intel-windows-implementation-color-read-format-completely-broken")) {
+            implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationDefault;
+        } else
+        #endif
+        {
+            #ifndef MAGNUM_TARGET_GLES
+            extensions.push_back(Extensions::ARB::ES3_1_compatibility::string());
+            #endif
+
+            implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31;
+        }
+    } else
+    #endif
+    {
+        implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationDefault;
+    }
+
     /* Framebuffer reading implementation in desktop/ES */
     #ifndef MAGNUM_TARGET_WEBGL
     #ifndef MAGNUM_TARGET_GLES

src/Magnum/GL/Implementation/FramebufferState.h

@@ -84,6 +84,8 @@ struct FramebufferState {
     FramebufferTarget(AbstractFramebuffer::*bindInternalImplementation)();
     #endif
 
+    GLenum(AbstractFramebuffer::*implementationColorReadFormatTypeImplementation)(GLenum what);
+
     void(Framebuffer::*createImplementation)();
     void(Framebuffer::*renderbufferImplementation)(Framebuffer::BufferAttachment, GLuint);
     #ifndef MAGNUM_TARGET_GLES

src/Magnum/GL/Implementation/driverSpecific.cpp

@@ -139,6 +139,36 @@ namespace {
            EGL_NO_SURFACE. Supplying a 32x32 PBuffer to work around that. */
         "swiftshader-egl-context-needs-pbuffer",
         #endif
+
+        #ifndef MAGNUM_TARGET_GLES
+        /* Even with the DSA variant, where GL_IMPLEMENTATION_COLOR_READ_* is
+           passed to glGetNamedFramebufferParameter(), Mesa complains that the
+           framebuffer is not bound for reading. Relevant code:
+           https://github.com/mesa3d/mesa/blob/212c0c630a849e4737e2808a993d708cbb2f18f7/src/mesa/main/framebuffer.c#L841-L843
+           Workaround is to explicitly bind the framebuffer for reading. */
+        "mesa-implementation-color-read-format-dsa-explicit-binding",
+        #endif
+
+        #if !defined(MAGNUM_TARGET_GLES2) && defined(CORRADE_TARGET_WINDOWS)
+        /* Intel drivers on Windows return GL_UNSIGNED_BYTE for *both*
+           GL_IMPLEMENTATION_COLOR_READ_FORMAT and _TYPE when using either glGetNamedFramebufferParameter() or glGetFramebufferParameter(),
+           independently on what's the actual framebuffer format. Using
+           glGetInteger() makes it return GL_RGBA and GL_UNSIGNED_BYTE for
+           RGBA8 framebuffers, and cause a "Error has been generated. GL error
+           GL_INVALID_OPERATION in GetIntegerv: (ID: 2576729458) Generic error"
+           when it is not. Since glGetInteger() is actually able to return a
+           correct value in *one circumstance*, it's preferrable to the other
+           random shit the driver is doing. */
+        "intel-windows-implementation-color-read-format-completely-broken",
+        #endif
+
+        #ifndef MAGNUM_TARGET_GLES
+        /* NVidia seems to be returning values for the default framebuffer when
+           GL_IMPLEMENTATION_COLOR_READ_FORMAT and _TYPE is queried using
+           glGetNamedFramebufferParameter(). Using either glGetInteger() or
+           glGetFramebufferParameter() works correctly. */
+        "nv-implementation-color-read-format-dsa-broken",
+        #endif
         /* [workarounds] */
     };
 }

src/Magnum/GL/Test/FramebufferGLTest.cpp

@@ -160,12 +160,39 @@ struct FramebufferGLTest: OpenGLTester {
     void blit();
     #endif
 
+    void implementationColorReadFormat();
+
     #if defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL)
     private:
         TextureFormat rgbaFormatES2, depthStencilFormatES2;
     #endif
 };
 
+constexpr struct {
+    const char* name;
+    RenderbufferFormat renderbufferFormat;
+    PixelFormat expectedFormat;
+    PixelType expectedType;
+} ImplementationColorReadFormatData[]{
+    {"classic",
+        #if !defined(MAGNUM_TARGET_GLES2) || !defined(MAGNUM_TARGET_WEBGL)
+        RenderbufferFormat::RGBA8,
+        #else
+        RenderbufferFormat::RGBA4,
+        #endif
+        PixelFormat::RGBA,
+        #if !defined(MAGNUM_TARGET_GLES2) || !defined(MAGNUM_TARGET_WEBGL)
+        PixelType::UnsignedByte
+        #else
+        GL::PixelType::UnsignedShort4444
+        #endif
+        },
+    #ifndef MAGNUM_TARGET_GLES2
+    {"integer", RenderbufferFormat::RG32UI, PixelFormat::RGInteger, PixelType::UnsignedInt},
+    {"float", RenderbufferFormat::RGBA16F, PixelFormat::RGBA, PixelType::HalfFloat}
+    #endif
+};
+
 FramebufferGLTest::FramebufferGLTest() {
     addTests({&FramebufferGLTest::construct,
               &FramebufferGLTest::constructMove,
@@ -273,6 +300,8 @@ FramebufferGLTest::FramebufferGLTest() {
               #endif
               });
 
+    addInstancedTests({&FramebufferGLTest::implementationColorReadFormat}, Containers::arraySize(ImplementationColorReadFormatData));
+
     #if defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL)
     if(Context::current().isExtensionSupported<Extensions::EXT::texture_storage>()) {
         rgbaFormatES2 = TextureFormat::RGBA8;
@@ -2092,6 +2121,22 @@ void FramebufferGLTest::blit() {
 }
 #endif
 
+void FramebufferGLTest::implementationColorReadFormat() {
+    auto&& data = ImplementationColorReadFormatData[testCaseInstanceId()];
+    setTestCaseDescription(data.name);
+
+    Renderbuffer color;
+    color.setStorage(data.renderbufferFormat, {32, 32});
+    Framebuffer framebuffer{{{}, {32, 32}}};
+    framebuffer.attachRenderbuffer(Framebuffer::ColorAttachment{0}, color);
+
+    PixelFormat format = framebuffer.implementationColorReadFormat();
+    PixelType type = framebuffer.implementationColorReadType();
+    MAGNUM_VERIFY_NO_GL_ERROR();
+    CORRADE_COMPARE(format, data.expectedFormat);
+    CORRADE_COMPARE(type, data.expectedType);
+}
+
 }}}}
 
 CORRADE_TEST_MAIN(Magnum::GL::Test::FramebufferGLTest)