mixer/Volume: refactor to class MixerMemento, per partition

Eliminate global variables, convert them to MixerMemento fields.

Closes https://github.com/MusicPlayerDaemon/MPD/issues/1583

NEWS

@@ -5,6 +5,7 @@ ver 0.23.9 (not yet released)
   - ffmpeg: support FFmpeg 5.1
 * output
   - pipewire: set app icon
+* fix bogus volume levels with multiple partitions
 * improve iconv detection
 * macOS: fix macOS 10 build problem (0.23.8 regression)
 

meson.build

@@ -352,7 +352,7 @@ sources = [
   'src/TagStream.cxx',
   'src/TagAny.cxx',
   'src/TimePrint.cxx',
-  'src/mixer/Volume.cxx',
+  'src/mixer/Memento.cxx',
   'src/PlaylistFile.cxx',
 ]
 

src/Partition.cxx

@@ -23,7 +23,6 @@
 #include "Log.hxx"
 #include "lib/fmt/ExceptionFormatter.hxx"
 #include "song/DetachedSong.hxx"
-#include "mixer/Volume.hxx"
 #include "IdleFlags.hxx"
 #include "client/Listener.hxx"
 #include "client/Client.hxx"
@@ -206,7 +205,7 @@ Partition::OnBorderPause() noexcept
 void
 Partition::OnMixerVolumeChanged(Mixer &, int) noexcept
 {
-	InvalidateHardwareVolume();
+	mixer_memento.InvalidateHardwareVolume();
 
 	/* notify clients */
 	EmitIdle(IDLE_MIXER);

src/Partition.hxx

@@ -25,6 +25,7 @@
 #include "queue/Listener.hxx"
 #include "output/MultipleOutputs.hxx"
 #include "mixer/Listener.hxx"
+#include "mixer/Memento.hxx"
 #include "player/Control.hxx"
 #include "player/Listener.hxx"
 #include "protocol/RangeArg.hxx"
@@ -76,6 +77,8 @@ struct Partition final : QueueListener, PlayerListener, MixerListener {
 
 	MultipleOutputs outputs;
 
+	MixerMemento mixer_memento;
+
 	PlayerControl pc;
 
 	ReplayGainMode replay_gain_mode = ReplayGainMode::OFF;

src/StateFile.cxx

@@ -27,7 +27,6 @@
 #include "storage/StorageState.hxx"
 #include "Partition.hxx"
 #include "Instance.hxx"
-#include "mixer/Volume.hxx"
 #include "SongLoader.hxx"
 #include "util/Domain.hxx"
 #include "Log.hxx"
@@ -47,7 +46,7 @@ StateFile::StateFile(StateFileConfig &&_config,
 void
 StateFile::RememberVersions() noexcept
 {
-	prev_volume_version = sw_volume_state_get_hash();
+	prev_volume_version = partition.mixer_memento.GetSoftwareVolumeStateHash();
 	prev_output_version = audio_output_state_get_version();
 	prev_playlist_version = playlist_state_get_hash(partition.playlist,
 							partition.pc);
@@ -59,7 +58,7 @@ StateFile::RememberVersions() noexcept
 bool
 StateFile::IsModified() const noexcept
 {
-	return prev_volume_version != sw_volume_state_get_hash() ||
+	return prev_volume_version != partition.mixer_memento.GetSoftwareVolumeStateHash() ||
 		prev_output_version != audio_output_state_get_version() ||
 		prev_playlist_version != playlist_state_get_hash(partition.playlist,
 								 partition.pc)
@@ -72,7 +71,7 @@ StateFile::IsModified() const noexcept
 inline void
 StateFile::Write(BufferedOutputStream &os)
 {
-	save_sw_volume_state(os);
+	partition.mixer_memento.SaveSoftwareVolumeState(os);
 	audio_output_state_save(os, partition.outputs);
 
 #ifdef ENABLE_DATABASE
@@ -125,7 +124,7 @@ try {
 
 	const char *line;
 	while ((line = file.ReadLine()) != nullptr) {
-		success = read_sw_volume_state(line, partition.outputs) ||
+		success = partition.mixer_memento.LoadSoftwareVolumeState(line, partition.outputs) ||
 			audio_output_state_read(line, partition.outputs) ||
 			playlist_state_restore(config, line, file, song_loader,
 					       partition.playlist,

src/command/OtherCommands.cxx

@@ -33,7 +33,6 @@
 #include "TimePrint.hxx"
 #include "decoder/DecoderPrint.hxx"
 #include "ls.hxx"
-#include "mixer/Volume.hxx"
 #include "time/ChronoUtil.hxx"
 #include "util/UriUtil.hxx"
 #include "util/StringAPI.hxx"
@@ -325,7 +324,7 @@ handle_getvol(Client &client, Request, Response &r)
 {
 	auto &partition = client.GetPartition();
 
-	const auto volume = volume_level_get(partition.outputs);
+	const auto volume = partition.mixer_memento.GetVolume(partition.outputs);
 	if (volume >= 0)
 		r.Fmt(FMT_STRING("volume: {}\n"), volume);
 
@@ -337,7 +336,8 @@ handle_setvol(Client &client, Request args, Response &)
 {
 	unsigned level = args.ParseUnsigned(0, 100);
 
-	volume_level_change(client.GetPartition().outputs, level);
+	auto &partition = client.GetPartition();
+	partition.mixer_memento.SetVolume(partition.outputs, level);
 	return CommandResult::OK;
 }
 
@@ -346,9 +346,11 @@ handle_volume(Client &client, Request args, Response &r)
 {
 	int relative = args.ParseInt(0, -100, 100);
 
-	auto &outputs = client.GetPartition().outputs;
+	auto &partition = client.GetPartition();
+	auto &outputs = partition.outputs;
+	auto &mixer_memento = partition.mixer_memento;
 
-	const int old_volume = volume_level_get(outputs);
+	const int old_volume = mixer_memento.GetVolume(outputs);
 	if (old_volume < 0) {
 		r.Error(ACK_ERROR_SYSTEM, "No mixer");
 		return CommandResult::ERROR;
@@ -361,7 +363,7 @@ handle_volume(Client &client, Request args, Response &r)
 		new_volume = 100;
 
 	if (new_volume != old_volume)
-		volume_level_change(outputs, new_volume);
+		mixer_memento.SetVolume(outputs, new_volume);
 
 	return CommandResult::OK;
 }

src/command/OutputCommands.cxx

@@ -33,7 +33,11 @@ handle_enableoutput(Client &client, Request args, Response &r)
 	assert(args.size == 1);
 	unsigned device = args.ParseUnsigned(0);
 
-	if (!audio_output_enable_index(client.GetPartition().outputs, device)) {
+	auto &partition = client.GetPartition();
+
+	if (!audio_output_enable_index(partition.outputs,
+				       partition.mixer_memento,
+				       device)) {
 		r.Error(ACK_ERROR_NO_EXIST, "No such audio output");
 		return CommandResult::ERROR;
 	}
@@ -47,7 +51,11 @@ handle_disableoutput(Client &client, Request args, Response &r)
 	assert(args.size == 1);
 	unsigned device = args.ParseUnsigned(0);
 
-	if (!audio_output_disable_index(client.GetPartition().outputs, device)) {
+	auto &partition = client.GetPartition();
+
+	if (!audio_output_disable_index(partition.outputs,
+					partition.mixer_memento,
+					device)) {
 		r.Error(ACK_ERROR_NO_EXIST, "No such audio output");
 		return CommandResult::ERROR;
 	}
@@ -61,7 +69,11 @@ handle_toggleoutput(Client &client, Request args, Response &r)
 	assert(args.size == 1);
 	unsigned device = args.ParseUnsigned(0);
 
-	if (!audio_output_toggle_index(client.GetPartition().outputs, device)) {
+	auto &partition = client.GetPartition();
+
+	if (!audio_output_toggle_index(partition.outputs,
+					partition.mixer_memento,
+				       device)) {
 		r.Error(ACK_ERROR_NO_EXIST, "No such audio output");
 		return CommandResult::ERROR;
 	}

src/command/PlayerCommands.cxx

@@ -25,7 +25,6 @@
 #include "SingleMode.hxx"
 #include "client/Client.hxx"
 #include "client/Response.hxx"
-#include "mixer/Volume.hxx"
 #include "Partition.hxx"
 #include "Instance.hxx"
 #include "IdleFlags.hxx"
@@ -131,7 +130,7 @@ handle_status(Client &client, [[maybe_unused]] Request args, Response &r)
 
 	const auto &playlist = partition.playlist;
 
-	const auto volume = volume_level_get(partition.outputs);
+	const auto volume = partition.mixer_memento.GetVolume(partition.outputs);
 	if (volume >= 0)
 		r.Fmt(FMT_STRING("volume: {}\n"), volume);
 

src/mixer/Memento.cxx

@@ -17,11 +17,10 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  */
 
-#include "Volume.hxx"
+#include "Memento.hxx"
 #include "output/MultipleOutputs.hxx"
 #include "Idle.hxx"
 #include "util/StringCompare.hxx"
-#include "system/PeriodClock.hxx"
 #include "io/BufferedOutputStream.hxx"
 
 #include <cassert>
@@ -30,22 +29,8 @@
 
 #define SW_VOLUME_STATE                         "sw_volume: "
 
-static unsigned volume_software_set = 100;
-
-/** the cached hardware mixer value; invalid if negative */
-static int last_hardware_volume = -1;
-/** the age of #last_hardware_volume */
-static PeriodClock hardware_volume_clock;
-
-void
-InvalidateHardwareVolume() noexcept
-{
-	/* flush the hardware volume cache */
-	last_hardware_volume = -1;
-}
-
 int
-volume_level_get(const MultipleOutputs &outputs) noexcept
+MixerMemento::GetVolume(const MultipleOutputs &outputs) noexcept
 {
 	if (last_hardware_volume >= 0 &&
 	    !hardware_volume_clock.CheckUpdate(std::chrono::seconds(1)))
@@ -56,8 +41,8 @@ volume_level_get(const MultipleOutputs &outputs) noexcept
 	return last_hardware_volume;
 }
 
-static bool
+inline bool
-software_volume_change(MultipleOutputs &outputs, unsigned volume)
+MixerMemento::SetSoftwareVolume(MultipleOutputs &outputs, unsigned volume)
 {
 	assert(volume <= 100);
 
@@ -67,8 +52,8 @@ software_volume_change(MultipleOutputs &outputs, unsigned volume)
 	return true;
 }
 
-static void
+inline void
-hardware_volume_change(MultipleOutputs &outputs, unsigned volume)
+MixerMemento::SetHardwareVolume(MultipleOutputs &outputs, unsigned volume)
 {
 	/* reset the cache */
 	last_hardware_volume = -1;
@@ -77,7 +62,7 @@ hardware_volume_change(MultipleOutputs &outputs, unsigned volume)
 }
 
 void
-volume_level_change(MultipleOutputs &outputs, unsigned volume)
+MixerMemento::SetVolume(MultipleOutputs &outputs, unsigned volume)
 {
 	assert(volume <= 100);
 
@@ -85,11 +70,11 @@ volume_level_change(MultipleOutputs &outputs, unsigned volume)
 
 	idle_add(IDLE_MIXER);
 
-	hardware_volume_change(outputs, volume);
+	SetHardwareVolume(outputs, volume);
 }
 
 bool
-read_sw_volume_state(const char *line, MultipleOutputs &outputs)
+MixerMemento::LoadSoftwareVolumeState(const char *line, MultipleOutputs &outputs)
 {
 	char *end = nullptr;
 	long int sv;
@@ -100,19 +85,13 @@ read_sw_volume_state(const char *line, MultipleOutputs &outputs)
 
 	sv = strtol(line, &end, 10);
 	if (*end == 0 && sv >= 0 && sv <= 100)
-		software_volume_change(outputs, sv);
+		SetSoftwareVolume(outputs, sv);
 
 	return true;
 }
 
 void
-save_sw_volume_state(BufferedOutputStream &os)
+MixerMemento::SaveSoftwareVolumeState(BufferedOutputStream &os) const
 {
 	os.Format(SW_VOLUME_STATE "%u\n", volume_software_set);
 }
-
-unsigned
-sw_volume_state_get_hash() noexcept
-{
-	return volume_software_set;
-}

src/mixer/Memento.hxx

@@ -0,0 +1,73 @@
+/*
+ * Copyright 2003-2021 The Music Player Daemon Project
+ * http://www.musicpd.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#pragma once
+
+#include "system/PeriodClock.hxx"
+
+class MultipleOutputs;
+class BufferedOutputStream;
+
+/**
+ * Cache for hardware/software volume levels.
+ */
+class MixerMemento {
+	unsigned volume_software_set = 100;
+
+	/** the cached hardware mixer value; invalid if negative */
+	int last_hardware_volume = -1;
+
+	/** the age of #last_hardware_volume */
+	PeriodClock hardware_volume_clock;
+
+public:
+	/**
+	 * Flush the hardware volume cache.
+	 */
+	void InvalidateHardwareVolume() noexcept {
+		last_hardware_volume = -1;
+	}
+
+	[[gnu::pure]]
+	int GetVolume(const MultipleOutputs &outputs) noexcept;
+
+	/**
+	 * Throws on error.
+	 */
+	void SetVolume(MultipleOutputs &outputs, unsigned volume);
+
+	bool LoadSoftwareVolumeState(const char *line, MultipleOutputs &outputs);
+
+	void SaveSoftwareVolumeState(BufferedOutputStream &os) const;
+
+	/**
+	 * Generates a hash number for the current state of the software
+	 * volume control.  This is used by timer_save_state_file() to
+	 * determine whether the state has changed and the state file should
+	 * be saved.
+	 */
+	[[gnu::pure]]
+	unsigned GetSoftwareVolumeStateHash() const noexcept {
+		return volume_software_set;
+	}
+
+private:
+	bool SetSoftwareVolume(MultipleOutputs &outputs, unsigned volume);
+	void SetHardwareVolume(MultipleOutputs &outputs, unsigned volume);
+};

src/mixer/Volume.hxx

@@ -1,55 +0,0 @@
-/*
- * Copyright 2003-2021 The Music Player Daemon Project
- * http://www.musicpd.org
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- */
-
-#ifndef MPD_VOLUME_HXX
-#define MPD_VOLUME_HXX
-
-class MultipleOutputs;
-class BufferedOutputStream;
-
-void
-InvalidateHardwareVolume() noexcept;
-
-[[gnu::pure]]
-int
-volume_level_get(const MultipleOutputs &outputs) noexcept;
-
-/**
- * Throws on error.
- */
-void
-volume_level_change(MultipleOutputs &outputs, unsigned volume);
-
-bool
-read_sw_volume_state(const char *line, MultipleOutputs &outputs);
-
-void
-save_sw_volume_state(BufferedOutputStream &os);
-
-/**
- * Generates a hash number for the current state of the software
- * volume control.  This is used by timer_save_state_file() to
- * determine whether the state has changed and the state file should
- * be saved.
- */
-[[gnu::pure]]
-unsigned
-sw_volume_state_get_hash() noexcept;
-
-#endif

src/output/OutputCommand.cxx

@@ -28,13 +28,15 @@
 #include "MultipleOutputs.hxx"
 #include "Client.hxx"
 #include "mixer/MixerControl.hxx"
-#include "mixer/Volume.hxx"
+#include "mixer/Memento.hxx"
 #include "Idle.hxx"
 
 extern unsigned audio_output_state_version;
 
 bool
-audio_output_enable_index(MultipleOutputs &outputs, unsigned idx)
+audio_output_enable_index(MultipleOutputs &outputs,
+			  MixerMemento &mixer_memento,
+			  unsigned idx)
 {
 	if (idx >= outputs.Size())
 		return false;
@@ -46,7 +48,7 @@ audio_output_enable_index(MultipleOutputs &outputs, unsigned idx)
 	idle_add(IDLE_OUTPUT);
 
 	if (ao.GetMixer() != nullptr) {
-		InvalidateHardwareVolume();
+		mixer_memento.InvalidateHardwareVolume();
 		idle_add(IDLE_MIXER);
 	}
 
@@ -58,7 +60,9 @@ audio_output_enable_index(MultipleOutputs &outputs, unsigned idx)
 }
 
 bool
-audio_output_disable_index(MultipleOutputs &outputs, unsigned idx)
+audio_output_disable_index(MultipleOutputs &outputs,
+			   MixerMemento &mixer_memento,
+			   unsigned idx)
 {
 	if (idx >= outputs.Size())
 		return false;
@@ -72,7 +76,7 @@ audio_output_disable_index(MultipleOutputs &outputs, unsigned idx)
 	auto *mixer = ao.GetMixer();
 	if (mixer != nullptr) {
 		mixer_close(mixer);
-		InvalidateHardwareVolume();
+		mixer_memento.InvalidateHardwareVolume();
 		idle_add(IDLE_MIXER);
 	}
 
@@ -84,7 +88,9 @@ audio_output_disable_index(MultipleOutputs &outputs, unsigned idx)
 }
 
 bool
-audio_output_toggle_index(MultipleOutputs &outputs, unsigned idx)
+audio_output_toggle_index(MultipleOutputs &outputs,
+			  MixerMemento &mixer_memento,
+			  unsigned idx)
 {
 	if (idx >= outputs.Size())
 		return false;
@@ -97,7 +103,7 @@ audio_output_toggle_index(MultipleOutputs &outputs, unsigned idx)
 		auto *mixer = ao.GetMixer();
 		if (mixer != nullptr) {
 			mixer_close(mixer);
-			InvalidateHardwareVolume();
+			mixer_memento.InvalidateHardwareVolume();
 			idle_add(IDLE_MIXER);
 		}
 	}

src/output/OutputCommand.hxx

@@ -28,26 +28,33 @@
 #define MPD_OUTPUT_COMMAND_HXX
 
 class MultipleOutputs;
+class MixerMemento;
 
 /**
  * Enables an audio output.  Returns false if the specified output
  * does not exist.
  */
 bool
-audio_output_enable_index(MultipleOutputs &outputs, unsigned idx);
+audio_output_enable_index(MultipleOutputs &outputs,
+			  MixerMemento &mixer_memento,
+			  unsigned idx);
 
 /**
  * Disables an audio output.  Returns false if the specified output
  * does not exist.
  */
 bool
-audio_output_disable_index(MultipleOutputs &outputs, unsigned idx);
+audio_output_disable_index(MultipleOutputs &outputs,
+			   MixerMemento &mixer_memento,
+			   unsigned idx);
 
 /**
  * Toggles an audio output.  Returns false if the specified output
  * does not exist.
  */
 bool
-audio_output_toggle_index(MultipleOutputs &outputs, unsigned idx);
+audio_output_toggle_index(MultipleOutputs &outputs,
+			  MixerMemento &mixer_memento,
+			  unsigned idx);
 
 #endif