On 10/19/2015 05:12 AM, Max Kellermann wrote:
Documentation missing (doc/protocol.xml).
Yes it is, I apologize. I had assumed it was generated from comments
Javadocs style and didn't give it much thought. I should have been more
attentive to this.
I have now spent about an hour trying to figure out how to make this, is
this set up in the make file? What is the procedure for building this
part of the documentation? I don't see any instructions anywere and
running doxygen doesn't seem to touch the protocol documentation. I have
added appropriate documentation but I have not been able to actually
check to make sure it builds correctly.
src/command/OutputCommands.cxx:95:59: error: comparison of unsigned
expression < 0 is always false [-Werror,-Wtautological-compare]
src/command/OutputCommands.cxx:104:29: error: comparison of unsigned
expression < 0 is always false [-Werror,-Wtautological-compare]
missed that, sorry.
+#include <iostream>
Why that? I would like to avoid using this heavily bloated library.
Good question, I was debugging something and forgot to remove it.
bool success = false;
- for (auto ao : outputs)
- success = output_mixer_set_volume(*ao, volume)
- || success;
+
+ std::vector<int> volumes(outputs.size(), -1);
+
+ for (unsigned i = 0; i < outputs.size(); i++){
+ volumes[i] = output_mixer_get_volume(*outputs[i]);
+ if(volumes[i] == 0){
+ volumes[i] = 1;
I don't understand that piece: set to 1 if it's zero? Why that?
The reason for this is because if an output gets set to 0 as a result of
adjusting the master volume it will be impossible to get it out of that
state without manually setting the volume of that output specifically to
something other than 0. If you are lowering the volume this code will
result in the volume staying at 0, if you raise the volume it will
result in the volume raising by some (smallish) amount. It's something
of an edge case because it will only happen in one of two cases. 1) the
master volume is set to 0, which could alternatively be handled by a
special case check to see if all volumes are the same and if so don't
bother with the complicated rescaling nonsense. 2) when there is a large
difference between two output volumes and the master volume is lowered
to near zero, the more outputs you have the bigger a problem this is.
@@ -142,12 +206,23 @@ MultipleOutputs::SetSoftwareVolume(unsigned volume)
{
assert(volume <= PCM_VOLUME_1);
- for (auto ao : outputs) {
- const auto mixer = ao->mixer;
+ std::vector<int> volumes(outputs.size(), -1);
+
+ for (unsigned i = 0; i < outputs.size(); i++){
+ volumes[i] = output_mixer_get_volume(*outputs[i]);
+ if(volumes[i] == 0){
+ volumes[i] = 1;
+ }
+ }
Duplicate code.
yup, and it really is more a part of the volume rescaling method anyway.
Which is it's self a bit big already...
+ bool result = hardware_volume_change(outputs, volume);
+
idle_add(IDLE_MIXER);
+ idle_add(IDLE_OUTPUT); //because of per-output volume
That means each global volume change wakes up every MPD client that
watches for output changes. I wouldn't do that. IDLE_OUTPUT should
exclude volume changes; we have a dedicated idle event for that
(IDLE_MIXER) and we should keep it that way.
+bool
+audio_output_set_volume(MultipleOutputs &outputs, unsigned idx, unsigned
volume)
+{
+ if (idx >= outputs.Size())
+ return false;
+
+ AudioOutput &ao = outputs.Get(idx);
+ if (ao.mixer) {
+ bool it_worked = mixer_set_volume(ao.mixer, volume,
IgnoreError());
+ if (!it_worked) {
+ return false;
+ }
+ }
+ else {
+ return false;
+ }
+
+ idle_add(IDLE_MIXER);
+ idle_add(IDLE_OUTPUT);
Again: omit IDLE_OUTPUT here.
This I was pretty unsure about, so I'm fine with this change. I went the
way I did is because this is somewhat semantically inconsistent. Using
the mixer signal to mean that (volume) values on outputs have changed,
when we have an output event. But I totally see what you're saying. This
does have the same end result effect in that when ever the main volume
is changed the client is going to have to reload all output volumes,
it's just different events causing it. This might be a sign I'm going
about this in the wrong way.
+ ao.player_control->UpdateAudio();
Why that? Did you just copy'n'paste that line, or did you understand
what this does?
+ ++audio_output_state_version;
This is just for tracking changes for the state file, and
device-specific volume levels are not stored in the state file, so
this line is wrong.
For both of these I assumed those were involved in communicating that a
state change had occurred. I was just making an assumption though.
I have attached an updated patch incorporating your feedback. After
making these changes I'm starting to think that maybe this would be
better implemented by adding a scaling factor to each output and use
that for scaling from the master volume rather than setting the volume
of each output directly, which would be closer to the original concept.
This would fix the issues with volume changes resulting in both mixer
and output changes needing to be fetched (which is still an issue no
matter what events are generated), and would also address the issue of
volumes getting set to 0 and getting stuck in a fairly graceful manner.
Thoughts?
diff --git a/doc/protocol.xml b/doc/protocol.xml
index 5427156..e0fb4af 100644
--- a/doc/protocol.xml
+++ b/doc/protocol.xml
@@ -1982,7 +1982,7 @@ OK
</para>
<programlisting>listmounts
-mount:
+mount:
storage: /home/foo/music
mount: foo
storage: nfs://192.168.1.4/export/mp3
@@ -2270,6 +2270,20 @@ OK
</para>
</listitem>
</varlistentry>
+ <varlistentry id="command_outputvolume">
+ <term>
+ <cmdsynopsis>
+ <command>outputvolume</command>
+ <arg choice="req"><replaceable>ID</replaceable></arg>
+ <arg choice="req"><replaceable>Volume</replaceable></arg>
+ </cmdsynopsis>
+ </term>
+ <listitem>
+ <para>
+ Set the volume of an individual output in the range 0-100.
+ </para>
+ </listitem>
+ </varlistentry>
<varlistentry id="command_outputs">
<term>
<cmdsynopsis>
@@ -2284,6 +2298,7 @@ OK
outputid: 0
outputname: My ALSA Device
outputenabled: 0
+outputvolume: 0
OK
</screen>
<para>
@@ -2305,6 +2320,11 @@ OK
<varname>outputenabled</varname>: Status of the output. 0 if disabled, 1 if enabled.
</para>
</listitem>
+ <listitem>
+ <para>
+ <varname>outputvolume</varname>: Volume level in the range 0 - 100 of this output if it has it's own output. For outputs that do not have their own volume this will not be returned.
+ </para>
+ </listitem>
</itemizedlist>
</listitem>
</varlistentry>
diff --git a/src/command/AllCommands.cxx b/src/command/AllCommands.cxx
index 8e8865f..40e5e1f 100644
--- a/src/command/AllCommands.cxx
+++ b/src/command/AllCommands.cxx
@@ -134,6 +134,7 @@ static constexpr struct command commands[] = {
{ "next", PERMISSION_CONTROL, 0, 0, handle_next },
{ "notcommands", PERMISSION_NONE, 0, 0, handle_not_commands },
{ "outputs", PERMISSION_READ, 0, 0, handle_devices },
+ { "outputvolume", PERMISSION_ADMIN, 2, 2, handle_outputvolume },
{ "password", PERMISSION_NONE, 1, 1, handle_password },
{ "pause", PERMISSION_CONTROL, 0, 1, handle_pause },
{ "ping", PERMISSION_NONE, 0, 0, handle_ping },
diff --git a/src/command/OutputCommands.cxx b/src/command/OutputCommands.cxx
index 7bbe5f9..9a873ae 100644
--- a/src/command/OutputCommands.cxx
+++ b/src/command/OutputCommands.cxx
@@ -83,3 +83,33 @@ handle_devices(Client &client, gcc_unused Request args, Response &r)
printAudioDevices(r, client.partition.outputs);
return CommandResult::OK;
}
+
+CommandResult
+handle_outputvolume(Client &client, Request args, Response &r)
+{
+ assert(args.size == 2);
+ unsigned device;
+ if (!args.Parse(0, device, r))
+ return CommandResult::ERROR;
+
+ if (device > client.partition.outputs.Size()-1) {
+ r.Error(ACK_ERROR_NO_EXIST, "No such audio output");
+ return CommandResult::ERROR;
+ }
+
+ unsigned volume;
+ if (!args.Parse(1, volume, r))
+ return CommandResult::ERROR;
+
+ if (volume > 100) {
+ r.Error(ACK_ERROR_NO_EXIST, "volume must be in the range 0 - 100");
+ return CommandResult::ERROR;
+ }
+
+ if (!audio_output_set_volume(client.partition.outputs, device, volume)) {
+ r.Error(ACK_ERROR_NO_EXIST, "could not set volume on the output");
+ return CommandResult::ERROR;
+ }
+
+ return CommandResult::OK;
+}
diff --git a/src/command/OutputCommands.hxx b/src/command/OutputCommands.hxx
index 3dd81bc..0673e59 100644
--- a/src/command/OutputCommands.hxx
+++ b/src/command/OutputCommands.hxx
@@ -38,4 +38,7 @@ handle_toggleoutput(Client &client, Request request, Response &response);
CommandResult
handle_devices(Client &client, Request request, Response &response);
+CommandResult
+handle_outputvolume(Client &client, Request request, Response &response);
+
#endif
diff --git a/src/mixer/MixerAll.cxx b/src/mixer/MixerAll.cxx
index 835cf4e..b3119df 100644
--- a/src/mixer/MixerAll.cxx
+++ b/src/mixer/MixerAll.cxx
@@ -49,6 +49,97 @@ output_mixer_get_volume(const AudioOutput &ao)
return volume;
}
+/**
+ * get the volumes of all outputs in a format useful for get_rescaled_volumes
+ * returns -1 for outputs that cannot have their volume changed,
+ * volumes == 0 are set to 1 so that they will not be stuck at 0
+ */
+static std::vector<int>
+get_output_volumes(const std::vector<AudioOutput *> &outputs){
+ std::vector<int> volumes(outputs.size(), -1);
+
+ for (unsigned i = 0; i < outputs.size(); i++){
+ volumes[i] = output_mixer_get_volume(*outputs[i]);
+ if(volumes[i] == 0){
+ //this allows volumes set to 0 to get out of 0 via the main volume
+ volumes[i] = 1;
+ }
+ }
+
+ return volumes;
+}
+
+/**
+ *
+ */
+static unsigned
+rescale_volume(unsigned new_average, unsigned current_average, unsigned max, int volume)
+{
+ unsigned new_volume = new_average;
+
+ //try to maintain proportion
+ if(current_average != 0 && volume > 0){
+ new_volume = volume * new_average / current_average;
+ if(new_volume > max){
+ new_volume = max;
+ }
+ if(! (new_volume > 0) ){ //catching error conditions
+ new_volume = 0;
+ }
+ }
+
+ return new_volume;
+}
+
+/**
+ * get a set of volumes based on the current set of volumes
+ * set all of the volumes such that their average is the given average
+ * maintaining their relative proportions as much as possible while
+ * respecting the constraint that all volumes must stay withing 0 to 100
+ */
+static std::vector<int>
+get_rescaled_volumes(const std::vector<AudioOutput *> &outputs, unsigned max, unsigned new_average)
+{
+ std::vector<int> volumes = get_output_volumes(outputs);
+ unsigned average = 0;
+
+ //calculate the current average and coun't how many valid (can have their volume set) outputs there are
+ unsigned valid_count = 0;
+ for(unsigned i = 0; i<volumes.size(); i++){
+ if(volumes[i] > 0){
+ average += volumes[i];
+ valid_count++;
+ }
+ }
+ if(valid_count < 1){
+ //all volumes are non-settable
+ return volumes;
+ }
+ average /= valid_count;
+
+ unsigned last_average = 0;
+
+ //keep trying to scale the volumes until they match the desired volume, quit if it ever looks like we aren't making a difference anymore.
+ while(new_average != average && last_average != average){
+ last_average = average;
+ average = 0;
+
+ for(unsigned i = 0; i<volumes.size(); i++){
+ if(volumes[i] < 0){
+ //invalid volume, bail
+ continue;
+ }
+
+ volumes[i] = rescale_volume(new_average, last_average, max, volumes[i]);
+
+ average += volumes[i];
+ }
+ average /= valid_count;
+ }
+
+ return volumes;
+}
+
int
MultipleOutputs::GetVolume() const
{
@@ -97,9 +188,12 @@ MultipleOutputs::SetVolume(unsigned volume)
assert(volume <= 100);
bool success = false;
- for (auto ao : outputs)
- success = output_mixer_set_volume(*ao, volume)
- || success;
+
+ std::vector<int> volumes = get_rescaled_volumes(outputs, 100, volume);
+
+ for (unsigned i = 0; i < outputs.size(); i++){
+ success = output_mixer_set_volume(*outputs[i], volumes[i]) || success;
+ }
return success;
}
@@ -142,12 +236,14 @@ MultipleOutputs::SetSoftwareVolume(unsigned volume)
{
assert(volume <= PCM_VOLUME_1);
- for (auto ao : outputs) {
- const auto mixer = ao->mixer;
+ std::vector<int> volumes = get_rescaled_volumes(outputs, PCM_VOLUME_1, volume);
+
+ for (unsigned i = 0; i < outputs.size(); i++){
+ const auto mixer = outputs[i]->mixer;
if (mixer != nullptr &&
(&mixer->plugin == &software_mixer_plugin ||
&mixer->plugin == &null_mixer_plugin))
- mixer_set_volume(mixer, volume, IgnoreError());
+ mixer_set_volume(mixer, volumes[i], IgnoreError());
}
}
diff --git a/src/mixer/Volume.cxx b/src/mixer/Volume.cxx
index 8bc8d87..140da56 100644
--- a/src/mixer/Volume.cxx
+++ b/src/mixer/Volume.cxx
@@ -87,9 +87,11 @@ volume_level_change(MultipleOutputs &outputs, unsigned volume)
volume_software_set = volume;
+ bool result = hardware_volume_change(outputs, volume);
+
idle_add(IDLE_MIXER);
- return hardware_volume_change(outputs, volume);
+ return result;
}
bool
diff --git a/src/output/OutputCommand.cxx b/src/output/OutputCommand.cxx
index 83abcf2..3eabae0 100644
--- a/src/output/OutputCommand.cxx
+++ b/src/output/OutputCommand.cxx
@@ -31,6 +31,8 @@
#include "player/Control.hxx"
#include "mixer/MixerControl.hxx"
#include "Idle.hxx"
+#include "mixer/MixerInternal.hxx"
+#include "util/Error.hxx"
extern unsigned audio_output_state_version;
@@ -104,3 +106,25 @@ audio_output_toggle_index(MultipleOutputs &outputs, unsigned idx)
return true;
}
+
+bool
+audio_output_set_volume(MultipleOutputs &outputs, unsigned idx, unsigned volume)
+{
+ if (idx >= outputs.Size())
+ return false;
+
+ AudioOutput &ao = outputs.Get(idx);
+ if (ao.mixer) {
+ bool it_worked = mixer_set_volume(ao.mixer, volume, IgnoreError());
+ if (!it_worked) {
+ return false;
+ }
+ }
+ else {
+ return false;
+ }
+
+ idle_add(IDLE_MIXER);
+
+ return true;
+}
diff --git a/src/output/OutputCommand.hxx b/src/output/OutputCommand.hxx
index 5b53cd1..e0918b2 100644
--- a/src/output/OutputCommand.hxx
+++ b/src/output/OutputCommand.hxx
@@ -50,4 +50,11 @@ audio_output_disable_index(MultipleOutputs &outputs, unsigned idx);
bool
audio_output_toggle_index(MultipleOutputs &outputs, unsigned idx);
+/**
+ * Sets the volume of an audio output. Returns false if the specified output
+ * does not exist, or if it's volume cannot be set.
+ */
+bool
+audio_output_set_volume(MultipleOutputs &outputs, unsigned idx, unsigned volume);
+
#endif
diff --git a/src/output/OutputPrint.cxx b/src/output/OutputPrint.cxx
index d2ddbbf..26916f9 100644
--- a/src/output/OutputPrint.cxx
+++ b/src/output/OutputPrint.cxx
@@ -27,6 +27,8 @@
#include "MultipleOutputs.hxx"
#include "Internal.hxx"
#include "client/Response.hxx"
+#include "mixer/MixerInternal.hxx"
+#include "util/Error.hxx"
void
printAudioDevices(Response &r, const MultipleOutputs &outputs)
@@ -38,5 +40,15 @@ printAudioDevices(Response &r, const MultipleOutputs &outputs)
"outputname: %s\n"
"outputenabled: %i\n",
i, ao.name, ao.enabled);
+
+ //if they have independent volume report it
+ if(ao.mixer)
+ {
+ int volume = ao.mixer->GetVolume(IgnoreError());
+ if(volume > -1) //-1 indicates an error state
+ {
+ r.Format("outputvolume: %i\n",volume);
+ }
+ }
}
}
_______________________________________________
mpd-devel mailing list
mpd-devel@musicpd.org
http://mailman.blarg.de/listinfo/mpd-devel
