GL: don't assume debug output callbacks are called from the same thread.

Interesting that I didn't run into this until testing on Mesa AMD
drivers. So far it worked for NV, Mesa Intel, Intel Windows, Android and
many more. Heh. Also improved the test to actually verify the user
pointer gets passed through correctly and updated the docs to reflect
this behavior.

doc/changelog.dox

@@ -157,6 +157,10 @@ See also:
 -   Reworked @ref GL::Mesh internals to make use of
     @gl_extension{ARB,direct_state_access} instead of being the last code path
     stuck on @gl_extension{EXT,direct_state_access}
+-   @ref GL::DebugOutput::setCallback() no longer assumes the debug message
+    callback is called from the same thread as the one with the corresponding
+    GL context (which is the case on Mesa AMD drivers). The documentation was
+    updated to reflect that as well.
 
 @subsubsection changelog-latest-changes-math Math library
 

src/Magnum/GL/DebugOutput.cpp

@@ -36,18 +36,10 @@
 
 namespace Magnum { namespace GL {
 
-namespace {
-
-void
-#ifdef CORRADE_TARGET_WINDOWS
-APIENTRY
-#endif
-callbackWrapper(GLenum source, GLenum type, GLuint id, GLenum severity, GLsizei length, const GLchar* message, const void* userParam) {
-    Context::current().state().debug->messageCallback(DebugOutput::Source(source), DebugOutput::Type(type), id, DebugOutput::Severity(severity), std::string{message, std::size_t(length)}, userParam);
-}
+namespace Implementation {
 
-void defaultCallback(const DebugOutput::Source source, const DebugOutput::Type type, const UnsignedInt id, const DebugOutput::Severity severity, const std::string& string, const void*) {
-    Debug output;
+void defaultDebugCallback(const DebugOutput::Source source, const DebugOutput::Type type, const UnsignedInt id, const DebugOutput::Severity severity, const std::string& string, std::ostream* out) {
+    Debug output{out};
     output << "Debug output:";
 
     switch(severity) {
@@ -104,6 +96,19 @@ void defaultCallback(const DebugOutput::Source source, const DebugOutput::Type t
 
 }
 
+namespace {
+
+void
+#ifdef CORRADE_TARGET_WINDOWS
+APIENTRY
+#endif
+callbackWrapper(GLenum source, GLenum type, GLuint id, GLenum severity, GLsizei length, const GLchar* message, const void* userParam) {
+    const auto& callback = *static_cast<const Implementation::DebugState::MessageCallback*>(userParam);
+    callback.callback(DebugOutput::Source(source), DebugOutput::Type(type), id, DebugOutput::Severity(severity), std::string{message, std::size_t(length)}, callback.userParam);
+}
+
+}
+
 Int DebugOutput::maxLoggedMessages() {
     if(!Context::current().isExtensionSupported<Extensions::KHR::debug>())
         return 0;
@@ -143,7 +148,9 @@ void DebugOutput::setCallback(const Callback callback, const void* userParam) {
 }
 
 void DebugOutput::setDefaultCallback() {
-    setCallback(defaultCallback, nullptr);
+    setCallback([](DebugOutput::Source source, DebugOutput::Type type, UnsignedInt id, DebugOutput::Severity severity, const std::string& string, const void*) {
+        Implementation::defaultDebugCallback(source, type, id, severity, string, Debug::output());
+    });
 }
 
 void DebugOutput::setEnabledInternal(const GLenum source, const GLenum type, const GLenum severity, const std::initializer_list<UnsignedInt> ids, const bool enabled) {
@@ -169,12 +176,13 @@ void DebugOutput::callbackImplementationNoOp(Callback, const void*) {}
 #ifndef MAGNUM_TARGET_GLES2
 void DebugOutput::callbackImplementationKhrDesktopES32(const Callback callback, const void* userParam) {
     /* Replace the callback */
-    const Callback original = Context::current().state().debug->messageCallback;
-    Context::current().state().debug->messageCallback = callback;
+    const Callback original = Context::current().state().debug->messageCallback.callback;
+    Context::current().state().debug->messageCallback.callback = callback;
+    Context::current().state().debug->messageCallback.userParam = userParam;
 
     /* Adding callback */
     if(!original && callback)
-        glDebugMessageCallback(callbackWrapper, userParam);
+        glDebugMessageCallback(callbackWrapper,  &Context::current().state().debug->messageCallback);
 
     /* Deleting callback */
     else if(original && !callback)
@@ -185,12 +193,13 @@ void DebugOutput::callbackImplementationKhrDesktopES32(const Callback callback,
 #ifdef MAGNUM_TARGET_GLES
 void DebugOutput::callbackImplementationKhrES(const Callback callback, const void* userParam) {
     /* Replace the callback */
-    const Callback original = Context::current().state().debug->messageCallback;
-    Context::current().state().debug->messageCallback = callback;
+    const Callback original = Context::current().state().debug->messageCallback.callback;
+    Context::current().state().debug->messageCallback.callback = callback;
+    Context::current().state().debug->messageCallback.userParam = userParam;
 
     /* Adding callback */
     if(!original && callback)
-        glDebugMessageCallbackKHR(callbackWrapper, userParam);
+        glDebugMessageCallbackKHR(callbackWrapper, &Context::current().state().debug->messageCallback);
 
     /* Deleting callback */
     else if(original && !callback)

src/Magnum/GL/DebugOutput.h

@@ -369,6 +369,13 @@ class MAGNUM_GL_EXPORT DebugOutput {
          * OpenGL ES 3.2 is not supported and @gl_extension{KHR,debug} desktop or
          * ES extension (covered also by @gl_extension{ANDROID,extension_pack_es31a})
          * is not available, this function does nothing.
+         *
+         * @attention The function is not necessarily called from the same
+         *      thread as the one that caused the message to appear --- in
+         *      particular, you can't assume the @ref GL::Context will be
+         *      present in the callback context. It might work on some drivers,
+         *      but not on others.
+         *
          * @see @ref setDefaultCallback(),
          *      @ref Renderer::Feature::DebugOutputSynchronous,
          *      @fn_gl_keyword{DebugMessageCallback}
@@ -805,6 +812,11 @@ class MAGNUM_GL_EXPORT DebugGroup {
 /** @debugoperatorclassenum{DebugGroup,DebugGroup::Source} */
 MAGNUM_GL_EXPORT Debug& operator<<(Debug& debug, DebugGroup::Source value);
 
+/* Exposed for testing */
+namespace Implementation {
+    MAGNUM_GL_EXPORT void defaultDebugCallback(DebugOutput::Source source, DebugOutput::Type type, UnsignedInt id, DebugOutput::Severity severity, const std::string& string, std::ostream* output);
+}
+
 }}
 #else
 #error this header is not available in WebGL build

src/Magnum/GL/Implementation/DebugState.cpp

@@ -35,8 +35,7 @@ DebugState::DebugState(Context& context, std::vector<std::string>& extensions):
     maxLabelLength{0},
     maxLoggedMessages{0},
     maxMessageLength{0},
-    maxStackDepth{0},
-    messageCallback(nullptr)
+    maxStackDepth{0}
 {
     #ifndef MAGNUM_TARGET_GLES2
     #ifndef MAGNUM_TARGET_GLES

src/Magnum/GL/Implementation/DebugState.h

@@ -50,7 +50,10 @@ struct DebugState {
     void(*popGroupImplementation)();
 
     GLint maxLabelLength, maxLoggedMessages, maxMessageLength, maxStackDepth;
-    DebugOutput::Callback messageCallback;
+    struct MessageCallback {
+        DebugOutput::Callback callback{};
+        const void* userParam{};
+    } messageCallback;
 };
 
 }}}

src/Magnum/GL/OpenGLTester.cpp

@@ -50,6 +50,7 @@ OpenGLTester::OpenGLTester(): TestSuite::Tester{TestSuite::Tester::TesterConfigu
         DebugOutput::setDefaultCallback();
 
         /* Disable "Buffer detailed info" message on NV (too spammy) */
+        if(Context::current().detectedDriver() & Context::DetectedDriver::NVidia)
             DebugOutput::setEnabled(DebugOutput::Source::Api, DebugOutput::Type::Other, {131185}, false);
     }
     #endif

src/Magnum/GL/Test/DebugOutputGLTest.cpp

@@ -35,7 +35,11 @@ namespace Magnum { namespace GL { namespace Test { namespace {
 struct DebugOutputGLTest: OpenGLTester {
     explicit DebugOutputGLTest();
 
-    void setCallback();
+    void setCallbackDefault();
+
+    void setup();
+    void teardown();
+
     void setEnabled();
 
     void messageNoOp();
@@ -45,11 +49,14 @@ struct DebugOutputGLTest: OpenGLTester {
     void groupNoOp();
     void group();
     void groupFallback();
+
+    std::ostringstream _out;
 };
 
 DebugOutputGLTest::DebugOutputGLTest() {
-    addTests({&DebugOutputGLTest::setCallback,
-              &DebugOutputGLTest::setEnabled,
+    addTests({&DebugOutputGLTest::setCallbackDefault});
+
+    addTests({&DebugOutputGLTest::setEnabled,
 
               &DebugOutputGLTest::messageNoOp,
               &DebugOutputGLTest::message,
@@ -57,10 +64,11 @@ DebugOutputGLTest::DebugOutputGLTest() {
 
               &DebugOutputGLTest::groupNoOp,
               &DebugOutputGLTest::group,
-              &DebugOutputGLTest::groupFallback});
+              &DebugOutputGLTest::groupFallback},
+        &DebugOutputGLTest::setup, &DebugOutputGLTest::teardown);
 }
 
-void DebugOutputGLTest::setCallback() {
+void DebugOutputGLTest::setCallbackDefault() {
     if(!Context::current().isExtensionSupported<Extensions::KHR::debug>())
         CORRADE_SKIP(Extensions::KHR::debug::string() + std::string(" is not supported"));
 
@@ -70,16 +78,41 @@ void DebugOutputGLTest::setCallback() {
     MAGNUM_VERIFY_NO_GL_ERROR();
 }
 
+void DebugOutputGLTest::setup() {
+    _out.str({});
+
+    if(Context::current().isExtensionSupported<Extensions::KHR::debug>())
+        DebugOutput::setCallback([](DebugOutput::Source source, DebugOutput::Type type, UnsignedInt id, DebugOutput::Severity severity, const std::string& string, const void* userPtr) {
+            Implementation::defaultDebugCallback(source, type, id, severity, string, static_cast<std::ostringstream*>(const_cast<void*>(userPtr)));
+        }, &_out);
+}
+
+void DebugOutputGLTest::teardown() {
+    if(Context::current().isExtensionSupported<Extensions::KHR::debug>())
+        DebugOutput::setDefaultCallback();
+}
+
 void DebugOutputGLTest::setEnabled() {
     if(!Context::current().isExtensionSupported<Extensions::KHR::debug>())
         CORRADE_SKIP(Extensions::KHR::debug::string() + std::string(" is not supported"));
 
-    /* Try at least some combinations */
+    /* Try at least some combinations. Calling a less-specific version will
+       reset the more-specific setting from earlier. */
+    DebugOutput::setEnabled(true);
     DebugOutput::setEnabled(DebugOutput::Source::Application, true);
     DebugOutput::setEnabled(DebugOutput::Source::Application, DebugOutput::Type::UndefinedBehavior, {3168, 35487, 234487}, false);
+
+    MAGNUM_VERIFY_NO_GL_ERROR();
+
+    DebugMessage::insert(DebugMessage::Source::Application, DebugMessage::Type::UndefinedBehavior,
+        35487, DebugOutput::Severity::Notification, "This message should get ignored");
+
+    /* Reset everything back */
     DebugOutput::setEnabled(true);
 
     MAGNUM_VERIFY_NO_GL_ERROR();
+
+    CORRADE_COMPARE(_out.str(), "");
 }
 
 void DebugOutputGLTest::messageNoOp() {
@@ -95,6 +128,8 @@ void DebugOutputGLTest::messageNoOp() {
         1337, DebugOutput::Severity::Notification, "Hello from OpenGL command stream!");
 
     MAGNUM_VERIFY_NO_GL_ERROR();
+
+    CORRADE_COMPARE(_out.str(), "");
 }
 
 void DebugOutputGLTest::message() {
@@ -102,13 +137,11 @@ void DebugOutputGLTest::message() {
         CORRADE_SKIP(Extensions::KHR::debug::string() + std::string(" is not supported"));
 
     /* Need to be careful, because the test runner is using debug output too */
-    std::ostringstream out;
-    Debug redirectDebug{&out};
     DebugMessage::insert(DebugMessage::Source::Application, DebugMessage::Type::Marker,
         1337, DebugOutput::Severity::High, "Hello from OpenGL command stream!");
 
     MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE(out.str(),
+    CORRADE_COMPARE(_out.str(),
         "Debug output: high severity application marker (1337): Hello from OpenGL command stream!\n");
 }
 
@@ -125,6 +158,8 @@ void DebugOutputGLTest::messageFallback() {
         1337, DebugOutput::Severity::Notification, "Hello from OpenGL command stream!");
 
     MAGNUM_VERIFY_NO_GL_ERROR();
+
+    CORRADE_COMPARE(_out.str(), "");
 }
 
 void DebugOutputGLTest::groupNoOp() {
@@ -137,6 +172,7 @@ void DebugOutputGLTest::groupNoOp() {
     }
 
     MAGNUM_VERIFY_NO_GL_ERROR();
+    CORRADE_COMPARE(_out.str(), "");
 }
 
 void DebugOutputGLTest::group() {
@@ -144,8 +180,6 @@ void DebugOutputGLTest::group() {
         CORRADE_SKIP(Extensions::KHR::debug::string() + std::string(" is not supported"));
 
     /* Need to be careful, because the test runner is using debug output too */
-    std::ostringstream out;
-    Debug redirectDebug{&out};
     {
         DebugGroup g1{DebugGroup::Source::Application, 42, "Automatic debug group"};
         DebugGroup g2;
@@ -154,7 +188,7 @@ void DebugOutputGLTest::group() {
     }
 
     MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE(out.str(),
+    CORRADE_COMPARE(_out.str(),
         "Debug output: application debug group enter (42): Automatic debug group\n"
         "Debug output: third party debug group enter (1337): Manual debug group\n"
         "Debug output: third party debug group leave (1337): Manual debug group\n"
@@ -171,6 +205,7 @@ void DebugOutputGLTest::groupFallback() {
     }
 
     MAGNUM_VERIFY_NO_GL_ERROR();
+    CORRADE_COMPARE(_out.str(), "");
 }
 
 }}}}