On 2015/10/17 08:03, Mike Abegg <[email protected]> wrote:
> This is a new feature that allows setting volume on a per-output basis.
> (mantis feature request reference:
> http://bugs.musicpd.org/view.php?id=4426) Original concept was to have the
> mixer for each output and the global mixer multiply their volume together.
> Looking into the source I found it would be easier to implement (and works
> just as well from a usability point of view) by having each output volume
> map directly to it's actual output and calculate the global volume as the
> average, and setting volume by trying to scale everything proportionately.
>
> The implementation does do some degree of compatibility checking as not all
> outputs can have per-output volume control (only software mixers seem to
> work) and clients can tell if a given output has a settable volume by if
> volume is reported from the outputs command. Volume for an output is set
> with a new outputvolume command. Disabled outputs can have their volumes
> set, but they do not affect and are not affected by the global volume.
>
> Attached is a patch that implements this feature. I have little experience
> in the MPD source so this probably needs more work, but it does show that
> this feature is probably feasible. I have been using this for the last few
> weeks and it seems to be working well for me, but I probably haven't covered
> all possible configurations.
Documentation missing (doc/protocol.xml).
> +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 || device < 0) {
src/command/OutputCommands.cxx:95:59: error: comparison of unsigned
expression < 0 is always false [-Werror,-Wtautological-compare]
> + 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 || volume < 0) {
src/command/OutputCommands.cxx:104:29: error: comparison of unsigned
expression < 0 is always false [-Werror,-Wtautological-compare]
> +#include <iostream>
Why that? I would like to avoid using this heavily bloated library.
> 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?
> @@ -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.
> + 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.
> + 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.
_______________________________________________
mpd-devel mailing list
[email protected]
http://mailman.blarg.de/listinfo/mpd-devel
