From d9e7b3c384e1bdf0b7bf3e05e300b7fdad4dc389 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 16 Feb 2019 18:43:28 +0100 Subject: [PATCH] GL: added *Framebuffer::implementationColorRead{Format,Type}(). Long overdue. Also wow, the drivers ARE SHIT. --- doc/changelog.dox | 13 +++++ doc/opengl-mapping.dox | 4 +- src/Magnum/GL/AbstractFramebuffer.cpp | 40 ++++++++++++++++ src/Magnum/GL/AbstractFramebuffer.h | 41 ++++++++++++++++ .../GL/Implementation/FramebufferState.cpp | 48 +++++++++++++++++++ .../GL/Implementation/FramebufferState.h | 2 + .../GL/Implementation/driverSpecific.cpp | 30 ++++++++++++ src/Magnum/GL/Test/FramebufferGLTest.cpp | 45 +++++++++++++++++ 8 files changed, 221 insertions(+), 2 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index cb0633a59..91822c062 100644 --- a/doc/changelog.dox +++ b/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 diff --git a/doc/opengl-mapping.dox b/doc/opengl-mapping.dox index 4a7b2bc52..cb85c5de9 100644 --- a/doc/opengl-mapping.dox +++ b/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 diff --git a/src/Magnum/GL/AbstractFramebuffer.cpp b/src/Magnum/GL/AbstractFramebuffer.cpp index 9029607a9..ead656af2 100644 --- a/src/Magnum/GL/AbstractFramebuffer.cpp +++ b/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); diff --git a/src/Magnum/GL/AbstractFramebuffer.h b/src/Magnum/GL/AbstractFramebuffer.h index bb2355549..ab0576662 100644 --- a/src/Magnum/GL/AbstractFramebuffer.h +++ b/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); diff --git a/src/Magnum/GL/Implementation/FramebufferState.cpp b/src/Magnum/GL/Implementation/FramebufferState.cpp index 2cb0010c1..93fae9741 100644 --- a/src/Magnum/GL/Implementation/FramebufferState.cpp +++ b/src/Magnum/GL/Implementation/FramebufferState.cpp @@ -233,6 +233,54 @@ FramebufferState::FramebufferState(Context& context, std::vector& e #endif #endif + /* Implementation-specific color read format/type implementation */ + #ifndef MAGNUM_TARGET_GLES + if(context.isExtensionSupported() && context.isExtensionSupported()) { + #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 diff --git a/src/Magnum/GL/Implementation/FramebufferState.h b/src/Magnum/GL/Implementation/FramebufferState.h index f939c7d33..e27ad72bd 100644 --- a/src/Magnum/GL/Implementation/FramebufferState.h +++ b/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 diff --git a/src/Magnum/GL/Implementation/driverSpecific.cpp b/src/Magnum/GL/Implementation/driverSpecific.cpp index caf385f30..e85d992af 100644 --- a/src/Magnum/GL/Implementation/driverSpecific.cpp +++ b/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] */ }; } diff --git a/src/Magnum/GL/Test/FramebufferGLTest.cpp b/src/Magnum/GL/Test/FramebufferGLTest.cpp index b3c97b768..963e4f4b0 100644 --- a/src/Magnum/GL/Test/FramebufferGLTest.cpp +++ b/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()) { 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)