[PATCH] Add pulseaudio configuration and fix volume bugs

Done

Details

3 participants

Leo Prikler
Ludovic Courtès
Marius Bakke

Owner: unassigned

Submitted by: Leo Prikler

Severity: normal

Merged with

Leo Prikler wrote on 9 Jan 2020 14:57

Recipients:(address . guix-patches@gnu.org)

Message-ID:20200109135746.1993-1-leo.prikler@student.tugraz.at

This series of patches adds a configuration type for pulseaudio and also fixes
a bug, where various applications would inadvertently max out the system volume
(see e.g. #38172).

The second patch of this series differs from the one proposed there, in that
it uses environment variables to achieve its goal rather than rebuilding
pulseaudio.  As a result, pulseaudio-service will very likely ignore user
configuration in $XDG_CONFIG_DIR/pulse unless those variables are unset.
Doing so in ~/.profile or an equivalent to it should suffice.

Regards,
Leo

Marius Bakke wrote on 9 Jan 2020 22:50

Recipients:

Message-ID:87blrc1epy.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (4 lines)

> This series of patches adds a configuration type for pulseaudio and also fixes

> a bug, where various applications would inadvertently max out the system volume

> (see e.g. #38172).

Derp, I did not notice you had re-rolled the patches already :-)

Please ignore the /etc/pulse related questions, this looks much better.

Toggle quote (6 lines)

> The second patch of this series differs from the one proposed there, in that

> it uses environment variables to achieve its goal rather than rebuilding

> pulseaudio. As a result, pulseaudio-service will very likely ignore user

> configuration in $XDG_CONFIG_DIR/pulse unless those variables are unset.

> Doing so in ~/.profile or an equivalent to it should suffice.

Sounds reasonable.  Users who wish to control pulseaudio outside of the
configuration system can just exclude the service, or unset PULSE_CONFIG
and PULSE_CLIENT_CONFIG in their profile.

Leo Prikler wrote on 9 Jan 2020 23:49

Re: bug#38172: WebkitGTK-based browsers: System volume suddenly maxed out when playing audio or video

Recipients:

Message-ID:dce4781f6aed35b80d0a2e566ffb2a676fa29917.camel@student.tugraz.at

Am Donnerstag, den 09.01.2020, 21:48 +0100 schrieb Marius Bakke:

Toggle quote (7 lines)> 
> I have a preference for making this field empty initially to have a
> 1:1
> compatibility with the current PA client and daemon configuration
> (i.e. nothing).  Then a follow-up patch can add this new
> configuration,
> perhaps with an explaining comment.

Fair enough. This would mean I'd have to split 0001 into two, but

okay.

Toggle quote (3 lines)

> Does it make sense to have default-script and system-script default

> to

> (file-append pulseaudio "...") and avoid the conditional altogether?

The idea behind it was to have the script itself in the code rather

than asking users to construct a mixed-text-file, but I'm fine either

way.

Toggle quote (9 lines)> This means pulseaudio will start looking in /etc/pulse for
> configuration
> files on foreign distributions too, right?
> 
> I wonder if there is better way to give it configuration
> files.  Perhaps
> by patching the D-Bus service files?  Not a blocker for this series,
> but
> something to consider in case /etc/pulse causes trouble.

This is already addressed by the renewed series I sent to guix-patches.

I know you already found that, but I'd like to repeat it for those who

thus far have only read this thread.

Toggle quote (1 lines)

> End on #t.

As above, but thanks for the hint, I missed the warning it seems.

Toggle quote (19 lines)> [...]
> 
> > From e24016f9a44a113847dd937ac47ab4bdb960236d Mon Sep 17 00:00:00
> > 2001
> > From: Leo Prikler <leo.prikler@student.tugraz.at>
> > Date: Thu, 9 Jan 2020 01:29:13 +0100
> > Subject: [PATCH 3/3] services: Add pulseaudio to %desktop-services.
> > 
> > * gnu/services/desktop.scm (%desktop-services): Add pulseaudio
> > service.
> 
> This will pull in "swh-plugins" which was the original intention
> behind
> pulseaudio-service before this patch series.  Before adding it to
> %desktop-services, I would prefer if the pulseaudio environment
> configuration could be made modular, so that there are no
> configuration
> differences for end users, i.e. they'd have to actively enable the
> LADSPA plugin.

I think adding a field ladspa-plugins, which accepts a list of packages
and adds their "lib/ladspa" would be the right approach here, but I
also feel, that this perhaps deserves its own service unrelated to
pulseaudio.  WDYT?
Either way, I agree on the "having to actively enable" part. 

Toggle quote (1 lines)

> As a final note, can you also update doc/guix.texi accordingly?

I will once I've figured out how to best handle these fields.

Regards,

Leo

Leo Prikler wrote on 10 Jan 2020 02:48

[PATCH 1/6] services: Add pulseaudio-configuration.

Recipients:(address . guix-patches@gnu.org)(address . 39053@debbugs.gnu.org)

Message-ID:20200110014823.10595-1-leo.prikler@student.tugraz.at

* gnu/services/sound (<pulseaudio-configuration>): New record.
(pulseaudio-etc): New procedure.
(pulseaudio-service-type): Update accordingly.
---
 gnu/services/sound.scm | 43 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

Toggle diff (74 lines)diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index aaca733729..307e62fd1b 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -34,6 +34,7 @@
   #:export (alsa-configuration
             alsa-service-type
 
+            pulseaudio-configuration
             pulseaudio-service-type))
 
 ;;; Commentary:
