Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent lmms freezing when a vst hangs #4790

Closed
wants to merge 3 commits into from
Closed

Prevent lmms freezing when a vst hangs #4790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
511527b
Prevent lmms freezing when a vst hangs
Jan 20, 2019
dadadc8
FINE! I'll reorder that variable! grumble grumble.. :)
Jan 24, 2019
b079ecb
reverted remoteplugin install (done in pr #4797)
Feb 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,17 @@ ENDIF(WANT_DEBUG_FPE)
# check for libsamplerate
FIND_PACKAGE(Samplerate 0.1.8 MODULE REQUIRED)

#check for presence of mutex
SET(CMAKE_REQUIRED_FLAGS "-std=c++11")

CHECK_CXX_SOURCE_COMPILES("
#include <mutex>
int main(int argc, const char* argv[]) {
std::mutex m;
return 0;
}
" HAS_STD_MUTEX)

# set compiler flags
IF(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
SET(WERROR_FLAGS "-Wall -Werror=unused-function -Wno-sign-compare -Wno-strict-overflow")
Expand Down
68 changes: 50 additions & 18 deletions include/RemotePlugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
#include <QtCore/QSystemSemaphore>
#endif


#ifdef LMMS_HAVE_SYS_SHM_H
#include <sys/shm.h>

Expand Down Expand Up @@ -93,6 +92,13 @@ typedef int32_t key_t;
#include <QtCore/QProcess>
#include <QtCore/QThread>

#ifdef USE_MINGW_THREADS_REPLACEMENT
# include <mingw.mutex.h>
#else
# include <mutex>
# include <chrono>
#endif

#ifndef SYNC_WITH_SHM_FIFO
#include <poll.h>
#include <unistd.h>
Expand Down Expand Up @@ -649,7 +655,7 @@ class LMMS_EXPORT RemotePluginBase
}
#endif

inline void invalidate()
virtual void invalidate()
{
#ifdef SYNC_WITH_SHM_FIFO
m_in->invalidate();
Expand Down Expand Up @@ -805,37 +811,56 @@ class LMMS_EXPORT RemotePlugin : public QObject, public RemotePluginBase

void updateSampleRate( sample_rate_t _sr )
{
lock();
sendMessage( message( IdSampleRateInformation ).addInt( _sr ) );
waitForMessage( IdInformationUpdated, true );
unlock();
if (try_lock())
{
sendMessage(message(IdSampleRateInformation).addInt(_sr));
waitForMessage(IdInformationUpdated, true);
unlock();
}
}


virtual void toggleUI()
{
lock();
sendMessage( IdToggleUI );
unlock();
if (try_lock())
{
sendMessage(IdToggleUI);
unlock();
}
}

int isUIVisible()
{
lock();
sendMessage( IdIsUIVisible );
unlock();
message m = waitForMessage( IdIsUIVisible );
return m.id != IdIsUIVisible ? -1 : m.getInt() ? 1 : 0;
if (try_lock()) {
sendMessage(IdIsUIVisible);
unlock();
message m = waitForMessage(IdIsUIVisible);
return m.id != IdIsUIVisible ? -1 : m.getInt() ? 1 : 0;
}

return 0;
}

inline bool failed() const
{
return m_failed;
}

inline void lock()
inline bool try_lock()
{
m_commMutex.lock();
if (!m_disconnected)
{
if (!m_commMutex.try_lock_for(std::chrono::milliseconds(1000)))
{
m_failed = true;
m_disconnected = true;
invalidate();
return false;
}
return true;
}
else
return false;
}

inline void unlock()
Expand All @@ -848,6 +873,13 @@ public slots:
virtual void hideUI();

protected:
virtual void invalidate() override
{
m_watcher.quit();
m_process.kill();
RemotePluginBase::invalidate();
}

inline void setSplittedChannels( bool _on )
{
m_splitChannels = _on;
Expand All @@ -857,12 +889,12 @@ public slots:
bool m_failed;
private:
void resizeSharedProcessingMemory();

bool m_disconnected;

QProcess m_process;
ProcessWatcher m_watcher;

QMutex m_commMutex;
std::recursive_timed_mutex m_commMutex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason you changed to std::recursive_timed_mutex? I think we can keep using QMutex, avoiding the MinGW compatibility problems.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to avoid QT at as much as possible when it's not related to the UI. In the longer term it would, hopefully, make it easier to separate lmms into two parts: a library and what it is now.

If it's not possible to not use QMutex for this, I'll of course change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand wanting to avoid Qt for core functionality. I'm just not very happy about the MinGW mutex support detection & workaround and would like to avoid it being copy&pasted around the CMake scripts.

What do you think about reverting this PR to QMutex for now until (a) we find a cleaner solution or (b) we drop support for old MinGW versions? An example of how (a) could look like can be seen in 57ca681 which is part of #4443. In this commit, I added include/mingw-std-threads/mutex which shadows the system mutex using include_directories(BEFORE include/mingw-std-threads) when it's found necessary. If done correctly, this could fix MinGW thread support for all targets in the project without the need of #ifdef USE_MINGW_THREADS_REPLACEMENT checks at every include.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give me a couple of days to think this over... I've deleted my previous msg because I responded to quickly; there may be other options.

Are we using mingw for any other reason than to cross-compile to windows?

This pr won't be merged any time soon, so we have all the time we need to find a solution that's clean and acceptable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justnope Don't worry about using QtCore functionality inside our core, this is completely fine and we'll probably always do it. It doesn't create any coupling between core and UI. The only reason I can see to use std mutex here is re-usability outside of LMMS. However, If you ever need to use this code outside of LMMS, you can simply swap QMutex for std::recursive_timed_mutex, their APIs are compatible.

Are we using mingw for any other reason than to cross-compile to windows?

We use winegcc for RemoteVstPlugin, and we support MSYS2 on Windows which uses MinGW.

bool m_splitChannels;
#ifdef USE_QT_SHMEM
QSharedMemory m_shmObj;
Expand Down
7 changes: 6 additions & 1 deletion plugins/VstEffect/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ ENDIF()

BUILD_PLUGIN(vsteffect VstEffect.cpp VstEffectControls.cpp VstEffectControlDialog.cpp VstSubPluginFeatures.cpp VstEffect.h VstEffectControls.h VstEffectControlDialog.h VstSubPluginFeatures.h MOCFILES VstEffectControlDialog.h VstEffectControls.h EMBEDDED_RESOURCES *.png)
TARGET_LINK_LIBRARIES(vsteffect vstbase)

IF(NOT HAS_STD_MUTEX)
target_include_directories(vsteffect PRIVATE
"${PROJECT_SOURCE_DIR}/src/3rdparty/mingw-std-threads")
target_compile_definitions(vsteffect PRIVATE
-DUSE_MINGW_THREADS_REPLACEMENT)
ENDIF()
ENDIF(LMMS_SUPPORT_VST)

7 changes: 7 additions & 0 deletions plugins/vestige/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,12 @@ IF(LMMS_SUPPORT_VST)
ENDIF()
BUILD_PLUGIN(vestige vestige.cpp vestige.h MOCFILES vestige.h EMBEDDED_RESOURCES "${CMAKE_CURRENT_SOURCE_DIR}/*.png")
TARGET_LINK_LIBRARIES(vestige vstbase)

IF(NOT HAS_STD_MUTEX)
target_include_directories(vestige PRIVATE
"${PROJECT_SOURCE_DIR}/src/3rdparty/mingw-std-threads")
target_compile_definitions(vestige PRIVATE
-DUSE_MINGW_THREADS_REPLACEMENT)
ENDIF()
ENDIF(LMMS_SUPPORT_VST)

Loading