diff --git a/doc/changelog.dox b/doc/changelog.dox index eec01f8ac..e42e6820c 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -251,6 +251,9 @@ See also: - A new @cpp "nv-broken-buffer-dsa" @ce workaround for a rendering corruption on NVidia 572.82 drivers happening when DSA codepaths for @ref GL::Buffer are used. See @ref opengl-workarounds for more information. +- A new @cpp "nv-framebuffer-invalidation-wants-draw-binding" @ce workaround + for @ref GL::Framebuffer::invalidate() affecting unrelated framebuffers on + NVidia. See @ref opengl-workarounds for more information. @subsubsection changelog-latest-new-math Math library diff --git a/src/Magnum/GL/AbstractFramebuffer.cpp b/src/Magnum/GL/AbstractFramebuffer.cpp index de1cb0d76..88b2eb280 100644 --- a/src/Magnum/GL/AbstractFramebuffer.cpp +++ b/src/Magnum/GL/AbstractFramebuffer.cpp @@ -510,6 +510,19 @@ void AbstractFramebuffer::invalidateImplementationDefault(AbstractFramebuffer& s #endif } +#ifndef MAGNUM_TARGET_WEBGL +void AbstractFramebuffer::invalidateImplementationNVidiaDrawFramebuffer(AbstractFramebuffer& self, const GLsizei count, const GLenum* const attachments) { + /* See the "nv-framebuffer-invalidation-wants-draw-binding" workaround for + details */ + self.bindInternal(FramebufferTarget::Draw); + #ifndef MAGNUM_TARGET_GLES2 + glInvalidateFramebuffer(GL_DRAW_FRAMEBUFFER, count, attachments); + #else + glDiscardFramebufferEXT(GL_DRAW_FRAMEBUFFER_APPLE, count, attachments); + #endif +} +#endif + #ifndef MAGNUM_TARGET_GLES void AbstractFramebuffer::invalidateImplementationDSA(AbstractFramebuffer& self, const GLsizei count, const GLenum* const attachments) { glInvalidateNamedFramebufferData(self._id, count, attachments); @@ -524,6 +537,15 @@ void AbstractFramebuffer::invalidateImplementationDefault(AbstractFramebuffer& s glInvalidateSubFramebuffer(GLenum(self.bindInternal()), count, attachments, rectangle.left(), rectangle.bottom(), rectangle.sizeX(), rectangle.sizeY()); } +#ifndef MAGNUM_TARGET_WEBGL +void AbstractFramebuffer::invalidateImplementationNVidiaDrawFramebuffer(AbstractFramebuffer& self, const GLsizei count, const GLenum* const attachments, const Range2Di& rectangle) { + /* See the "nv-framebuffer-invalidation-wants-draw-binding" workaround for + details */ + self.bindInternal(FramebufferTarget::Draw); + glInvalidateSubFramebuffer(GL_DRAW_FRAMEBUFFER, count, attachments, rectangle.left(), rectangle.bottom(), rectangle.sizeX(), rectangle.sizeY()); +} +#endif + #ifndef MAGNUM_TARGET_GLES void AbstractFramebuffer::invalidateImplementationDSA(AbstractFramebuffer& self, const GLsizei count, const GLenum* const attachments, const Range2Di& rectangle) { glInvalidateNamedFramebufferSubData(self._id, count, attachments, rectangle.left(), rectangle.bottom(), rectangle.sizeX(), rectangle.sizeY()); diff --git a/src/Magnum/GL/AbstractFramebuffer.h b/src/Magnum/GL/AbstractFramebuffer.h index be024b57c..953b311a4 100644 --- a/src/Magnum/GL/AbstractFramebuffer.h +++ b/src/Magnum/GL/AbstractFramebuffer.h @@ -894,6 +894,9 @@ class MAGNUM_GL_EXPORT AbstractFramebuffer { #if !(defined(MAGNUM_TARGET_WEBGL) && defined(MAGNUM_TARGET_GLES2)) static void MAGNUM_GL_LOCAL invalidateImplementationNoOp(AbstractFramebuffer& self, GLsizei, const GLenum*); static void MAGNUM_GL_LOCAL invalidateImplementationDefault(AbstractFramebuffer& self, GLsizei count, const GLenum* attachments); + #ifndef MAGNUM_TARGET_WEBGL + static void MAGNUM_GL_LOCAL invalidateImplementationNVidiaDrawFramebuffer(AbstractFramebuffer& self, GLsizei count, const GLenum* attachments); + #endif #ifndef MAGNUM_TARGET_GLES static void MAGNUM_GL_LOCAL invalidateImplementationDSA(AbstractFramebuffer& self, GLsizei count, const GLenum* attachments); #endif @@ -902,6 +905,9 @@ class MAGNUM_GL_EXPORT AbstractFramebuffer { #ifndef MAGNUM_TARGET_GLES2 static void MAGNUM_GL_LOCAL invalidateImplementationNoOp(AbstractFramebuffer& self, GLsizei, const GLenum*, const Range2Di&); static void MAGNUM_GL_LOCAL invalidateImplementationDefault(AbstractFramebuffer& self, GLsizei count, const GLenum* attachments, const Range2Di& rectangle); + #ifndef MAGNUM_TARGET_WEBGL + static void MAGNUM_GL_LOCAL invalidateImplementationNVidiaDrawFramebuffer(AbstractFramebuffer& self, GLsizei count, const GLenum* attachments, const Range2Di& rectangle); + #endif #ifndef MAGNUM_TARGET_GLES static void MAGNUM_GL_LOCAL invalidateImplementationDSA(AbstractFramebuffer& self, GLsizei count, const GLenum* attachments, const Range2Di& rectangle); #endif diff --git a/src/Magnum/GL/Implementation/FramebufferState.cpp b/src/Magnum/GL/Implementation/FramebufferState.cpp index 0aec3fc09..9e35d3926 100644 --- a/src/Magnum/GL/Implementation/FramebufferState.cpp +++ b/src/Magnum/GL/Implementation/FramebufferState.cpp @@ -391,7 +391,10 @@ FramebufferState::FramebufferState(Context& context, Containers::StaticArrayView extensions[Extensions::ARB::invalidate_subdata::Index] = Extensions::ARB::invalidate_subdata::string(); - if(context.isExtensionSupported()) { + if((context.detectedDriver() & Context::DetectedDriver::NVidia) && !context.isDriverWorkaroundDisabled("nv-framebuffer-invalidation-wants-draw-binding"_s)) { + invalidateImplementation = &AbstractFramebuffer::invalidateImplementationNVidiaDrawFramebuffer; + invalidateSubImplementation = &AbstractFramebuffer::invalidateImplementationNVidiaDrawFramebuffer; + } else if(context.isExtensionSupported()) { /* Extension added above */ invalidateImplementation = &AbstractFramebuffer::invalidateImplementationDSA; invalidateSubImplementation = &AbstractFramebuffer::invalidateImplementationDSA; @@ -405,21 +408,34 @@ FramebufferState::FramebufferState(Context& context, Containers::StaticArrayView invalidateSubImplementation = &AbstractFramebuffer::invalidateImplementationNoOp; } - /* Framebuffer invalidation implementation on ES2 */ + /* Framebuffer invalidation implementation on ES2. Yep, this is also + affected by the NVidia bug. */ #elif defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) if(context.isExtensionSupported()) { extensions[Extensions::EXT::discard_framebuffer::Index] = Extensions::EXT::discard_framebuffer::string(); - invalidateImplementation = &AbstractFramebuffer::invalidateImplementationDefault; + if((context.detectedDriver() & Context::DetectedDriver::NVidia) && !context.isDriverWorkaroundDisabled("nv-framebuffer-invalidation-wants-draw-binding"_s)) { + invalidateImplementation = &AbstractFramebuffer::invalidateImplementationNVidiaDrawFramebuffer; + } else { + invalidateImplementation = &AbstractFramebuffer::invalidateImplementationDefault; + } } else { invalidateImplementation = &AbstractFramebuffer::invalidateImplementationNoOp; } - /* Always available on ES3 */ + /* Always available on ES3, except for NVidia bugs */ #elif !(defined(MAGNUM_TARGET_WEBGL) && defined(MAGNUM_TARGET_GLES2)) - invalidateImplementation = &AbstractFramebuffer::invalidateImplementationDefault; - invalidateSubImplementation = &AbstractFramebuffer::invalidateImplementationDefault; + #ifndef MAGNUM_TARGET_WEBGL + if((context.detectedDriver() & Context::DetectedDriver::NVidia) && !context.isDriverWorkaroundDisabled("nv-framebuffer-invalidation-wants-draw-binding"_s)) { + invalidateImplementation = &AbstractFramebuffer::invalidateImplementationNVidiaDrawFramebuffer; + invalidateSubImplementation = &AbstractFramebuffer::invalidateImplementationNVidiaDrawFramebuffer; + } else + #endif + { + invalidateImplementation = &AbstractFramebuffer::invalidateImplementationDefault; + invalidateSubImplementation = &AbstractFramebuffer::invalidateImplementationDefault; + } #endif #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) diff --git a/src/Magnum/GL/Implementation/driverSpecific.cpp b/src/Magnum/GL/Implementation/driverSpecific.cpp index 8f25717e4..0d9c12a19 100644 --- a/src/Magnum/GL/Implementation/driverSpecific.cpp +++ b/src/Magnum/GL/Implementation/driverSpecific.cpp @@ -235,6 +235,25 @@ constexpr Containers::StringView KnownWorkarounds[]{ "nv-broken-buffer-dsa"_s, #endif +#ifndef MAGNUM_TARGET_WEBGL +/* On NV drivers (reproducible on both Linux and Windows and versions 570, 537, + so likely any version), a call to DSA glInvalidateNamedFramebufferData() + invalidates the currently bound *draw* framebuffer instead of the one + specified, but only if the framebuffer to be invalidated is currently bound + *read* framebuffer (how?! why there is such a weird interaction??). + + In a non-DSA code path, if glInvalidateFramebufferData() is called with + GL_READ_FRAMEBUFFER, it still clears the GL_DRAW_FRAMEBUFFER instead. The + workaround is thus to not use the strangely cursed DSA code path at all and + for the non-DSA code always bind & use GL_DRAW_FRAMEBUFFER. + + The same bug plagues the SubData variant as well, and even the + glDiscardFramebufferEXT() from the ancient EXT_discard_framebuffer ES2 + extension. Fully commented repro case is in the + FramebufferGLTest::invalidateAffectsDrawFramebufferNvidia() test. */ +"nv-framebuffer-invalidation-wants-draw-binding"_s, +#endif + #ifndef MAGNUM_TARGET_GLES /* SVGA3D (VMware host GL driver) glDrawArrays() draws nothing when the vertex buffer memory is initialized using glNamedBufferData() from ARB_DSA. Using diff --git a/src/Magnum/GL/Test/FramebufferGLTest.cpp b/src/Magnum/GL/Test/FramebufferGLTest.cpp index 1728a1849..3ad5ea803 100644 --- a/src/Magnum/GL/Test/FramebufferGLTest.cpp +++ b/src/Magnum/GL/Test/FramebufferGLTest.cpp @@ -25,26 +25,29 @@ DEALINGS IN THE SOFTWARE. */ +#include +#include +#include #include #include "Magnum/Image.h" #include "Magnum/ImageView.h" +#include "Magnum/GL/AbstractShaderProgram.h" +#include "Magnum/GL/Buffer.h" #include "Magnum/GL/Context.h" #include "Magnum/GL/CubeMapTexture.h" #include "Magnum/GL/Extensions.h" #include "Magnum/GL/Framebuffer.h" +#include "Magnum/GL/Mesh.h" #include "Magnum/GL/OpenGLTester.h" #include "Magnum/GL/PixelFormat.h" #include "Magnum/GL/Renderbuffer.h" #include "Magnum/GL/RenderbufferFormat.h" +#include "Magnum/GL/Shader.h" #include "Magnum/GL/Texture.h" #include "Magnum/GL/TextureFormat.h" #include "Magnum/Math/Color.h" -#ifndef MAGNUM_TARGET_WEBGL -#include -#endif - #ifndef MAGNUM_TARGET_GLES2 #include "Magnum/GL/BufferImage.h" #include "Magnum/GL/TextureArray.h" @@ -126,6 +129,7 @@ struct FramebufferGLTest: OpenGLTester { void clearStencil(); void clearDepthStencil(); #endif + #if !(defined(MAGNUM_TARGET_GLES2) && defined(MAGNUM_TARGET_WEBGL)) void invalidate(); #endif @@ -135,6 +139,10 @@ struct FramebufferGLTest: OpenGLTester { #ifndef MAGNUM_TARGET_GLES2 void invalidateSub(); #endif + #if !(defined(MAGNUM_TARGET_WEBGL) && defined(MAGNUM_TARGET_GLES2)) + void invalidateAffectsDrawFramebufferNvidia(); + #endif + void read(); void readView(); void readViewNullptr(); @@ -185,6 +193,18 @@ struct FramebufferGLTest: OpenGLTester { #endif }; +#if !(defined(MAGNUM_TARGET_GLES2) && defined(MAGNUM_TARGET_WEBGL)) +const struct { + const char* name; + bool sub; +} InvalidateAffectsDrawFramebufferNvidiaData[]{ + {"", false}, + #ifndef MAGNUM_TARGET_GLES2 + {"sub", true} + #endif +}; +#endif + constexpr struct { const char* name; RenderbufferFormat renderbufferFormat; @@ -276,6 +296,7 @@ FramebufferGLTest::FramebufferGLTest() { &FramebufferGLTest::clearStencil, &FramebufferGLTest::clearDepthStencil, #endif + #if !(defined(MAGNUM_TARGET_GLES2) && defined(MAGNUM_TARGET_WEBGL)) &FramebufferGLTest::invalidate, #endif @@ -285,7 +306,14 @@ FramebufferGLTest::FramebufferGLTest() { #ifndef MAGNUM_TARGET_GLES2 &FramebufferGLTest::invalidateSub, #endif - &FramebufferGLTest::read, + }); + + #if !(defined(MAGNUM_TARGET_WEBGL) && defined(MAGNUM_TARGET_GLES2)) + addInstancedTests({&FramebufferGLTest::invalidateAffectsDrawFramebufferNvidia}, + Containers::arraySize(InvalidateAffectsDrawFramebufferNvidiaData)); + #endif + + addTests({&FramebufferGLTest::read, &FramebufferGLTest::readView, &FramebufferGLTest::readViewNullptr, &FramebufferGLTest::readViewBadSize, @@ -341,9 +369,8 @@ FramebufferGLTest::FramebufferGLTest() { #endif } -#ifndef MAGNUM_TARGET_WEBGL using namespace Containers::Literals; -#endif +using namespace Math::Literals; void FramebufferGLTest::construct() { #ifndef MAGNUM_TARGET_GLES @@ -1511,6 +1538,142 @@ void FramebufferGLTest::invalidateSub() { } #endif +#if !(defined(MAGNUM_TARGET_WEBGL) && defined(MAGNUM_TARGET_GLES2)) +void FramebufferGLTest::invalidateAffectsDrawFramebufferNvidia() { + auto&& data = InvalidateAffectsDrawFramebufferNvidiaData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + /* Tests the "nv-framebuffer-invalidation-wants-draw-binding" workaround, + see its description for details. */ + + #ifndef MAGNUM_TARGET_GLES + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::ARB::framebuffer_object::string() << "is not supported."); + #endif + + /* Just the simplest possible shader to draw a square */ + struct Shader: AbstractShaderProgram { + typedef Attribute<0, Vector2> Position; + + explicit Shader() { + /* lol yes it's a naming clash here */ + #if defined(MAGNUM_TARGET_GLES2) + GL::Shader vert(Version::GLES200, GL::Shader::Type::Vertex); + GL::Shader frag(Version::GLES200, GL::Shader::Type::Fragment); + #else + GL::Shader vert(Version::GLES300, GL::Shader::Type::Vertex); + GL::Shader frag(Version::GLES300, GL::Shader::Type::Fragment); + #endif + + vert.addSource( + "#if !defined(GL_ES) && __VERSION__ == 120\n" + "#define mediump\n" + "#endif\n" + "#if (defined(GL_ES) && __VERSION__ < 300) || __VERSION__ == 120\n" + "#define in attribute\n" + "#endif\n" + "in mediump vec2 position;\n" + "void main() { gl_Position = vec4(position, 0.0, 1.0); }\n"_s); + frag.addSource( + "#if !defined(GL_ES) && __VERSION__ == 120\n" + "#define lowp\n" + "#endif\n" + "#if (defined(GL_ES) && __VERSION__ < 300) || __VERSION__ == 120\n" + "#define color gl_FragColor\n" + "#endif\n" + "#if (defined(GL_ES) && __VERSION__ >= 300) || (!defined(GL_ES) && __VERSION__ >= 130)\n" + "out lowp vec4 color;\n" + "#endif\n" + "void main() { color = vec4(0.0, 1.0, 1.0, 1.0); }\n"_s); + + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); + + attachShaders({vert, frag}); + bindAttributeLocation(0, "position"); + + CORRADE_INTERNAL_ASSERT_OUTPUT(link()); + } + } shader; + + /* Square that occupies center of the framebuffer */ + const Vector2 positions[]{ + /* 3--1 + | /| + |/ | + 2--0 */ + { 0.5f, -0.5f}, + { 0.5f, 0.5f}, + {-0.5f, -0.5f}, + {-0.5f, 0.5f} + }; + Mesh square{GL::MeshPrimitive::TriangleStrip}; + square + .addVertexBuffer(GL::Buffer{positions}, 0, Shader::Position{}) + .setCount(4); + + /* Two color-only framebuffers */ + Renderbuffer color1, color2; + color1.setStorage(RenderbufferFormat::RGBA8, Vector2i(16)); + color2.setStorage(RenderbufferFormat::RGBA8, Vector2i(16)); + Framebuffer first{{{}, Vector2i{16}}}, second{{{}, Vector2i{16}}}; + first.attachRenderbuffer(Framebuffer::ColorAttachment{0}, color1); + second.attachRenderbuffer(Framebuffer::ColorAttachment{0}, color2); + + /* To make the first framebuffer bound for reading but not for drawing. + If you uncomment the line below to unbind it again after, it works + properly (but only with DSA enabled, as otherwise the second.read() will + attempt to read from framebuffer 0 and generate an error). If you don't + read (and thus don't bind a GL_READ_FRAMEBUFFER at all) at all it works + as well. */ + first.read({{}, {1, 1}}, {PixelFormat::RGBA, PixelType::UnsignedByte}); + //glBindFramebuffer(GL_READ_FRAMEBUFFER, 0); + + /* Clear and bind the second framebuffer for drawing */ + GL::Renderer::setClearColor(0xff00ff_rgbf); + second + .clear(GL::FramebufferClear::Color) + .bind(); + + /* If reading here instead of above, it's broken the same way */ + //first.read({{}, {1, 1}}, {PixelFormat::RGBA, PixelType::UnsignedByte}); + + /* Render to the second framebuffer. In a DSA code path this gets ignored + due to the buggy invalidation below, and only the clear above is + executed. In other words, the invalidation only affects draws, not + clears, so it's not possible to reproduce with just clears. In a non-DSA + code path the clear gets ignored as well, for some reason. */ + shader.draw(square); + + /* The bug causes the invalidation to be called on `second`, not `first`. + In the DSA code path glInvalidateNamedFramebufferData() gets the ID of + `first` directly, but it choses to use `second` that's currently bound + for drawing. In the non-DSA code path the state tracker sees that + `first` is currently bound for reading (due to the read() above), so it + calls glInvalidateFramebufferData() with GL_READ_FRAMEBUFFER, but the + driver ignores that and uses `second` again. Uncommenting the bind(), + which binds `first` to the GL_DRAW_FRAMEBUFFER binding, will make it + correctly affect the desired framebuffer. */ + // first.bind(); + #ifndef MAGNUM_TARGET_GLES2 + if(data.sub) { + first.invalidate({Framebuffer::ColorAttachment{0}}, {{}, Vector2i{16}}); + } else + #endif + { + first.invalidate({Framebuffer::ColorAttachment{0}}); + } + + /* On the edges should be the clear color (to verify it actually works at + all, in the center the rendered square. With the bug, the center will + have the clear color as well in the DSA code path, without DSA both the + edges and the center will be all zero. */ + Image2D out = second.read({{}, Vector2i{16}}, {PixelFormat::RGBA, PixelType::UnsignedByte}); + MAGNUM_VERIFY_NO_GL_ERROR(); + CORRADE_COMPARE(out.pixels()[0][0], 0xff00ff_rgb); + CORRADE_COMPARE(out.pixels()[8][8], 0x00ffff_rgb); +} +#endif + const auto DataStorage = PixelStorage{}.setSkip({0, 16, 0}); const std::size_t DataOffset = 16*8;