From a1ed7f4863dfeb657ef7d268c457d8ec5d4658e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 19 Mar 2019 16:19:03 +0100 Subject: [PATCH] GL: fix Framebuffer::implementationColorRead*() to follow spec. Not sure what I thought here, but using GL_IMPLEMENTATION_COLOR_READ_* with glGetFramebufferParameteriv() is possible only since GL 4.5 (there's no extension adding it) and there's no equivalent for it in GLES 3.1. I assumed that since the entry point is defined in 3.1 then it would also accept the same enum, but apparently not -- and this inconsistency is not fixed in 3.2 either. Funnily enough, all drivers I tried (Mesa ES, SwiftShader, ANGLE) were accepting this. The first driver that complained was ARM Mali on Huawei P10 and there I assumed a driver bug -- but on the contrary, this is the only driver following the spec properly :) Now the glGetFramebufferParameteriv() API (and the DSA equivalent) is used only on GL 4.5 contexts and up, everywhere else it's done using the old-style glGetInteger(). I also renamed the implementations to not misleadingly mention ES 3.1, since it has nothing to do with this. The code path dispatch code is a bit entangled due to all the workarounds, hopefully not breaking any behavior that worked before. --- src/Magnum/GL/AbstractFramebuffer.cpp | 14 ++-- src/Magnum/GL/AbstractFramebuffer.h | 10 ++- .../GL/Implementation/FramebufferState.cpp | 68 +++++++++---------- 3 files changed, 42 insertions(+), 50 deletions(-) diff --git a/src/Magnum/GL/AbstractFramebuffer.cpp b/src/Magnum/GL/AbstractFramebuffer.cpp index 9da1b8e7f..c8d5e4bff 100644 --- a/src/Magnum/GL/AbstractFramebuffer.cpp +++ b/src/Magnum/GL/AbstractFramebuffer.cpp @@ -221,35 +221,33 @@ PixelType AbstractFramebuffer::implementationColorReadType() { return PixelType((this->*Context::current().state().framebuffer->implementationColorReadFormatTypeImplementation)(GL_IMPLEMENTATION_COLOR_READ_TYPE)); } -GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationDefault(const GLenum what) { +GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationGlobal(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) { +#ifndef MAGNUM_TARGET_GLES +GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationFramebuffer(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) { +GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationFramebufferDSA(const GLenum what) { GLint formatType; glGetNamedFramebufferParameteriv(_id, what, &formatType); return formatType; } -GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationES31DSAMesa(const GLenum what) { +GLenum AbstractFramebuffer::implementationColorReadFormatTypeImplementationFramebufferDSAMesa(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); + return implementationColorReadFormatTypeImplementationFramebufferDSA(what); } #endif diff --git a/src/Magnum/GL/AbstractFramebuffer.h b/src/Magnum/GL/AbstractFramebuffer.h index cdd9da1c9..35de2a3ee 100644 --- a/src/Magnum/GL/AbstractFramebuffer.h +++ b/src/Magnum/GL/AbstractFramebuffer.h @@ -772,13 +772,11 @@ 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 + GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationGlobal(GLenum what); #ifndef MAGNUM_TARGET_GLES - GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationES31DSA(GLenum what); - GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationES31DSAMesa(GLenum what); + GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationFramebuffer(GLenum what); + GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationFramebufferDSA(GLenum what); + GLenum MAGNUM_GL_LOCAL implementationColorReadFormatTypeImplementationFramebufferDSAMesa(GLenum what); #endif GLenum MAGNUM_GL_LOCAL checkStatusImplementationDefault(FramebufferTarget target); diff --git a/src/Magnum/GL/Implementation/FramebufferState.cpp b/src/Magnum/GL/Implementation/FramebufferState.cpp index e268d0d12..e6b236f21 100644 --- a/src/Magnum/GL/Implementation/FramebufferState.cpp +++ b/src/Magnum/GL/Implementation/FramebufferState.cpp @@ -256,50 +256,46 @@ FramebufferState::FramebufferState(Context& context, std::vector& e /* Implementation-specific color read format/type implementation */ #ifndef MAGNUM_TARGET_GLES - if(context.isExtensionSupported() && context.isExtensionSupported()) { + /* Get(Named)FramebufferParameteriv() supports querying + GL_IMPLEMENTATION_COLOR_READ_{FORMAT,TYPE} since GL 4.5. No + corresponding extension enabling this, only a mention of Bug 12360 + that's supposed to have more information about this. But the Khronos + bugzilla is lost to internet history now and everything gets redirected + to the mostly-empty GitHub issue tracker (and it doesn't even have the + old bugs imported), so this is all I got. The whole thing is a + clusterfuck: + - ES3.1 adds GetFramebufferParameteriv() but it *doesn't* allow + GL_IMPLEMENTATION_COLOR_READ_FORMAT to be used with it. ES3.2 + doesn't fix that omission either. Funnily enough, most drivers + (including NV, Mesa and SwiftShader) support such a query, the only + driver which doesn't (and thus matches the spec) is on my Huawei + P10. What. + - Intel implementation on Windows, even though supporting 4.5 and + DSA, returns absolute garbage on everything except the most basic + GetInteger query + - NVidia returns broken values when calling the DSA code path + - Mesa needs the framebuffer to be bound even for DSA queries */ + if(context.isVersionSupported(Version::GL450) #ifdef CORRADE_TARGET_WINDOWS - if((context.detectedDriver() & Context::DetectedDriver::IntelWindows) && - !context.isDriverWorkaroundDisabled("intel-windows-implementation-color-read-format-completely-broken")) { - implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationDefault; - - } else + && !((context.detectedDriver() & Context::DetectedDriver::IntelWindows) && + !context.isDriverWorkaroundDisabled("intel-windows-implementation-color-read-format-completely-broken")) #endif - { + ) { + if(context.isExtensionSupported() + && !((context.detectedDriver() & Context::DetectedDriver::NVidia) && !context.isDriverWorkaroundDisabled("nv-implementation-color-read-format-dsa-broken")) + ) { /* 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; + if((context.detectedDriver() & Context::DetectedDriver::Mesa) && !context.isDriverWorkaroundDisabled("mesa-implementation-color-read-format-dsa-explicit-binding")) + implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationFramebufferDSAMesa; + else implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationFramebufferDSA; + } else { + implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationFramebuffer; } } else #endif { - implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationDefault; + implementationColorReadFormatTypeImplementation = &AbstractFramebuffer::implementationColorReadFormatTypeImplementationGlobal; } /* Framebuffer reading implementation in desktop/ES */