[pulseaudio-tickets] [Bug 97777] Clipping with per-stream volume above 100% even though device volume below 100%

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=97777

--- Comment #6 from Tanu Kaskinen <tanuk at iki.fi> ---
(In reply to Niklas Haas from comment #5)
> > module-match can almost do what you want. The only problem is that it
> > overrides volumes set by module-stream-restore. See [1] for documentation.
> > Adding a new configuration option to change the hook callback priority
> > should be pretty trivial (the module uses a hook to get notified of new
> > streams, and the callback priorities decide the ordering between
> > module-match and module-stream-restore).
> 
> Indeed it does, and it does work well for me, apart from the part where it
> overrides stream restoring. On the subject of making it configurable, it
> feels sort of “weird” to have this filter expose its hook priority as a user
> configurable variable, and using the filter also seem like a bit of abuse.

Yes, exposing the hook priority to users is awkward, but I couldn't think of
better ways to configure the ordering between module-match and
module-stream-restore.

> If we're touching source code, wouldn't it be better to add an option to
> module-stream-restore indicating what volume to pick for streams that it
> fails observing?

That sounds quite good.

> Or perhaps adding a new `module-default-volume` that runs
> after everything else and just sets a configurable volume if unset.

That's certainly a possibility, but having a separate module for such a small
bit of functionality doesn't feel very nice.

Another possibility would be to add an option for default stream volume to
daemon.conf.

Currently I like the idea of adding an option to module-stream-restore the
most.

> That being said, module-match being capable of running before
> module-stream-restore definitely seems like a useful feature to have
> regardless, because I might want to set different default volumes based on
> the title (but still retain the ability to make changes of my own).
> 
> P.s. I wonder if it would be possible to get module-match to detect changes
> to the match table in realtime, although for that I suppose I could use
> inotify + unload-module && load-module.

My preferred method of realtime configuration changes is to use pactl or other
client (the infrastructure for communicating between clients and individual
modules sucks currently, though). That is, no manual editing of configuration
files. That said, reloading configuration files could still be useful for
distribution maintainers, if for example installing some package causes
configuration changes, so I don't think I'd oppose a patch that implements
configuration reloading.

I'm a bit concerned that automatic reloading could cause trouble when updating
pulseaudio, if the update brings new configuration that is not supported by the
currently-running daemon process. Requiring manual action ("pactl
reload-configuration" or SIGHUP) might be a good idea.

> Anyway, how do you propose exposing this to the user? I noticed while digging
> through the source that there's a pa_sink_input_new_data->volume_is_set bool,
> maybe the right way to approach this would be to add a ‘force’ option to
> module_stream_restore which forcibly applies saved volumes even if
> volume_is_set was already true by the time the fixate hook got called?

I don't think that would be much better than having a hook priority option in
module-match, because module-stream-restore can't reliably force anything that
happens in hook callbacks after module-stream-restore's own callback.
Explaining the semantics accurately to users would be as problematic as
explaining the hook priorities. Adding a default volume option to
module-stream-restore seems like a better solution.

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/pulseaudio-bugs/attachments/20160914/8bf6c903/attachment.html>