From f7e5fb61b14980fba93557ccaa1ad2c4c98bae1f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 7 Nov 2017 17:18:04 +0100 Subject: [PATCH 1/6] Remove unused wxSoundSyncOnlyAdaptor::m_playing variable It seems to have been replaced by wxSoundPlaybackStatus::m_playing a long time ago but was still kept, resulting in confusion and always returning false from IsPlaying() as it tested a wrong variable. Fix this by removing this one completely and always using the other one everywhere. --- src/unix/sound.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index 342a58f7e4..394c786d66 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -294,7 +294,7 @@ class wxSoundSyncOnlyAdaptor : public wxSoundBackend { public: wxSoundSyncOnlyAdaptor(wxSoundBackend *backend) - : m_backend(backend), m_playing(false) {} + : m_backend(backend) {} virtual ~wxSoundSyncOnlyAdaptor() { delete m_backend; @@ -324,7 +324,6 @@ private: friend class wxSoundAsyncPlaybackThread; wxSoundBackend *m_backend; - bool m_playing; #if wxUSE_THREADS // player thread holds this mutex and releases it after it finishes // playing, so that the main thread knows when it can play sound @@ -341,7 +340,7 @@ wxThread::ExitCode wxSoundAsyncPlaybackThread::Entry() &m_adapt->m_status); m_data->DecRef(); - m_adapt->m_playing = false; + m_adapt->m_status.m_playing = false; m_adapt->m_mutexRightToPlay.Unlock(); wxLogTrace(wxT("sound"), wxT("terminated async playback thread")); return 0; From 6ebc56939fde4607dd68990687e28968af4cfdb1 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 7 Nov 2017 17:19:22 +0100 Subject: [PATCH 2/6] Acquire mutex before modifying variable in wxSoundSyncOnlyAdaptor The mutex must be locked to avoid data races with the thread actually playing the sound. --- src/unix/sound.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index 394c786d66..20646f2d7e 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -386,14 +386,11 @@ void wxSoundSyncOnlyAdaptor::Stop() wxLogTrace(wxT("sound"), wxT("asking audio to stop")); #if wxUSE_THREADS + m_mutexRightToPlay.Lock(); + // tell the player thread (if running) to stop playback ASAP: m_status.m_stopRequested = true; - // acquire the mutex to be sure no sound is being played, then - // release it because we don't need it for anything (the effect of this - // is that calling thread will wait until playback thread reacts to - // our request to interrupt playback): - m_mutexRightToPlay.Lock(); m_mutexRightToPlay.Unlock(); wxLogTrace(wxT("sound"), wxT("audio was stopped")); #endif From 6c1a557a0f75549081c6e8702356987fa523bbda Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 7 Nov 2017 17:21:23 +0100 Subject: [PATCH 3/6] Lock and unlock mutex in both wxSound threads using it The code was completely broken as it locked the mutex in only one thread and then tried to unlock it in another one, which made no sense, didn't protect against anything and resulted in errors and assert failures. Fix this by locking and unlocking the mutex in both threads before accessing shared data or playing sound. Closes #17990. --- src/unix/sound.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index 20646f2d7e..3a7c684a7a 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -336,6 +336,7 @@ private: #if wxUSE_THREADS wxThread::ExitCode wxSoundAsyncPlaybackThread::Entry() { + m_adapt->m_mutexRightToPlay.Lock(); m_adapt->m_backend->Play(m_data, m_flags & ~wxSOUND_ASYNC, &m_adapt->m_status); @@ -361,6 +362,7 @@ bool wxSoundSyncOnlyAdaptor::Play(wxSoundData *data, unsigned flags, wxThread *th = new wxSoundAsyncPlaybackThread(this, data, flags); th->Create(); th->Run(); + m_mutexRightToPlay.Unlock(); wxLogTrace(wxT("sound"), wxT("launched async playback thread")); return true; #else From d788588cfc3694663eab20fe7a48459514ea5902 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 7 Nov 2017 17:23:09 +0100 Subject: [PATCH 4/6] Use wxMutexLocker instead of manual Lock/Unlock() calls This is safer and also makes the code simpler and shorter. --- src/unix/sound.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index 3a7c684a7a..ccecdc7e0d 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -336,13 +336,14 @@ private: #if wxUSE_THREADS wxThread::ExitCode wxSoundAsyncPlaybackThread::Entry() { - m_adapt->m_mutexRightToPlay.Lock(); + wxMutexLocker locker(m_adapt->m_mutexRightToPlay); + m_adapt->m_backend->Play(m_data, m_flags & ~wxSOUND_ASYNC, &m_adapt->m_status); m_data->DecRef(); m_adapt->m_status.m_playing = false; - m_adapt->m_mutexRightToPlay.Unlock(); + wxLogTrace(wxT("sound"), wxT("terminated async playback thread")); return 0; } @@ -355,14 +356,13 @@ bool wxSoundSyncOnlyAdaptor::Play(wxSoundData *data, unsigned flags, if (flags & wxSOUND_ASYNC) { #if wxUSE_THREADS - m_mutexRightToPlay.Lock(); + wxMutexLocker locker(m_mutexRightToPlay); m_status.m_playing = true; m_status.m_stopRequested = false; data->IncRef(); wxThread *th = new wxSoundAsyncPlaybackThread(this, data, flags); th->Create(); th->Run(); - m_mutexRightToPlay.Unlock(); wxLogTrace(wxT("sound"), wxT("launched async playback thread")); return true; #else @@ -373,13 +373,9 @@ bool wxSoundSyncOnlyAdaptor::Play(wxSoundData *data, unsigned flags, else { #if wxUSE_THREADS - m_mutexRightToPlay.Lock(); + wxMutexLocker locker(m_mutexRightToPlay); #endif - bool rv = m_backend->Play(data, flags, status); -#if wxUSE_THREADS - m_mutexRightToPlay.Unlock(); -#endif - return rv; + return m_backend->Play(data, flags, status); } } @@ -388,12 +384,11 @@ void wxSoundSyncOnlyAdaptor::Stop() wxLogTrace(wxT("sound"), wxT("asking audio to stop")); #if wxUSE_THREADS - m_mutexRightToPlay.Lock(); + wxMutexLocker lock(m_mutexRightToPlay); // tell the player thread (if running) to stop playback ASAP: m_status.m_stopRequested = true; - m_mutexRightToPlay.Unlock(); wxLogTrace(wxT("sound"), wxT("audio was stopped")); #endif } From 2558c91ae8440c4264f3f7e2a2c023755b8906be Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 19 Nov 2017 22:44:57 +0100 Subject: [PATCH 5/6] Don't fail to okay all mono WAVs in OSS wxSound backend Ever since the changes of 544c4a3bdea365dbefd72bea3c0c4568cf03845a (almost 14 years ago), playing mono WAVs with wxSound completely failed if setting the sound device to mono using SNDCTL_DSP_STEREO ioctl failed. This doesn't look like a wise thing to do, so don't consider this as a fatal failure, but just play mono as stereo (and even possibly stereo as mono) instead. This fixes the sound sample being broken out of the box on many (all?) Linux systems. --- src/unix/sound.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index ccecdc7e0d..ae38289009 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -224,7 +224,6 @@ bool wxSoundBackendOSS::InitDSP(int dev, const wxSoundData *data) if (tmp != stereo) { wxLogTrace(wxT("sound"), wxT("Unable to set DSP to %s."), stereo? wxT("stereo"):wxT("mono")); - m_needConversion = true; } tmp = data->m_samplingRate; From 35902735034335271ddbaf7fb45bce3601f7946a Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 19 Nov 2017 22:51:51 +0100 Subject: [PATCH 6/6] Document that osspd requirement for using OSS under Linux Without SDL, wxSound doesn't work out of the box on most (all?) contemporary Linux systems, so at least document this. See #14899. --- interface/wx/sound.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/interface/wx/sound.h b/interface/wx/sound.h index 0fdcb9d9d1..69756cc91d 100644 --- a/interface/wx/sound.h +++ b/interface/wx/sound.h @@ -17,8 +17,11 @@ This class represents a short sound (loaded from Windows WAV file), that can be stored in memory and played. - Currently this class is implemented on Windows and Unix (and uses either - Open Sound System or Simple DirectMedia Layer). + Currently this class is implemented on Windows and Unix and can use either + Open Sound System (OSS) or Simple DirectMedia Layer (SDL) under the latter. + Notice that OSS is not provided any more by most, and maybe even all, + Linux systems in 2017 and osspd (OSS Proxy Daemon) package typically needs + to be installed to make it work. @library{wxadv} @category{media}