Stream the volume catcher a little:

- Use LL_DEBUGS() for potential debug output.
- Enclose mutex locking in their own scope, to make unlocking automatic and also limit the life time of a lock to as short as possible
- Introduce mCleanupMutex to replace std::unique_lock pwLock(*this). I'm baffled using lock as a mutex like that did even compile.
- Remove virtual inheritance, as it is not needed here.

4 files changed, 86 insertions, 115 deletions

diff --git a/indra/media_plugins/cef/linux/volume_catcher_linux.cpp b/indra/media_plugins/cef/linux/volume_catcher_linux.cpp
index 3c037b45ba..439c0a6768 100644
--- a/indra/media_plugins/cef/linux/volume_catcher_linux.cpp
+++ b/indra/media_plugins/cef/linux/volume_catcher_linux.cpp
@@ -32,28 +32,29 @@
 
 VolumeCatcher::VolumeCatcher()
 {
-	// only init once we receive which implementation to use
-
-	// debugClear();
-	// debugPrint("init volume catcher\n");
 }
 
-void VolumeCatcher::onEnablePipeWireVolumeCatcher(bool enable) {
+void VolumeCatcher::onEnablePipeWireVolumeCatcher(bool enable)
+{
 	if (pimpl != nullptr)
 		return;
 
-	if (enable) {
-		// debugPrint("volume catcher using pipewire\n");
+	if (enable)
+    {
+		LL_DEBUGS() << "volume catcher using pipewire" << LL_ENDL;
 		pimpl = new VolumeCatcherPipeWire();
-	} else {
-		// debugPrint("volume catcher using pulseaudio\n");
+	}
+    else
+    {
+		LL_DEBUGS() << "volume catcher using pulseaudio" << LL_ENDL;
 		pimpl = new VolumeCatcherPulseAudio();
 	}
 }
 
 VolumeCatcher::~VolumeCatcher()
 {
-	if (pimpl != nullptr) {
+	if (pimpl != nullptr)
+    {
 		delete pimpl;
 		pimpl = nullptr;
 	}
@@ -68,15 +69,12 @@ void VolumeCatcher::setVolume(F32 volume)
 
 void VolumeCatcher::setPan(F32 pan)
 {
-	if (pimpl != nullptr) {
+	if (pimpl != nullptr)
 		pimpl->setPan(pan);
-	}
 }
 
 void VolumeCatcher::pump()
 {
-	if (pimpl != nullptr) {
+	if (pimpl != nullptr)
 		pimpl->pump();
-	}
 }
-
diff --git a/indra/media_plugins/cef/linux/volume_catcher_linux.h b/indra/media_plugins/cef/linux/volume_catcher_linux.h
index 0f11c537b1..ff00d0672e 100644
--- a/indra/media_plugins/cef/linux/volume_catcher_linux.h
+++ b/indra/media_plugins/cef/linux/volume_catcher_linux.h
@@ -49,7 +49,7 @@ extern "C" {
 
 #include "media_plugin_base.h"
 
-class VolumeCatcherPulseAudio : public virtual VolumeCatcherImpl
+class VolumeCatcherPulseAudio : public VolumeCatcherImpl
 {
 public:
 	VolumeCatcherPulseAudio();
@@ -78,7 +78,7 @@ public:
 	bool mGotSyms;
 };
 
-class VolumeCatcherPipeWire : public virtual VolumeCatcherImpl
+class VolumeCatcherPipeWire : public VolumeCatcherImpl
 {
 public:
 	VolumeCatcherPipeWire();
@@ -121,31 +121,15 @@ public:
 	F32 mVolume = 1.0f; // max by default
 	// F32 mPan = 0.0f; // center
 
-	pw_thread_loop* mThreadLoop;
-	pw_context* mContext;
-	pw_core* mCore;
-	pw_registry* mRegistry;
+	pw_thread_loop* mThreadLoop = nullptr;
+	pw_context* mContext = nullptr;
+	pw_core* mCore = nullptr;
+	pw_registry* mRegistry = nullptr;
 	spa_hook mRegistryListener;
 
 	std::unordered_set<ChildNode*> mChildNodes;
 	std::mutex mChildNodesMutex;
+    std::mutex mCleanupMutex;
 };
 
-// static void debugClear()
-// {
-// 	auto file = fopen("volume-catcher-log.txt", "w");
-// 	fprintf(file, "\n");
-// 	fclose(file);
-// }
-
-// static void debugPrint(const char* format, ...)
-// {
-//     va_list args;
-//     va_start(args, format);
-// 	auto file = fopen("volume-catcher-log.txt", "a");
-//     vfprintf(file, format, args);
-// 	fclose(file);
-//     va_end(args);
-// }
-
 #endif // VOLUME_CATCHER_LINUX_H
diff --git a/indra/media_plugins/cef/linux/volume_catcher_pipewire.cpp b/indra/media_plugins/cef/linux/volume_catcher_pipewire.cpp
index ca1808d63c..027fdb77f6 100755
--- a/indra/media_plugins/cef/linux/volume_catcher_pipewire.cpp
+++ b/indra/media_plugins/cef/linux/volume_catcher_pipewire.cpp
@@ -62,8 +62,7 @@ VolumeCatcherPipeWire::~VolumeCatcherPipeWire()
 
 static void registryEventGlobal(
 	void *data, uint32_t id, uint32_t permissions, const char *type,
-	uint32_t version, const struct spa_dict *props
-)
+	uint32_t version, const struct spa_dict *props)
 {
 	static_cast<VolumeCatcherPipeWire*>(data)->handleRegistryEventGlobal(
 		id, permissions, type, version, props
@@ -82,14 +81,14 @@ bool VolumeCatcherPipeWire::loadsyms(std::string pw_dso_name)
 
 void VolumeCatcherPipeWire::init()
 {
-	// debugPrint("init\n");
-	
+    LL_DEBUGS() << "init" << LL_ENDL;
+
 	mGotSyms = loadsyms("libpipewire-0.3.so.0");
 	
 	if (!mGotSyms)
 		return;
 
-	// debugPrint("got syms\n");
+    LL_DEBUGS() << "successfully got symbols" << LL_ENDL;
 
 	llpw_init(NULL, NULL);
 
@@ -115,7 +114,7 @@ void VolumeCatcherPipeWire::init()
 
 	mRegistry = pw_core_get_registry(mCore, PW_VERSION_REGISTRY, 0);
 
-	// debugPrint("got registry\n");
+    LL_DEBUGS() << "pw_core_get_registry: " <<  (mRegistry?"success":"nullptr") << LL_ENDL;
 
 	spa_zero(mRegistryListener);
 
@@ -125,27 +124,31 @@ void VolumeCatcherPipeWire::init()
 
 	llpw_thread_loop_start(mThreadLoop);
 
-	// debugPrint("started thread loop\n");
+    LL_DEBUGS() << "thread loop started" << LL_ENDL;
 }
 
 void VolumeCatcherPipeWire::cleanup()
 {
-	std::unique_lock childNodesLock(mChildNodesMutex);
-	for (auto* childNode : mChildNodes) {
-		childNode->destroy();
-	}
-	mChildNodes.clear();
-	childNodesLock.unlock();
-
-	std::unique_lock pwLock(*this);
-	if (mRegistry)
-		llpw_proxy_destroy((struct pw_proxy*)mRegistry);
-	spa_zero(mRegistryListener);
-	if (mCore)
-		llpw_core_disconnect(mCore);
-	if (mContext)
-		llpw_context_destroy(mContext);
-	pwLock.unlock();
+    {
+        std::unique_lock childNodesLock(mChildNodesMutex);
+        for (auto *childNode: mChildNodes)
+            childNode->destroy();
+
+        mChildNodes.clear();
+    }
+
+    {
+        std::unique_lock pwLock(mCleanupMutex);
+        if (mRegistry)
+            llpw_proxy_destroy((struct pw_proxy *) mRegistry);
+
+        spa_zero(mRegistryListener);
+
+        if (mCore)
+            llpw_core_disconnect(mCore);
+        if (mContext)
+            llpw_context_destroy(mContext);
+    }
 
 	if (!mThreadLoop)
 		return;
@@ -153,49 +156,33 @@ void VolumeCatcherPipeWire::cleanup()
 	llpw_thread_loop_stop(mThreadLoop);
 	llpw_thread_loop_destroy(mThreadLoop);
 
-	// debugPrint("cleanup done\n");
+    LL_DEBUGS() << "cleanup done" << LL_ENDL;
 }
 
-void VolumeCatcherPipeWire::lock() {
+void VolumeCatcherPipeWire::lock()
+{
 	if (!mThreadLoop)
 		return;
 
 	llpw_thread_loop_lock(mThreadLoop);
 }
 
-void VolumeCatcherPipeWire::unlock() {
+void VolumeCatcherPipeWire::unlock()
+{
 	if (!mThreadLoop)
 		return;
 
 	llpw_thread_loop_unlock(mThreadLoop);
 }
 
-// #define INV_LERP(a, b, v) (v - a) / (b - a)
-
-// #include <sys/time.h>
-
 void VolumeCatcherPipeWire::ChildNode::updateVolume()
 {
 	if (!mActive)
 		return;
 
 	F32 volume = std::clamp(mImpl->mVolume, 0.0f, 1.0f);
-	// F32 pan = std::clamp(mImpl->mPan, -1.0f, 1.0f);
-
-	// debugClear();
-	// struct timeval time;
-	// gettimeofday(&time, NULL);
-	// double t = (double)time.tv_sec + (double)(time.tv_usec / 1000) / 1000;
-	// debugPrint("time is %f\n", t);
-	// F32 pan = std::sin(t * 2.0d);
-	// debugPrint("pan is %f\n", pan);
-
-	// uint32_t channels = 2;
-	// float volumes[channels];
-	// volumes[1] = INV_LERP(-1.0f, 0.0f, pan) * volume; // left
-	// volumes[0] = INV_LERP(1.0f, 0.0f, pan) * volume; // right
-
-	uint32_t channels = 1;
+
+	const uint32_t channels = 1;
 	float volumes[channels];
 	volumes[0] = volume;
 
@@ -210,9 +197,10 @@ void VolumeCatcherPipeWire::ChildNode::updateVolume()
 	spa_pod_builder_array(&builder, sizeof(float), SPA_TYPE_Float, channels, volumes);
 	spa_pod* pod = static_cast<spa_pod*>(spa_pod_builder_pop(&builder, &frame));
 
-	std::lock_guard pwLock(*mImpl);
-
-	pw_node_set_param(mProxy, SPA_PARAM_Props, 0, pod);
+    {
+        std::lock_guard pwLock(*mImpl);
+        pw_node_set_param(mProxy, SPA_PARAM_Props, 0, pod);
+    }
 }
 
 void VolumeCatcherPipeWire::ChildNode::destroy()
@@ -222,16 +210,18 @@ void VolumeCatcherPipeWire::ChildNode::destroy()
 
 	mActive = false;
 
-	std::unique_lock childNodesLock(mImpl->mChildNodesMutex);
-	mImpl->mChildNodes.erase(this);
-	childNodesLock.unlock();
-	
+    {
+        std::unique_lock childNodesLock(mImpl->mChildNodesMutex);
+        mImpl->mChildNodes.erase(this);
+    }
+
 	spa_hook_remove(&mNodeListener);
 	spa_hook_remove(&mProxyListener);
 
-	std::lock_guard pwLock(*mImpl);
-
-	llpw_proxy_destroy(mProxy);
+    {
+        std::lock_guard pwLock(*mImpl);
+        llpw_proxy_destroy(mProxy);
+    }
 }
 
 static void nodeEventInfo(void* data, const struct pw_node_info* info)
@@ -246,17 +236,18 @@ static void nodeEventInfo(void* data, const struct pw_node_info* info)
 	if (!isPluginPid(pid))
 		return;
 
-	// const char* appName = spa_dict_lookup(info->props, PW_KEY_APP_NAME);
-	// debugPrint("got app: %s\n", appName);
+    const char* appName = spa_dict_lookup(info->props, PW_KEY_APP_NAME);
+    LL_DEBUGS() << "got app: " << appName << LL_ENDL;
 	
 	auto* const childNode = static_cast<VolumeCatcherPipeWire::ChildNode*>(data);
-	// debugPrint("init volume to: %f\n", childNode->mImpl->mVolume);
+    LL_DEBUGS() << "init volume: " << childNode->mImpl->mVolume  << LL_ENDL;
 
 	childNode->updateVolume();
 
-	std::lock_guard childNodesLock(childNode->mImpl->mChildNodesMutex);
-
-	childNode->mImpl->mChildNodes.insert(childNode);
+    {
+        std::lock_guard childNodesLock(childNode->mImpl->mChildNodesMutex);
+        childNode->mImpl->mChildNodes.insert(childNode);
+    }
 }
 
 static const struct pw_node_events NODE_EVENTS = {
@@ -284,9 +275,9 @@ static const struct pw_proxy_events PROXY_EVENTS = {
 
 void VolumeCatcherPipeWire::handleRegistryEventGlobal(
 	uint32_t id, uint32_t permissions, const char *type, uint32_t version,
-	const struct spa_dict *props
-) {
-	if (props == nullptr || strcmp(type, PW_TYPE_INTERFACE_Node) != 0)
+	const struct spa_dict *props)
+{
+	if (props == nullptr  || type == nullptr || strcmp(type, PW_TYPE_INTERFACE_Node) != 0)
 		return;
 
 	const char* mediaClass = spa_dict_lookup(props, PW_KEY_MEDIA_CLASS);
@@ -310,27 +301,25 @@ void VolumeCatcherPipeWire::handleRegistryEventGlobal(
 
 void VolumeCatcherPipeWire::setVolume(F32 volume)
 {
-	// debugPrint("setting all volume to: %f\n", volume);
+    LL_DEBUGS() << "setting volume to: " << volume << LL_ENDL;
 
 	mVolume = volume;
 
-	std::unique_lock childNodeslock(mChildNodesMutex);
-	std::unordered_set<ChildNode*> copyOfChildNodes(mChildNodes);
-	childNodeslock.unlock();
+    {
+        std::unique_lock childNodeslock(mChildNodesMutex);
+        std::unordered_set<ChildNode *> copyOfChildNodes(mChildNodes);
 
-	// debugPrint("for %d nodes\n", copyOfChildNodes.size());
+        LL_DEBUGS() << "found " << copyOfChildNodes.size() << " child nodes" << LL_ENDL;
 
-	for (auto* childNode : copyOfChildNodes) {
-		childNode->updateVolume();
-	}
+        for (auto* childNode : copyOfChildNodes)
+            childNode->updateVolume();
+    }
 }
 
 void VolumeCatcherPipeWire::setPan(F32 pan)
 {
-	// mPan = pan;
-	// setVolume(mVolume);
 }
 
 void VolumeCatcherPipeWire::pump()
 {
-}
\ No newline at end of file
+}
diff --git a/indra/media_plugins/cef/volume_catcher.h b/indra/media_plugins/cef/volume_catcher.h
index e11ecf5881..3e53a7e961 100644
--- a/indra/media_plugins/cef/volume_catcher.h
+++ b/indra/media_plugins/cef/volume_catcher.h
@@ -43,7 +43,7 @@ public:
 	virtual void pump() = 0; // call this at least a few times a second if you can - it affects how quickly we can 'catch' a new audio source and adjust its volume
 };
 
-class VolumeCatcher : public virtual VolumeCatcherImpl
+class VolumeCatcher : public VolumeCatcherImpl
 {
 public:
 	VolumeCatcher();