From 673022b161f0ff4b0801b40d8cef6ed2c811307b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 7 Oct 2021 16:02:43 +0200 Subject: [PATCH] GL: deduplicate global defaultFramebuffer state across shared libraries. Can't really automatically test this, unfortunately, as the windowless tests don't operate on the default framebuffer. --- doc/changelog.dox | 3 ++ src/Magnum/GL/AbstractFramebuffer.cpp | 27 ++++++++++++++---- src/Magnum/GL/AbstractFramebuffer.h | 18 ++++++++++-- src/Magnum/GL/Context.cpp | 28 +++++++++++++------ src/Magnum/GL/Context.h | 5 ++-- src/Magnum/GL/DefaultFramebuffer.cpp | 10 ------- src/Magnum/GL/DefaultFramebuffer.h | 5 ---- .../GL/Implementation/FramebufferState.h | 2 +- 8 files changed, 62 insertions(+), 36 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 4af346df2..5394da935 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -283,6 +283,9 @@ See also: flag for integration with custom-created OpenGL contexts. See also [mosra/magnum#493](https://github.com/mosra/magnum/pull/493) and [mosra/magnum#494](https://github.com/mosra/magnum/pull/494). +- The @ref GL::defaultFramebuffer global now deduplicates its state when a + statically-built Magnum is linked to multiple shared libraries, following + what the global @ref GL::Context::current() already does @subsubsection changelog-latest-changes-math Math library diff --git a/src/Magnum/GL/AbstractFramebuffer.cpp b/src/Magnum/GL/AbstractFramebuffer.cpp index 868f10dc7..53c0bf3a8 100644 --- a/src/Magnum/GL/AbstractFramebuffer.cpp +++ b/src/Magnum/GL/AbstractFramebuffer.cpp @@ -285,12 +285,23 @@ void AbstractFramebuffer::blitImplementationNV(AbstractFramebuffer& source, Abst } #endif +Range2Di AbstractFramebuffer::viewport() const { + /* For default framebuffer the viewport is stored inside the state tracker + instead. See the _viewport variable docs for details. */ + return _id ? _viewport : Context::current().state().framebuffer.defaultViewport; +} + AbstractFramebuffer& AbstractFramebuffer::setViewport(const Range2Di& rectangle) { + Implementation::FramebufferState& state = Context::current().state().framebuffer; + CORRADE_INTERNAL_ASSERT(rectangle != Implementation::FramebufferState::DisengagedViewport); - _viewport = rectangle; + + /* For default framebuffer the viewport is stored inside the state tracker + instead. See the _viewport variable docs for details. */ + (_id ? _viewport : state.defaultViewport) = rectangle; /* Update the viewport if the framebuffer is currently bound */ - if(Context::current().state().framebuffer.drawBinding == _id) + if(state.drawBinding == _id) setViewportInternal(); return *this; @@ -299,16 +310,20 @@ AbstractFramebuffer& AbstractFramebuffer::setViewport(const Range2Di& rectangle) void AbstractFramebuffer::setViewportInternal() { Implementation::FramebufferState& state = Context::current().state().framebuffer; - CORRADE_INTERNAL_ASSERT(_viewport != Implementation::FramebufferState::DisengagedViewport); + /* For default framebuffer the viewport is stored inside the state tracker + instead. See the _viewport variable docs for details. */ + const Range2Di& viewport = _id ? _viewport : Context::current().state().framebuffer.defaultViewport; + + CORRADE_INTERNAL_ASSERT(viewport != Implementation::FramebufferState::DisengagedViewport); CORRADE_INTERNAL_ASSERT(state.drawBinding == _id); /* Already up-to-date, nothing to do */ - if(state.viewport == _viewport) + if(state.viewport == viewport) return; /* Update the state and viewport */ - state.viewport = _viewport; - glViewport(_viewport.left(), _viewport.bottom(), _viewport.sizeX(), _viewport.sizeY()); + state.viewport = viewport; + glViewport(viewport.left(), viewport.bottom(), viewport.sizeX(), viewport.sizeY()); } AbstractFramebuffer& AbstractFramebuffer::clear(const FramebufferClearMask mask) { diff --git a/src/Magnum/GL/AbstractFramebuffer.h b/src/Magnum/GL/AbstractFramebuffer.h index c03a5c0b3..dcb06dc98 100644 --- a/src/Magnum/GL/AbstractFramebuffer.h +++ b/src/Magnum/GL/AbstractFramebuffer.h @@ -262,7 +262,7 @@ class MAGNUM_GL_EXPORT AbstractFramebuffer { void bind(); /** @brief Viewport rectangle */ - Range2Di viewport() const { return _viewport; } + Range2Di viewport() const; /** * @brief Set viewport @@ -740,8 +740,10 @@ class MAGNUM_GL_EXPORT AbstractFramebuffer { #else protected: #endif - /* Used by the (constexpr) DefaultFramebuffer constructor and both - the NoCreate and normal constructor of Framebuffer */ + /* Used by the (constexpr) DefaultFramebuffer constructor (which passes + in an empty viewport anyway so we don't need to pass it to the state + tracker instead) and both the NoCreate and normal constructor of + Framebuffer */ constexpr explicit AbstractFramebuffer(GLuint id, const Range2Di& viewport, ObjectFlags flags) noexcept: _id{id}, _viewport{viewport}, _flags{flags} {} ~AbstractFramebuffer() = default; @@ -767,6 +769,16 @@ class MAGNUM_GL_EXPORT AbstractFramebuffer { void MAGNUM_GL_LOCAL setViewportInternal(); GLuint _id; + /* Used only if _id != 0, for default framebuffer the viewport is + stored inside the state tracker. This is done in order to avoid race + conditions when using Magnum from multiple threads (due to the + global GL::defaultFramebuffer) and to work around issues caused by + this very global symbol being duplicated when statically-compiled + Magnum is linked to multiple shared libraries (such as every shared + library thinking the default viewport is different). It's stored in + the state tracker because it wouldn't be possible to employ our + symbol deduplication tricks on GL::defaultFramebuffer on all + platforms without turning it into a pointer or a function. */ Range2Di _viewport; ObjectFlags _flags; diff --git a/src/Magnum/GL/Context.cpp b/src/Magnum/GL/Context.cpp index fdeafb5b8..df727ff6a 100644 --- a/src/Magnum/GL/Context.cpp +++ b/src/Magnum/GL/Context.cpp @@ -41,7 +41,6 @@ #ifndef MAGNUM_TARGET_WEBGL #include "Magnum/GL/DebugOutput.h" #endif -#include "Magnum/GL/DefaultFramebuffer.h" #include "Magnum/GL/Extensions.h" #include "Magnum/GL/Framebuffer.h" #include "Magnum/GL/Mesh.h" @@ -982,13 +981,26 @@ bool Context::tryCreate(const Configuration& configuration) { if(!workaround.second) Debug(output) << " " << workaround.first; } - /** @todo Get rid of these */ - /* Initialize functionality based on current OpenGL version and extensions. - If we are on a windowless context don't touch the default framebuffer - to avoid potential race conditions with default framebuffer on another - thread. */ - if(!(_configurationFlags & Configuration::Flag::Windowless)) - DefaultFramebuffer::initializeContextBasedFunctionality(*this); + /* Fetch default framebuffer size and set up default clear color. If we are + on a windowless context don't bother retrieving the default framebuffer + viewport. This used to touch the GL::defaultFramebuffer global and the + Windowless flag got originally added to prevent a race when initializing + Magnum from multiple threads. But in order to work around issues caused + by this very global symbol being duplicated when statically-compiled + Magnum is linked to multiple shared libraries (such as every shared + library thinking the default viewport is different), the viewport had to + be moved to the state tracker, which is immune against symbol + duplication. It wouldn't be possible to deduplicate the + GL::defaultFramebuffer global itself without turning it into a pointer + or a function. */ + if(!(_configurationFlags & Configuration::Flag::Windowless)) { + Implementation::FramebufferState& state = _state->framebuffer; + GLint viewport[4]; + glGetIntegerv(GL_VIEWPORT, viewport); + state.defaultViewport = state.viewport = Range2Di::fromSize({viewport[0], viewport[1]}, {viewport[2], viewport[3]}); + CORRADE_INTERNAL_ASSERT(state.defaultViewport != Implementation::FramebufferState::DisengagedViewport); + } + /** @todo Get rid of this as well somehow */ Renderer::initializeContextBasedFunctionality(); /* Enable GPU validation, if requested */ diff --git a/src/Magnum/GL/Context.h b/src/Magnum/GL/Context.h index fe01327be..b845b8990 100644 --- a/src/Magnum/GL/Context.h +++ b/src/Magnum/GL/Context.h @@ -925,9 +925,8 @@ class MAGNUM_GL_EXPORT Context::Configuration { /** * Treat the context as windowless, assume there's no default - * framebuffer and thus don't touch @ref defaultFramebuffer in any - * way. Useful for preventing race conditions when creating OpenGL - * contexts in background threads. + * framebuffer and thus don't initialize or touch + * @ref defaultFramebuffer in any way. * * This flag is implicitly enabled in all * @ref Platform::WindowlessEglContext::Configuration "Platform::Windowless*Application::Configuration", diff --git a/src/Magnum/GL/DefaultFramebuffer.cpp b/src/Magnum/GL/DefaultFramebuffer.cpp index 8c2bb1642..7cdfb24de 100644 --- a/src/Magnum/GL/DefaultFramebuffer.cpp +++ b/src/Magnum/GL/DefaultFramebuffer.cpp @@ -113,16 +113,6 @@ void DefaultFramebuffer::invalidate(std::initializer_list