@@ -106,19 +107,57 @@ ctl.!default {
 ;;; PulseAudio
 ;;;
 
+(define-record-type* <pulseaudio-configuration>
+  pulseaudio-configuration make-pulseaudio-configuration
+  pulseaudio-configuration?
+  (client-conf pulseaudio-client-conf
+               (default '()))
+  (daemon-conf pulseaudio-daemon-conf
+               (default '()))
+  (script-file pulseaudio-script-file
+               (default (file-append pulseaudio "/etc/pulse/default.pa")))
+  (system-script-file pulseaudio-system-script-file
+                      (default
+                        (file-append pulseaudio "/etc/pulse/system.pa"))))
+
 (define (pulseaudio-environment config)
   ;; Define this variable in the global environment such that
   ;; pulseaudio swh-plugins works.
   `(("LADSPA_PATH"
      . ,(file-append swh-plugins "/lib/ladspa"))))
 
+(define (pulseaudio-conf-entry arg)
+  (match arg
+    ((key . value)
+     (format #f "~a = ~s~%" key value))
+    ((? string? _)
+     (string-append arg "\n"))))
+
+(define pulseaudio-etc
+  (match-lambda
+    (($ <pulseaudio-configuration> client-conf daemon-conf
+                                   default-script-file system-script-file)
+     `(("pulse"
+        ,(file-union
+          "pulse"
+          `(("client.conf"
+             ,(apply mixed-text-file "client.conf"
+                     (map pulseaudio-conf-entry client-conf)))
+            ("daemon.conf"
+             ,(apply mixed-text-file "daemon.conf"
+                     "default-script-file = " default-script-file "\n"
+                     (map pulseaudio-conf-entry daemon-conf)))
+            ("default.pa" ,default-script-file)
+            ("system.pa" ,system-script-file))))))))
+
 (define pulseaudio-service-type
   (service-type
    (name 'pulseaudio)
    (extensions
     (list (service-extension session-environment-service-type
-                             pulseaudio-environment)))
-   (default-value #f)
+                             pulseaudio-environment)
+          (service-extension etc-service-type pulseaudio-etc)))
+   (default-value (pulseaudio-configuration))
    (description "Configure PulseAudio sound support.")))
 
 ;;; sound.scm ends here
-- 
2.24.1

Leo Prikler wrote on 10 Jan 2020 02:48

[PATCH 2/6] services: pulseaudio-service-type: Honor /etc.

Recipients:(address . guix-patches@gnu.org)(address . 39053@debbugs.gnu.org)

Message-ID:20200110014823.10595-2-leo.prikler@student.tugraz.at

* gnu/services/sound (pulseaudio-environment): Add PULSE_CONFIG and
PULSE_CLIENTCONFIG.
---
 gnu/services/sound.scm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Toggle diff (19 lines)diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index 307e62fd1b..6b2b345a44 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -123,8 +123,10 @@ ctl.!default {
 (define (pulseaudio-environment config)
   ;; Define this variable in the global environment such that
   ;; pulseaudio swh-plugins works.
-  `(("LADSPA_PATH"
-     . ,(file-append swh-plugins "/lib/ladspa"))))
+  `(("LADSPA_PATH" . ,(file-append swh-plugins "/lib/ladspa"))
+    ;; Define these variables, so that pulseaudio honors /etc.
+    ("PULSE_CONFIG" . "/etc/pulse/daemon.conf")
+    ("PULSE_CLIENTCONFIG" . "/etc/pulse/client.conf")))
 
 (define (pulseaudio-conf-entry arg)
   (match arg
-- 
2.24.1

Leo Prikler wrote on 10 Jan 2020 02:48

[PATCH 3/6] services: pulseaudio-configuration: Set flat-volumes to no.

Recipients:(address . guix-patches@gnu.org)(address . 39053@debbugs.gnu.org)

Message-ID:20200110014823.10595-3-leo.prikler@student.tugraz.at

* gnu/services/sound (pulseaudio-configuration) [daemon-conf]:
Disable flat-volumes.  Enabling flat-volumes, as is the upstream default,
causes unpleasant experiences to users when applications inadvertently max
out the system volume (e.g. as in #38172).
---
 gnu/services/sound.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Toggle diff (15 lines)diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index 6b2b345a44..5d72b3ada2 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -113,7 +113,7 @@ ctl.!default {
   (client-conf pulseaudio-client-conf
                (default '()))
   (daemon-conf pulseaudio-daemon-conf
-               (default '()))
+               (default '((flat-volumes . no))))
   (script-file pulseaudio-script-file
                (default (file-append pulseaudio "/etc/pulse/default.pa")))
   (system-script-file pulseaudio-system-script-file
-- 
2.24.1

Leo Prikler wrote on 10 Jan 2020 02:48

[PATCH 4/6] services: Split ladspa-service-type from pulseaudio-service-type.

Recipients:(address . guix-patches@gnu.org)(address . 39053@debbugs.gnu.org)

Message-ID:20200110014823.10595-4-leo.prikler@student.tugraz.at

* gnu/services/sound.scm (ladspa-configuration): New record.
(ladspa-environment): New procedure.
(ladspa-service-type): New service type.
(pulseaudio-environment): Remove LADSPA_PATH.
* doc/guix.texi: Adjust documentation.
---
 doc/guix.texi          | 20 +++++++++++---------
 gnu/services/sound.scm | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 10 deletions(-)

Toggle diff (86 lines)diff --git a/doc/guix.texi b/doc/guix.texi
index 60491af4d4..431895aa7d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15951,19 +15951,21 @@ pcm.!default @{
 See @uref{https://www.alsa-project.org/main/index.php/Asoundrc} for the
 details.
 
-@deffn {Scheme Variable} pulseaudio-service-type
-This is the type for the @uref{http://www.pulseaudio.org/, PulseAudio
-sound server}, which sets the @var{LADSPA_PATH} environment variable to
-allow PulseAudio load modules from @code{swh-plugins} package.
+@deffn {Scheme Variable} ladspa-service-type
+This service sets the @var{LADSPA_PATH} variable, so that programs, which
+respect it, e.g. PulseAudio, can load LADSPA plugins.
 
-See @uref{http://plugin.org.uk/ladspa-swh/docs/ladspa-swh.html} for the
-details.
-
-The following example will setup the service:
+The following example will setup the service to enable modules from the
+@code{swh-plugins} package:
 
 @lisp
-(service pulseaudio-service-type)
+(service ladspa-service-type
+         (ladspa-configuration (plugins (list swh-plugins))))
 @end lisp
+
+See @uref{http://plugin.org.uk/ladspa-swh/docs/ladspa-swh.html} for the
+details.
+
 @end deffn
 
 @node Database Services
diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index 5d72b3ada2..64b45f585f 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -35,7 +35,10 @@
             alsa-service-type
 
             pulseaudio-configuration
-            pulseaudio-service-type))
+            pulseaudio-service-type
+
+            ladspa-configuration
+            ladspa-service-type))
 
 ;;; Commentary:
 ;;;
@@ -162,4 +165,32 @@ ctl.!default {
    (default-value (pulseaudio-configuration))
    (description "Configure PulseAudio sound support.")))
 
+
+;;;
+;;; LADSPA
+;;;
+
+(define-record-type* <ladspa-configuration>
+  ladspa-configuration make-ladspa-configuration
+  ladspa-configuration?
+  (plugins ladspa-plugins (default '())))
+
+(define (ladspa-environment config)
+  ;; Define this variable in the global environment such that
+  ;; pulseaudio swh-plugins (and similar LADSPA plugins) work.
+  `(("LADSPA_PATH" .
+     (string-join
+      ',(map (lambda (package) (file-append package "/lib/ladspa"))
+             (ladspa-plugins config))
+      ":"))))
+
+(define ladspa-service-type
+  (service-type
+   (name 'ladspa)
+   (extensions
+    (list (service-extension session-environment-service-type
+                             ladspa-environment)))
+   (default-value (ladspa-configuration))
+   (description "Configure LADSPA plugins.")))
+
 ;;; sound.scm ends here
-- 
2.24.1

Leo Prikler wrote on 10 Jan 2020 02:48

[PATCH 6/6] services: Add pulseaudio to %desktop-services.

Recipients:(address . guix-patches@gnu.org)(address . 39053@debbugs.gnu.org)

Message-ID:20200110014823.10595-6-leo.prikler@student.tugraz.at

* gnu/services/desktop.scm (%desktop-services): Add pulseaudio service.

---

gnu/services/desktop.scm | 1 +

1 file changed, 1 insertion(+)

Toggle diff (14 lines)diff --git a/gnu/services/desktop.scm b/gnu/services/desktop.scm
index b40622a637..1be05fda4e 100644
--- a/gnu/services/desktop.scm
+++ b/gnu/services/desktop.scm
@@ -1183,6 +1183,7 @@ or setting its password with passwd.")))
 
          x11-socket-directory-service
 
+         (service pulseaudio-service-type)
          (service alsa-service-type)
 
          %base-services))
-- 
2.24.1

Leo Prikler wrote on 10 Jan 2020 02:48

[PATCH 5/6] doc: Add pulseaudio documentation.

Recipients:(address . guix-patches@gnu.org)(address . 39053@debbugs.gnu.org)

Message-ID:20200110014823.10595-5-leo.prikler@student.tugraz.at

* doc/guile.texi: Add documentation for pulseaudio-service-type and
pulseaudio-configuration.
---
 doc/guix.texi | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Toggle diff (55 lines)diff --git a/doc/guix.texi b/doc/guix.texi
index 431895aa7d..05d826f6d8 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15951,6 +15951,48 @@ pcm.!default @{
 See @uref{https://www.alsa-project.org/main/index.php/Asoundrc} for the
 details.
 
+@deffn {Scheme Variable} pulseaudio-service-type
+This is the type for the  @uref{http://www.pulseaudio.org/, PulseAudio}
+sound server.  It exists to allow system overrides of the default settings
+via @code{pulseaudio-configuration}, see below.
+
+@quotation Warning
+This service on its own does not ensure, that the @code{pulseaudio} package
+exists on your machine.  It merely adds configuration files for it, as
+detailed below.  In the (admittedly unlikely) case, that you find yourself
+without a @code{pulseaudio} package, consider enabling it through the
+@code{alsa-service-type} above.
+@end quotation
+@end deffn
+
+@deftp {Data Type} pulseaudio-configuration
+Data type representing the configuration for @code{pulseaudio-service}.
+
+@table @asis
+@item @code{client-conf} (default: @var{()})
+List of settings to set in @file{client.conf}.
+Accepts a list of strings or a symbol-value pairs.  A string will be
+inserted as-is with a newline added.  A pair will be formatted as
+``key = value'', again with a newline added.
+
+@item @code{daemon-conf} (default: @var{((flat-volumes . no))})
+List of settings to set in @file{daemon.conf}, formatted just like
+@code{client-conf}.
+
+In addition to the above, @code{default-script-file} will be set to the
+value of @code{script-file}.  By default, @var{flat-volumes} is set to
+``no'', so as to avoid bugs related to this feature.
+
+@item @code{script-file}
+Script file to use as as @file{default.pa}.  Defaults to the one included by
+the @code{pulseaudio} package.
+
+@item @code{system-script-file}
+Script file to use as as @file{system.pa}.  Defaults to the one included by
+the @code{pulseaudio} package.
+@end table
+@end deftp
+
 @deffn {Scheme Variable} ladspa-service-type
 This service sets the @var{LADSPA_PATH} variable, so that programs, which
 respect it, e.g. PulseAudio, can load LADSPA plugins.
-- 
2.24.1

Marius Bakke wrote on 11 Jan 2020 17:48

Re: bug#38172: WebkitGTK-based browsers: System volume suddenly maxed out when playing audio or video

Recipients:

Message-ID:87zheuym66.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (11 lines)> Am Donnerstag, den 09.01.2020, 21:48 +0100 schrieb Marius Bakke:
>> 
>> I have a preference for making this field empty initially to have a
>> 1:1
>> compatibility with the current PA client and daemon configuration
>> (i.e. nothing).  Then a follow-up patch can add this new
>> configuration,
>> perhaps with an explaining comment.
> Fair enough.  This would mean I'd have to split 0001 into two, but
> okay.

Excellent. :-)

Toggle quote (7 lines)

>> Does it make sense to have default-script and system-script default

>> to

>> (file-append pulseaudio "...") and avoid the conditional altogether?

> The idea behind it was to have the script itself in the code rather

> than asking users to construct a mixed-text-file, but I'm fine either

> way.

Right. I just have a preference for the default being "up-front",

instead of magic hiding behind an #f. :-)

There's also LOCAL-FILE and PLAIN-FILE, which are more "obvious" than

MIXED-TEXT-FILE.

It could be useful to support plain strings for users who don't wish to

much about with G-expressions though, hopefully users will send a bug

report if they find it limiting.

Marius Bakke wrote on 11 Jan 2020 17:54

Recipients:(address . control@debbugs.gnu.org)

Message-ID:87v9piyluy.fsf@devup.no

merge 39053 39052 39054 39055 39062 39061 39063 39065 39064

thanks

Marius Bakke wrote on 11 Jan 2020 17:57

Recipients:(address . 39053@debbugs.gnu.org)

Message-ID:87tv52ylpn.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (21 lines)> * gnu/services/sound (pulseaudio-environment): Add PULSE_CONFIG and
> PULSE_CLIENTCONFIG.
> ---
>  gnu/services/sound.scm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
> index 307e62fd1b..6b2b345a44 100644
> --- a/gnu/services/sound.scm
> +++ b/gnu/services/sound.scm
> @@ -123,8 +123,10 @@ ctl.!default {
>  (define (pulseaudio-environment config)
>    ;; Define this variable in the global environment such that
>    ;; pulseaudio swh-plugins works.
> -  `(("LADSPA_PATH"
> -     . ,(file-append swh-plugins "/lib/ladspa"))))
> +  `(("LADSPA_PATH" . ,(file-append swh-plugins "/lib/ladspa"))
> +    ;; Define these variables, so that pulseaudio honors /etc.
> +    ("PULSE_CONFIG" . "/etc/pulse/daemon.conf")
> +    ("PULSE_CLIENTCONFIG" . "/etc/pulse/client.conf")))

I squashed this with the first patch.

By the way, for future merge request, please open a single bug report

and send all patches to NNNNN@debbugs.gnu.org. I've merged the bugs

opened by this series.

Marius Bakke wrote on 11 Jan 2020 17:59

Re: [bug#39063] [PATCH 3/6] services: pulseaudio-configuration: Set flat-volumes to no.

Recipients:(address . 39053@debbugs.gnu.org)

Message-ID:87r206yln8.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (5 lines)

> * gnu/services/sound (pulseaudio-configuration) [daemon-conf]:

> Disable flat-volumes. Enabling flat-volumes, as is the upstream default,

> causes unpleasant experiences to users when applications inadvertently max

> out the system volume (e.g. as in #38172).

I moved this comment inside the service definition code so that we don't

have to reach for 'git blame' when we forget why it was added.

Marius Bakke wrote on 11 Jan 2020 18:00

Re: [bug#39065] [PATCH 4/6] services: Split ladspa-service-type from pulseaudio-service-type.

Recipients:(address . 39053@debbugs.gnu.org)

Message-ID:87o8vayll9.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (6 lines)

> * gnu/services/sound.scm (ladspa-configuration): New record.

> (ladspa-environment): New procedure.

> (ladspa-service-type): New service type.

> (pulseaudio-environment): Remove LADSPA_PATH.

> * doc/guix.texi: Adjust documentation.

This forgot to actually remove LADSPA_PATH from PULSEAUDIO-ENVIRONMENT;

fixed!

The previous pulseaudio-service-type is so recent that I don't think a

deprecation notice is necessary.

Marius Bakke wrote on 11 Jan 2020 18:09

Re: [bug#39064] [PATCH 5/6] doc: Add pulseaudio documentation.

Recipients:(address . 39053@debbugs.gnu.org)

Message-ID:87lfqdzzrj.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (3 lines)

> * doc/guile.texi: Add documentation for pulseaudio-service-type and

> pulseaudio-configuration.

Ideally this would have been squashed with the first commit, but then

we'd have to move ladspa-service-type first, so I kept it as a

standalone commit. I also added a copyright notice for you.

[...]

Toggle quote (18 lines)> +@deftp {Data Type} pulseaudio-configuration
> +Data type representing the configuration for @code{pulseaudio-service}.
> +
> +@table @asis
> +@item @code{client-conf} (default: @var{()})
> +List of settings to set in @file{client.conf}.
> +Accepts a list of strings or a symbol-value pairs.  A string will be
> +inserted as-is with a newline added.  A pair will be formatted as
> +``key = value'', again with a newline added.
> +
> +@item @code{daemon-conf} (default: @var{((flat-volumes . no))})
> +List of settings to set in @file{daemon.conf}, formatted just like
> +@code{client-conf}.
> +
> +In addition to the above, @code{default-script-file} will be set to the
> +value of @code{script-file}.  By default, @var{flat-volumes} is set to
> +``no'', so as to avoid bugs related to this feature.

The first sentence of this paragraph is obsolete, no? The second is

rather vague, so I opted to remove the whole thing. Let me know if you

think something should be added!

Toggle quote (10 lines)> +@item @code{script-file}
> +Script file to use as as @file{default.pa}.  Defaults to the one included by
> +the @code{pulseaudio} package.
> +
> +@item @code{system-script-file}
> +Script file to use as as @file{system.pa}.  Defaults to the one included by
> +the @code{pulseaudio} package.
> +@end table
> +@end deftp

I added a (default: ...) on these two and removed the related sentences.

Marius Bakke wrote on 11 Jan 2020 18:10

Re: [bug#39053] [PATCH 6/6] services: Add pulseaudio to %desktop-services.

Recipients:

Message-ID:87imlhzzp4.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (2 lines)

> * gnu/services/desktop.scm (%desktop-services): Add pulseaudio service.

I added a "Fixes: " line to the commit message pointing to the bug

report that inspired this series.

Marius Bakke wrote on 11 Jan 2020 18:23

Re: [bug#39053] [PATCH] Add pulseaudio configuration and fix volume bugs

Recipients:(address . 38172-done@debbugs.gnu.org)

Message-ID:87ftglzz3l.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (4 lines)

> This series of patches adds a configuration type for pulseaudio and also fixes

> a bug, where various applications would inadvertently max out the system volume

> (see e.g. #38172).

Thanks! I've pushed the patches with mentioned tweaks in

2c7511fb6..71e33e32f.

Closed

Marius Bakke wrote on 11 Jan 2020 18:25

Re: [bug#39064] [PATCH 5/6] doc: Add pulseaudio documentation.

Recipients:(address . 39053@debbugs.gnu.org)

Message-ID:87d0bpzyzp.fsf@devup.no

Marius Bakke <mbakke@fastmail.com> writes:

Toggle quote (12 lines)>> +@item @code{script-file}
>> +Script file to use as as @file{default.pa}.  Defaults to the one included by
>> +the @code{pulseaudio} package.
>> +
>> +@item @code{system-script-file}
>> +Script file to use as as @file{system.pa}.  Defaults to the one included by
>> +the @code{pulseaudio} package.
>> +@end table
>> +@end deftp
>
> I added a (default: ...) on these two and removed the related sentences.

I also changed to @var{...} instead of @code{...} for the options; and
used @code{...} instead of @var{...} on the defaults after reading the
generated HTML.

Leo Prikler wrote on 11 Jan 2020 19:37

Re: [bug#39053] [PATCH] Add pulseaudio configuration and fix volume bugs

Recipients:(address . 38172-done@debbugs.gnu.org)

Message-ID:d17731158ca344803f624bc6c08e5490065f9c40.camel@student.tugraz.at

Am Samstag, den 11.01.2020, 18:23 +0100 schrieb Marius Bakke:

Toggle quote (10 lines)> Leo Prikler <leo.prikler@student.tugraz.at> writes:
> 
> > This series of patches adds a configuration type for pulseaudio and
> > also fixes
> > a bug, where various applications would inadvertently max out the
> > system volume
> > (see e.g. #38172).
> 
> Thanks!  I've pushed the patches with mentioned tweaks in
> 2c7511fb6..71e33e32f.

Thanks!  Also, I'm sorry about accidentally opening like 10 bugs due to
my misconfiguration there.  I only noticed after the fact, that merely
CC'ing the original bug does nothing, when the mail is still sent to
guix-patches as well.  I've learned my lesson and will be more careful
in the future.

I do still have some open questions, though.

Toggle quote (11 lines)> > +In addition to the above, @code{default-script-file} will be set to
> > the
> > +value of @code{script-file}.  By default, @var{flat-volumes} is
> > set
> > to
> > +``no'', so as to avoid bugs related to this feature.
> > 
> The first sentence of this paragraph is obsolete, no?  The second is
> rather vague, so I opted to remove the whole thing.  Let me know if
> you
> think something should be added!

I'm not quite sure about the first sentence.  While everyone can read
the code and the output files produced from it, I think we should
document, that we actually always insert this line into
@file{daemon.conf}.
For instance, if you don't supply your own @file{default.pa}, the first
line of @file{daemon.conf} will be

Toggle snippet (3 lines)

default-script-file = /gnu/store/<hash>-pulseaudio-

<version>/etc/pulse/default.pa

What are your thoughts on this?

Toggle quote (2 lines)

> I added a (default: ...) on these two and removed the related

> sentences.

I was hesitant to do that due to the line limits. Do those not count

for documentation or are such exceptions allowed?

Thanks again for your help and also thanks for your feedback.

Regards,

Leo

Closed

Marius Bakke wrote on 12 Jan 2020 00:49

Recipients:

Message-ID:87r205y2oj.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (17 lines)> Am Samstag, den 11.01.2020, 18:23 +0100 schrieb Marius Bakke:
>> Leo Prikler <leo.prikler@student.tugraz.at> writes:
>> 
>> > This series of patches adds a configuration type for pulseaudio and
>> > also fixes
>> > a bug, where various applications would inadvertently max out the
>> > system volume
>> > (see e.g. #38172).
>> 
>> Thanks!  I've pushed the patches with mentioned tweaks in
>> 2c7511fb6..71e33e32f.
> Thanks!  Also, I'm sorry about accidentally opening like 10 bugs due to
> my misconfiguration there.  I only noticed after the fact, that merely
> CC'ing the original bug does nothing, when the mail is still sent to
> guix-patches as well.  I've learned my lesson and will be more careful
> in the future.

Right, no worries!

Toggle quote (24 lines)> I do still have some open questions, though.
>> > +In addition to the above, @code{default-script-file} will be set to
>> > the
>> > +value of @code{script-file}.  By default, @var{flat-volumes} is
>> > set
>> > to
>> > +``no'', so as to avoid bugs related to this feature.
>> > 
>> The first sentence of this paragraph is obsolete, no?  The second is
>> rather vague, so I opted to remove the whole thing.  Let me know if
>> you
>> think something should be added!
> I'm not quite sure about the first sentence.  While everyone can read
> the code and the output files produced from it, I think we should
> document, that we actually always insert this line into
> @file{daemon.conf}.
> For instance, if you don't supply your own @file{default.pa}, the first
> line of @file{daemon.conf} will be
> --8<---------------cut here---------------start------------->8---
> default-script-file = /gnu/store/<hash>-pulseaudio-
> <version>/etc/pulse/default.pa
> --8<---------------cut here---------------end--------------->8---
> What are your thoughts on this?

Oh, right. I suppose that can be surprising. Would you like to

resubmit a documentation update?

I wonder if we could use the flunking new 'this-record', and refer to

the SCRIPT-FILE directly in the default parameter list. But that's

probably overkill for this instance. :-)

Toggle quote (5 lines)

>> I added a (default: ...) on these two and removed the related

>> sentences.

> I was hesitant to do that due to the line limits. Do those not count

> for documentation or are such exceptions allowed?

Not sure what/if there are line length limits on guix.texi, but 95

characters should be okay for one-off instances like these. I suppose

we could escape a line break if it breaks someones workflow.

Toggle quote (2 lines)

> Thanks again for your help and also thanks for your feedback.

Thanks!

Closed

Leo Prikler wrote on 12 Jan 2020 01:32

Recipients:

Message-ID:ac2fe47d175836a1eaa25d12830cbb0f510759cd.camel@student.tugraz.at

Am Sonntag, den 12.01.2020, 00:49 +0100 schrieb Marius Bakke:

Toggle quote (2 lines)

> Oh, right. I suppose that can be surprising. Would you like to

> resubmit a documentation update?

Sure, see the attachment. (Not sure if that's overkill.)

Toggle quote (3 lines)

> I wonder if we could use the flunking new 'this-record', and refer to

> the SCRIPT-FILE directly in the default parameter list. But that's

> probably overkill for this instance. :-)

Well, actually, that might not be /that/ bad, were it not for the fact
that defaults have to be documented.  Using 'this-record' would allow
people to use the system-script-file there as well, although I don't
know if the value will be read when PA starts in system mode.
That said, we would also force people to copypasta this into their own
configuration.  Is that still acceptable?

Toggle quote (4 lines)

> Not sure what/if there are line length limits on guix.texi, but 95

> characters should be okay for one-off instances like these. I

> suppose

> we could escape a line break if it breaks someones workflow.

I personally would not notice a line limit either way safe for perhaps
some very short lines, due to having olivetti enabled in all text
modes.

Regards,
Leo

From c1f3757d53423cc51f1f9017edd4427c20e7025c Mon Sep 17 00:00:00 2001
From: Leo Prikler <leo.prikler@student.tugraz.at>
Date: Sun, 12 Jan 2020 01:20:27 +0100
Subject: [PATCH] doc: Fully document pulseaudio daemon-conf.

* doc/guix.texi: Add note about the default-script-file line and the intention
behind it.
---
 doc/guix.texi | 11 +++++++++++
 1 file changed, 11 insertions(+)

Toggle diff (24 lines)diff --git a/doc/guix.texi b/doc/guix.texi
index d2038d18e1..0b8569b54a 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15979,6 +15979,17 @@ inserted as-is with a newline added.  A pair will be formatted as
 @item @var{daemon-conf} (default: @code{'((flat-volumes . no))})
 List of settings to set in @file{daemon.conf}, formatted just like
 @var{client-conf}.
+@quotation Note
+In addition to the above settings being applied, the first line of the
+generated @file{daemon.conf} will always read
+@code{default-script-file = FILE} with @var{FILE} being the path to the
+supplied @var{script-file}.
+This makes pulseaudio read the correct @file{default.pa} when started in
+user-mode (the default behaviour).
+Such a line does not exist for @file{system.pa}.  If you need to load this
+file for some reason, you'll have to use environment variables as detailed
+in the PulseAudio documentation.
+@end quotation
 
 @item @var{script-file} (default: @code{(file-append pulseaudio "/etc/pulse/default.pa")})
 Script file to use as as @file{default.pa}.
-- 
2.24.1

Closed

Marius Bakke wrote on 12 Jan 2020 20:55

Recipients:

Message-ID:87ftgkxxel.fsf@devup.no

Leo Prikler <leo.prikler@student.tugraz.at> writes:

Toggle quote (5 lines)

> Am Sonntag, den 12.01.2020, 00:49 +0100 schrieb Marius Bakke:

>> Oh, right. I suppose that can be surprising. Would you like to

>> resubmit a documentation update?

> Sure, see the attachment. (Not sure if that's overkill.)

Looks good.

[...]

Toggle quote (20 lines)> diff --git a/doc/guix.texi b/doc/guix.texi
> index d2038d18e1..0b8569b54a 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -15979,6 +15979,17 @@ inserted as-is with a newline added.  A pair will be formatted as
>  @item @var{daemon-conf} (default: @code{'((flat-volumes . no))})
>  List of settings to set in @file{daemon.conf}, formatted just like
>  @var{client-conf}.
> +@quotation Note
> +In addition to the above settings being applied, the first line of the
> +generated @file{daemon.conf} will always read
> +@code{default-script-file = FILE} with @var{FILE} being the path to the
> +supplied @var{script-file}.
> +This makes pulseaudio read the correct @file{default.pa} when started in
> +user-mode (the default behaviour).
> +Such a line does not exist for @file{system.pa}.  If you need to load this
> +file for some reason, you'll have to use environment variables as detailed
> +in the PulseAudio documentation.
> +@end quotation

I tried finding documentation for system.pa to no avail.  Do you have a
link at hand?  Perhaps it's better to remove the system.pa sentences
from here, and add a note below "system-script-file" how to make it
effective, preferably with an @url{...} pointing to PA documentation.

WDYT?

Closed

Leo Prikler wrote on 12 Jan 2020 21:22

Recipients:

Message-ID:79745f2b1534d7562f4384df6fd7bd0b3294a91d.camel@student.tugraz.at

Am Sonntag, den 12.01.2020, 20:55 +0100 schrieb Marius Bakke:

Toggle quote (43 lines)> Leo Prikler <leo.prikler@student.tugraz.at> writes:
> 
> > Am Sonntag, den 12.01.2020, 00:49 +0100 schrieb Marius Bakke:
> > > Oh, right.  I suppose that can be surprising.  Would you like to
> > > resubmit a documentation update?
> > Sure, see the attachment.  (Not sure if that's overkill.)
> 
> Looks good.
> 
> [...]
> 
> 
> > diff --git a/doc/guix.texi b/doc/guix.texi
> > index d2038d18e1..0b8569b54a 100644
> > --- a/doc/guix.texi
> > +++ b/doc/guix.texi
> > @@ -15979,6 +15979,17 @@ inserted as-is with a newline added.  A
> > pair will be formatted as
> >  @item @var{daemon-conf} (default: @code{'((flat-volumes . no))})
> >  List of settings to set in @file{daemon.conf}, formatted just like
> >  @var{client-conf}.
> > +@quotation Note
> > +In addition to the above settings being applied, the first line of
> > the
> > +generated @file{daemon.conf} will always read
> > +@code{default-script-file = FILE} with @var{FILE} being the path
> > to the
> > +supplied @var{script-file}.
> > +This makes pulseaudio read the correct @file{default.pa} when
> > started in
> > +user-mode (the default behaviour).
> > +Such a line does not exist for @file{system.pa}.  If you need to
> > load this
> > +file for some reason, you'll have to use environment variables as
> > detailed
> > +in the PulseAudio documentation.
> > +@end quotation
> 
> I tried finding documentation for system.pa to no avail.  Do you have
> a
> link at hand?  Perhaps it's better to remove the system.pa sentences
> from here, and add a note below "system-script-file" how to make it
> effective, preferably with an @url{...} pointing to PA documentation.

That's not surprising, given that the format for default.pa and
system.pa is the same.  The convention in PulseAudio is, that the
former be loaded when it is started in "user mode", and the latter be
loaded when it is started in "system mode".  However, there exists but
one environment variable (PULSE_SCRIPT now that I recall its name),
that overrides BOTH settings.

Upon closer inspection, it seems however, that this environment
variable is not the only way to to supply a script to load.
Reading the output of `pulseaudio --help` also contains a few lines on
that.

Toggle snippet (12 lines)

STARTUP SCRIPT:

-L, --load="MODULE ARGUMENTS" Load the specified plugin

module with

the specified argument

-F, --file=FILENAME Run the specified script

-C Open a command line on the

running TTY

after startup

-n Don't load default script file

Toggle quote (1 lines)

> WDYT?

It should be worded in a way, that does not cause such a
misunderstanding.  What I meant to convey (but failed at doing so) was
something along the lines of "consult the PulseAudio documentation on
how to load scripts", not "consult the PulseAudio documentation on
system.pa".  Not loading system.pa unless being told to do so is a
Guix-specific feature ;)

Can you patch this documentation in a way that people, who are not me,
also understand it?  I fully admit that I'm weak at explaining.

Regards,
Leo

Closed

Ludovic Courtès wrote on 17 Jan 2020 17:30

Re: [bug#39066] [PATCH 6/6] services: Add pulseaudio to %desktop-services.

Recipients:(name . Leo Prikler)(address . leo.prikler@student.tugraz.at)

Message-ID:8736ce11vv.fsf@gnu.org

Leo Prikler <leo.prikler@student.tugraz.at> skribis:

Toggle quote (2 lines)

> * gnu/services/desktop.scm (%desktop-services): Add pulseaudio service.

This was done in 71e33e32fcedbd0aaafda4fd548fb8443064253c.

Closing!

Ludo'.

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 39053@debbugs.gnu.org

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date