Merge pull request #986 from jpcima/runloop-timers

Fix attempt for X11 runloop problems

plugins/vst/SfizzVstEditor.cpp

@@ -59,11 +59,8 @@ bool PLUGIN_API SfizzVstEditor::open(void* parent, const VSTGUI::PlatformType& p
     config = &x11config;
 #endif
 
-    Editor* editor = editor_.get();
+    Editor* editor = new Editor(*this);
-    if (!editor) {
+    editor_.reset(editor);
-        editor = new Editor(*this);
-        editor_.reset(editor);
-    }
 
     if (!frame->open(parent, platformType, config)) {
         fprintf(stderr, "[sfizz] error opening frame\n");
@@ -122,12 +119,21 @@ void PLUGIN_API SfizzVstEditor::close()
         for (FObject* update : updates_)
             update->removeDependent(this);
 
-        if (editor_)
+        if (editor_) {
             editor_->close();
+            editor_ = nullptr;
+        }
+
         if (frame->getNbReference() != 1)
             frame->forget();
-        else
+        else {
             frame->close();
+#if !defined(__APPLE__) && !defined(_WIN32)
+            // if vstgui is done using the runloop, destroy it
+            if (!RunLoop::get())
+                _runLoop = nullptr;
+#endif
+        }
         this->frame = nullptr;
     }
 
@@ -158,8 +164,6 @@ CMessageResult SfizzVstEditor::notify(CBaseObject* sender, const char* message)
             //   notifier of X11 events is working. If there is, remove this and
             //   avoid polluting Linux hosts which implement the loop correctly.
             runLoop->processSomeEvents();
-
-            runLoop->cleanupDeadHandlers();
         }
     }
 #endif

plugins/vst/X11RunLoop.cpp

@@ -1,33 +1,38 @@
-// SPDX-License-Identifier: GPL-3.0
+// SPDX-License-Identifier: BSD-2-Clause
+
+// This code is part of the sfizz library and is licensed under a BSD 2-clause
+// license. You should have receive a LICENSE.md file along with the code.
+// If not, contact the sfizz maintainers at https://github.com/sfztools/sfizz
 
 #if !defined(__APPLE__) && !defined(_WIN32)
 #include "X11RunLoop.h"
 #include "vstgui/lib/platform/linux/x11platform.h"
 #include "base/source/fobject.h"
+#include <vector>
+#include <typeinfo>
+#include <cstdio>
+#include <cassert>
 
 namespace VSTGUI {
 
-RunLoop::RunLoop(Steinberg::FUnknown* runLoop)
+struct RunLoop::Impl {
-    : runLoop(runLoop)
+    struct EventHandler;
-{
+    struct TimerHandler;
-}
 
-RunLoop::~RunLoop() {}
+    using EventHandlers = std::vector<Steinberg::IPtr<EventHandler>>;
+    using TimerHandlers = std::vector<Steinberg::IPtr<TimerHandler>>;
 
-SharedPointer<RunLoop> RunLoop::get()
+    EventHandlers eventHandlers;
-{
+    TimerHandlers timerHandlers;
-    return X11::RunLoop::get().cast<VSTGUI::RunLoop>();
+    Steinberg::FUnknownPtr<Steinberg::Linux::IRunLoop> runLoop;
-}
+};
 
-struct RunLoop::EventHandler final : Steinberg::Linux::IEventHandler, public Steinberg::FObject {
+//------------------------------------------------------------------------------
+struct RunLoop::Impl::EventHandler final : Steinberg::Linux::IEventHandler, public Steinberg::FObject {
     X11::IEventHandler* handler { nullptr };
     bool alive { false };
 
-    void PLUGIN_API onFDIsSet(Steinberg::Linux::FileDescriptor) override
+    void PLUGIN_API onFDIsSet(Steinberg::Linux::FileDescriptor) override;
-    {
-        if (alive && handler)
-            handler->onEvent();
-    }
 
     DELEGATE_REFCOUNT(Steinberg::FObject)
     DEFINE_INTERFACES
@@ -35,15 +40,11 @@ struct RunLoop::EventHandler final : Steinberg::Linux::IEventHandler, public Ste
     END_DEFINE_INTERFACES(Steinberg::FObject)
 };
 
-struct RunLoop::TimerHandler final : Steinberg::Linux::ITimerHandler, public Steinberg::FObject {
+struct RunLoop::Impl::TimerHandler final : Steinberg::Linux::ITimerHandler, public Steinberg::FObject {
     X11::ITimerHandler* handler { nullptr };
     bool alive { false };
 
-    void PLUGIN_API onTimer() override
+    void PLUGIN_API onTimer() override;
-    {
-        if (alive && handler)
-            handler->onTimer();
-    }
 
     DELEGATE_REFCOUNT(Steinberg::FObject)
     DEFINE_INTERFACES
@@ -51,45 +52,126 @@ struct RunLoop::TimerHandler final : Steinberg::Linux::ITimerHandler, public Ste
     END_DEFINE_INTERFACES(Steinberg::FObject)
 };
 
+//------------------------------------------------------------------------------
+void PLUGIN_API RunLoop::Impl::EventHandler::onFDIsSet(Steinberg::Linux::FileDescriptor)
+{
+    SharedPointer<RunLoop> runLoop = RunLoop::get();
+    if (!runLoop) {
+        fprintf(stderr, "[x11] event has fired without active runloop\n");
+        return;
+    }
+
+    if (alive && handler)
+        handler->onEvent();
+}
+
+void PLUGIN_API RunLoop::Impl::TimerHandler::onTimer()
+{
+    SharedPointer<RunLoop> runLoop = RunLoop::get();
+    if (!runLoop) {
+        fprintf(stderr, "[x11] timer has fired without active runloop\n");
+        return;
+    }
+
+    if (alive && handler)
+        handler->onTimer();
+}
+
+//------------------------------------------------------------------------------
+RunLoop::RunLoop(Steinberg::FUnknown* runLoop)
+    : impl(new Impl)
+{
+    impl->runLoop = runLoop;
+}
+
+RunLoop::~RunLoop()
+{
+    //dumpCurrentState();
+
+    if (0) {
+        // remove any leftover handlers
+        for (size_t i = 0; i < impl->eventHandlers.size(); ++i) {
+            const auto& eh = impl->eventHandlers[i];
+            if (eh->alive && eh->handler) {
+                impl->runLoop->unregisterEventHandler(eh.get());
+            }
+        }
+        for (size_t i = 0; i < impl->timerHandlers.size(); ++i) {
+            const auto& th = impl->timerHandlers[i];
+            if (th->alive && th->handler) {
+                impl->runLoop->unregisterTimer(th.get());
+            }
+        }
+    }
+}
+
+SharedPointer<RunLoop> RunLoop::get()
+{
+    return X11::RunLoop::get().cast<VSTGUI::RunLoop>();
+}
 
 void RunLoop::processSomeEvents()
 {
-    for (size_t i = 0; i < eventHandlers.size(); ++i) {
+    for (size_t i = 0; i < impl->eventHandlers.size(); ++i) {
-        const auto& eh = eventHandlers[i];
+        const auto& eh = impl->eventHandlers[i];
         if (eh->alive && eh->handler) {
             eh->handler->onEvent();
         }
     }
 }
 
-void RunLoop::cleanupDeadHandlers()
+void RunLoop::dumpCurrentState()
 {
-    for (size_t i = 0; i < eventHandlers.size(); ++i) {
+    fprintf(stderr, "=== X11 runloop ===\n");
-        const auto& eh = eventHandlers[i];
+
-        if (!eh->alive) {
+    fprintf(stderr, "\t" "Event slots:\n");
-            runLoop->unregisterEventHandler(eh);
+    for (size_t i = 0, n = impl->eventHandlers.size(); i < n; ++i) {
-            eventHandlers.erase(eventHandlers.begin() + i--);
+        Impl::EventHandler *eh = impl->eventHandlers[i].get();
-        }
+        fprintf(stderr, "\t\t" "(%lu) alive=%d handler=%p type=%s\n", i, eh->alive, eh->handler, (eh->alive && eh->handler) ? typeid(*eh->handler).name() : "");
     }
-    for (size_t i = 0; i < timerHandlers.size(); ++i) {
+
-        const auto& th = timerHandlers[i];
+    fprintf(stderr, "\t" "Timer slots:\n");
-        if (!th->alive) {
+    for (size_t i = 0, n = impl->timerHandlers.size(); i < n; ++i) {
-            runLoop->unregisterTimer(th);
+        Impl::TimerHandler *th = impl->timerHandlers[i].get();
-            timerHandlers.erase(timerHandlers.begin() + i--);
+        fprintf(stderr, "\t\t" "(%lu) alive=%d handler=%p type=%s\n", i, th->alive, th->handler, (th->alive && th->handler) ? typeid(*th->handler).name() : "");
-        }
     }
+
+    fprintf(stderr, "===/X11 runloop ===\n");
+}
+
+template <class T>
+static void insertHandler(std::vector<Steinberg::IPtr<T>>& list, Steinberg::IPtr<T> handler)
+{
+    size_t i = 0;
+    size_t n = list.size();
+    while (i < n && list[i]->alive)
+        ++i;
+    if (i < n)
+        list[i] = handler;
+    else
+        list.emplace_back(handler);
+}
+
+template <class T, class U>
+static size_t findHandler(const std::vector<Steinberg::IPtr<T>>& list, U* handler)
+{
+    for (size_t i = 0, n = list.size(); i < n; ++i) {
+        if (list[i]->alive && list[i]->handler == handler)
+            return i;
+    }
+    return ~size_t(0);
 }
 
 bool RunLoop::registerEventHandler(int fd, X11::IEventHandler* handler)
 {
-    if (!runLoop)
+    if (!impl->runLoop)
         return false;
 
-    auto smtgHandler = Steinberg::owned(new EventHandler());
+    auto smtgHandler = Steinberg::owned(new Impl::EventHandler);
     smtgHandler->handler = handler;
     smtgHandler->alive = true;
-    if (runLoop->registerEventHandler(smtgHandler, fd) == Steinberg::kResultTrue) {
+    if (impl->runLoop->registerEventHandler(smtgHandler, fd) == Steinberg::kResultTrue) {
-        eventHandlers.push_back(smtgHandler);
+        insertHandler(impl->eventHandlers, smtgHandler);
         return true;
     }
     return false;
@@ -97,29 +179,31 @@ bool RunLoop::registerEventHandler(int fd, X11::IEventHandler* handler)
 
 bool RunLoop::unregisterEventHandler(X11::IEventHandler* handler)
 {
-    if (!runLoop)
+    if (!impl->runLoop)
         return false;
 
-    for (size_t i = 0; i < eventHandlers.size(); ++i) {
+    size_t index = findHandler(impl->eventHandlers, handler);
-        const auto& eh = eventHandlers[i];
+    if (index == ~size_t(0))
-        if (eh->alive && eh->handler == handler) {
+        return false;
-            eh->alive = false;
+
-            return true;
+    Impl::EventHandler *eh = impl->eventHandlers[index].get();
-        }
+    if (!impl->runLoop->unregisterEventHandler(eh))
-    }
+        return false;
-    return false;
+
+    eh->alive = false;
+    return true;
 }
 
 bool RunLoop::registerTimer(uint64_t interval, X11::ITimerHandler* handler)
 {
-    if (!runLoop)
+    if (!impl->runLoop)
         return false;
 
-    auto smtgHandler = Steinberg::owned(new TimerHandler());
+    auto smtgHandler = Steinberg::owned(new Impl::TimerHandler);
     smtgHandler->handler = handler;
     smtgHandler->alive = true;
-    if (runLoop->registerTimer(smtgHandler, interval) == Steinberg::kResultTrue) {
+    if (impl->runLoop->registerTimer(smtgHandler, interval) == Steinberg::kResultTrue) {
-        timerHandlers.push_back(smtgHandler);
+        insertHandler(impl->timerHandlers, smtgHandler);
         return true;
     }
     return false;
@@ -127,17 +211,19 @@ bool RunLoop::registerTimer(uint64_t interval, X11::ITimerHandler* handler)
 
 bool RunLoop::unregisterTimer(X11::ITimerHandler* handler)
 {
-    if (!runLoop)
+    if (!impl->runLoop)
         return false;
 
-    for (size_t i = 0; i < timerHandlers.size(); ++i) {
+    size_t index = findHandler(impl->timerHandlers, handler);
-        const auto& th = timerHandlers[i];
+    if (index == ~size_t(0))
-        if (th->alive && th->handler == handler) {
+        return false;
-            th->alive = false;
+
-            return true;
+    Impl::TimerHandler *th = impl->timerHandlers[index].get();
-        }
+    if (!impl->runLoop->unregisterTimer(th))
-    }
+        return false;
-    return false;
+
+    th->alive = false;
+    return true;
 }
 
 } // namespace VSTGUI

plugins/vst/X11RunLoop.h

@@ -1,16 +1,28 @@
-// SPDX-License-Identifier: GPL-3.0
+// SPDX-License-Identifier: BSD-2-Clause
-/*
-  This is a modified version the X11 run loop from vst3editor.cpp.
 
-  This version is edited to add more safeguards to protect against host bugs.
+// This code is part of the sfizz library and is licensed under a BSD 2-clause
-  It also permits to call event processing externally in case the host has a
+// license. You should have receive a LICENSE.md file along with the code.
-  defective X11 event loop notifier.
+// If not, contact the sfizz maintainers at https://github.com/sfztools/sfizz
+
+/*
+  This runloop connects to X11 VSTGUI, it connects VST3 and VSTGUI together.
+  The Windows and macOS runloops do not need this, the OS-provided
+  functionality is used instead.
+
+  Previously, this was based on VSTGUI code provided by Steinberg.
+  This is replaced with a rewrite, because the original code has too many
+  issues. For example, it has no robustness in case handlers get added or
+  removed within the execution of the handler.
+
+  This version allows to call event processing externally, in case the host
+  has a defective X11 event loop notifier. (some versions of Bitwig do)
 */
 
 #pragma once
 #if !defined(__APPLE__) && !defined(_WIN32)
 #include "vstgui/lib/platform/linux/x11frame.h"
 #include "pluginterfaces/gui/iplugview.h"
+#include <memory>
 
 namespace VSTGUI {
 
@@ -22,24 +34,16 @@ public:
     static SharedPointer<RunLoop> get();
 
     void processSomeEvents();
-    void cleanupDeadHandlers();
+    void dumpCurrentState();
 
     // X11::IRunLoop
-    bool registerEventHandler(int fd, X11::IEventHandler* handler);
+    bool registerEventHandler(int fd, X11::IEventHandler* handler) override;
-    bool unregisterEventHandler(X11::IEventHandler* handler);
+    bool unregisterEventHandler(X11::IEventHandler* handler) override;
-    bool registerTimer(uint64_t interval, X11::ITimerHandler* handler);
+    bool registerTimer(uint64_t interval, X11::ITimerHandler* handler) override;
-    bool unregisterTimer(X11::ITimerHandler* handler);
+    bool unregisterTimer(X11::ITimerHandler* handler) override;
 
-private:
+    struct Impl;
-    struct EventHandler;
+    std::unique_ptr<Impl> impl;
-    struct TimerHandler;
-
-private:
-    using EventHandlers = std::vector<Steinberg::IPtr<EventHandler>>;
-    using TimerHandlers = std::vector<Steinberg::IPtr<TimerHandler>>;
-    EventHandlers eventHandlers;
-    TimerHandlers timerHandlers;
-    Steinberg::FUnknownPtr<Steinberg::Linux::IRunLoop> runLoop;
 };
 
 } // namespace VSTGUI