Bug 211684 - audio/pulseaudio: Respect hw.snd.default_unit
Summary: audio/pulseaudio: Respect hw.snd.default_unit
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jan Beich
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-08-09 03:18 UTC by Tobias Kortkamp
Modified: 2016-11-11 05:28 UTC (History)
3 users (show)

See Also:
jbeich: merge-quarterly+


Attachments
pulseaudio.diff (1.54 KB, patch)
2016-08-09 03:18 UTC, Tobias Kortkamp
no flags Details | Diff
Proposed patch (since 419133 revision) for v8.0 (935 bytes, patch)
2016-08-09 23:14 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v8.0 (2.38 KB, patch)
2016-08-10 14:25 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v8.0 (2.61 KB, patch)
2016-08-10 15:49 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v8.0 (3.72 KB, patch)
2016-08-10 16:42 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v9.0 (11.19 KB, patch)
2016-08-10 16:50 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v8.0 (3.46 KB, patch)
2016-08-13 10:12 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v9.0 (10.93 KB, patch)
2016-08-13 10:13 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v9.0 (10.93 KB, patch)
2016-08-21 18:42 UTC, lightside
no flags Details | Diff
Proposed patch (since 419133 revision) for v9.0 (10.81 KB, patch)
2016-10-02 01:12 UTC, lightside
no flags Details | Diff
Proposed patch (since 424411 revision) for v9.0 (10.64 KB, patch)
2016-10-25 07:18 UTC, lightside
no flags Details | Diff
Proposed patch (since 424621 revision) (3.49 KB, patch)
2016-10-25 14:02 UTC, lightside
no flags Details | Diff
Proposed patch (since 424621 revision) (3.63 KB, patch)
2016-11-09 20:58 UTC, lightside
no flags Details | Diff
Proposed patch (since 424621 revision) (3.57 KB, patch)
2016-11-09 21:50 UTC, lightside
lightside: maintainer-approval? (gnome)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Kortkamp freebsd_committer 2016-08-09 03:18:26 UTC
Created attachment 173433 [details]
pulseaudio.diff

I apologize in advance for the length, odd style and timing of this
bug report.  I know there is a pending update to PulseAudio 9.0 in
bug #210548.  But this should also be applicable to that version.
This one has been bugging me for a while, and others apparently:
https://forums.freebsd.org/threads/45829/#post-319611

IMHO the current pkg-message of PulseAudio reads like a bug report
already, so I'm going to paste part of it here:

> Pulseaudio doesn't know about the hw.snd.default_unit=3 sysctl for the
> FreeBSD OSS driver that is used to select the active input/output. So
> for Pulseaudio we also need to tell it which input/output to use. The
> difference is that Pulseaudio has separate input and output configure lines.
> To change the default sink (output):
> # pacmd set-default-sink 3
> To change the default source (input):
> # pacmd set-default-source 3

I propose a small change to module-detect.c so we can make PulseAudio
actually respect the default device.

By loading an OSS module instance that uses /dev/dsp first (before all
the others in /dev/sndstat) we can make PulseAudio default to using
it instead of /dev/dsp0 i.e. it actually respects the default sound
unit set with sysctl hw.snd.default_unit.  The default device will
appear again in /dev/sndstat and thus will be duplicated in PulseAudio
but that should not matter much for the benefit gained (a sane default
user experience and a smaller patch).

How to test that this works (or how I tested that this works):

0. this is before patching and assumes you haven't changed the system
   configuration i.e. /usr/local/etc/pulse/*
1. rm -r ~/.config/pulse (just to make sure there are no previous settings left)
2. set the default unit to something that isn't 0 e.g. sysctl hw.snd.default_unit=5
3. pkill pulseaudio
4. pulseaudio --daemonize=no -v (optional, only do this if you want to see what's going on)
5. paplay /usr/local/share/sounds/alsa/Front_Center.wav (assuming
   alsa-utils installed, otherwise use another wav file)   
6. silence...
7. a quick look at pacmd list-sinks shows that /dev/dsp0 is the active
   sink (the one with the *) and not /dev/dsp5

8. ok let's apply this patch
9. cd /usr/ports/audio/pulseaudio; svn patch ~/pulseaudio.diff
10. make reinstall
11. pkill pulseaudio
12. rm -r ~/.config/pulse (just to make sure there are no previous settings left)
13. pulseaudio --daemonize=no -v
14. paplay /usr/local/share/sounds/alsa/Front_Center.wav
15. "Front...Center". Great!
16. in pacmd list-sinks we see that /dev/dsp is the active sink

Thanks to PulseAudio suspending and reopening devices, changes to
hw.snd.default_unit are also picked up without restarting the daemon
(albeit with some latency and when no client currently uses it), or
editing some config file, or running an extra command after sysctl hw.snd.default_unit=5.

Of course, even with this patch applied users can still use all the
other sinks like before if they have more advanced use cases than just
wanting Firefox (or any other PulseAudio client) to play sound.

portlint ok (ignoring preexisting warnings), Poudriere testport on
FreeBSD 9.3/i386 ok
Comment 1 lightside 2016-08-09 23:14:50 UTC
Created attachment 173479 [details]
Proposed patch (since 419133 revision) for v8.0

Hello Tobias Kortkamp.

I investigated this issue and corrected the approach by PulseAudio's developer(s), which recognizes hw.snd.default_unit value, by parsing and comparing the last "default" string for /dev/sndstat output.
I attached patch for current audio/pulseaudio 8.0 version. If it works for you, then I can propose it (with slight modifications) for 9.0 version update in bug 210548.

The other possible approach is to use SNDCTL_AUDIOINFO to get necessary values (e.g. mixer_dev):
-8<--
// cc snd_default_unit.c -o snd_default_unit.o && ./snd_default_unit.o
#include <sys/ioctl.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/soundcard.h>
#include <stdio.h>

int main() {
	int mixerfd = open("/dev/dsp", O_RDWR, 0);
	oss_audioinfo ainfo;
	ainfo.dev = -1; // specify -1 to get default device

	int result = ioctl(mixerfd, SNDCTL_AUDIOINFO, &ainfo);
	close(mixerfd);

	if (result == -1 ) {
		printf("error getting device (%s) info.\n", ainfo.name);
		return 1;
	}

	printf("default device name: %s\ndefault device node name: %s\n\
default device number: %d\ndefault device mixer number: %d\n",\
		ainfo.name, ainfo.devnode, ainfo.dev, ainfo.mixer_dev);

	return 0;
}
-->8-

The idea about how to get default sound device for above approach is from changes in bug 209742, proposed by Shane Ambler.
Comment 2 Tobias Kortkamp freebsd_committer 2016-08-10 00:28:40 UTC
(In reply to lightside from comment #1)
Hi Lightside,

thanks for looking into it.

Your patch works but has a big flaw in that it only creates a
sink for the default device.  The other devices are skipped.
That's a big divergence from the current behavior.  Users might
want to switch streams to a specific sink (OSS device).

Given that it only creates one sink, there is no point in even
reading /dev/sndstat anymore as we could just create one sink for
/dev/dsp.

Since you went to the trouble of recreating the patch in a
different way, I'd like to know why you dismissed my approach?
What's wrong with it in your eyes?  I ask because I'm willing to
do the work on this one, even for PulseAudio 9.0, if someone tells
me what to change and explains why.

Let me summarize my POV of what the requirements of this are:

1. PulseAudio does not respect hw.snd.default_unit and does not set
   its default sink to the sink corresponding to the default device.

2. PulseAudio uses the first sink as the default sink by default,
   so one way to force PulseAudio to use it, is by adding the default
   device first.

3. That's what I did with my patch, but since /dev/dsp will appear
   again in /dev/sndstat with a different name we get a duplicated
   device in the sink list.  If we don't want that, we need to add the
   default device first, then all the others.  Your patch is half of
   that solution.  There needs to be e.g. a second loop over /dev/sndstat
   after the first one.

4. Not adding /dev/dsp to the sink list means changes to
   hw.snd.default_unit are not picked up by the PulseAudio daemon
   anymore without restarting it.

5. But we also absolutely want to have corresponding sinks for ALL
   other OSS devices in PulseAudio to retain the current flexibility.
Comment 3 lightside 2016-08-10 06:13:35 UTC
(In reply to comment #2)
> Your patch works but has a big flaw in that it only creates a
> sink for the default device.  The other devices are skipped.
Yes, I noticed this just before reading your comment #2.

(In reply to comment #2)
> Since you went to the trouble of recreating the patch in a
> different way, I'd like to know why you dismissed my approach?
I didn't dismiss it, but tried to create a different approach, which uses a logic by PulseAudio's developers.
As I said in comment #1, there is a possibility to determine hw.snd.default_unit.

I think, the other possible approach is following:
If user didn't set-default-sink (or other possible value(s)), then try to determine the hw.snd.default_unit and change it accordingly. The next step is to distinguish this, if hw.snd.default_unit changed again, but user didn't change the set-default-sink, then correct the value. But currently, I'm not sure about step two. And this is just an ideas.

Looks like, I was CC'ed automatically, because you mentioned the bug 210548.
Comment 4 lightside 2016-08-10 14:25:22 UTC
Created attachment 173511 [details]
Proposed patch (since 419133 revision) for v8.0

(In reply to comment #3)
> I think, the other possible approach is following:
> If user didn't set-default-sink (or other possible value(s)), then try to
> determine the hw.snd.default_unit and change it accordingly.
I created new patch for mentioned approach, which also parses "play" and "rec" strings to determine the default sink and/or source device(s).
While testing I noticed, that PulseAudio may decide to use previously changed default sink, in case of source only device, for example:
# sysctl hw.snd.default_unit=0
hw.snd.default_unit: 0 -> 0
% cat /dev/sndstat | sed -e 's|<.*>|<Some device>|'
Installed devices:
pcm0: <Some device> (play/rec) default
pcm1: <Some device> (play)
pcm2: <Some device> (rec)
% echo -n | paplay --raw
% pacmd dump | grep set-default-s
set-default-sink oss_output.dsp0
set-default-source oss_input.dsp0
% pacmd exit

# sysctl hw.snd.default_unit=1
hw.snd.default_unit: 0 -> 1
% cat /dev/sndstat | sed -e 's|<.*>|<Some device>|'
Installed devices:
pcm0: <Some device> (play/rec)
pcm1: <Some device> (play) default
pcm2: <Some device> (rec)
% echo -n | paplay --raw
% pacmd dump | grep set-default-s
set-default-sink oss_output.dsp1
set-default-source oss_input.dsp0
% pacmd exit

# sysctl hw.snd.default_unit=2
hw.snd.default_unit: 1 -> 2
% cat /dev/sndstat | sed -e 's|<.*>|<Some device>|'
Installed devices:
pcm0: <Some device> (play/rec)
pcm1: <Some device> (play)
pcm2: <Some device> (rec) default
% echo -n | paplay --raw
% pacmd dump | grep set-default-s
set-default-sink oss_output.dsp1
set-default-source oss_input.dsp2
% pacmd exit

# sysctl hw.snd.default_unit=0
hw.snd.default_unit: 2 -> 0
% echo -n | paplay --raw
% pacmd dump | grep set-default-s
set-default-sink oss_output.dsp0
set-default-source oss_input.dsp0
% pacmd exit

# sysctl hw.snd.default_unit=2
hw.snd.default_unit: 0 -> 2
% echo -n | paplay --raw
% pacmd dump | grep set-default-s
set-default-sink oss_output.dsp0
set-default-source oss_input.dsp2
% pacmd exit

# sysctl hw.snd.default_unit=0
hw.snd.default_unit: 2 -> 0
% echo -n | paplay --raw
% pacmd dump | grep set-default-s
set-default-sink oss_output.dsp0
set-default-source oss_input.dsp0
% pacmd exit
Comment 5 Tobias Kortkamp freebsd_committer 2016-08-10 15:00:51 UTC
(In reply to lightside from comment #4)
> I created new patch for mentioned approach, which also parses "play" and "rec" strings to determine the default sink and/or source device(s).

Works for me. :-)

> While testing I noticed, that PulseAudio may decide to use previously changed default sink, in case of source only device, for example:

Seems ok to me.  If I set a play-only device as default then PulseAudio will select the most recently selected source as default source.
Comment 6 lightside 2016-08-10 15:49:26 UTC
Created attachment 173513 [details]
Proposed patch (since 419133 revision) for v8.0

(In reply to comment #5)
> Works for me. :-)
Good :)

(In reply to comment #5)
> Seems ok to me.  If I set a play-only device as default then PulseAudio will
> select the most recently selected source as default source.
Looks like, this is related to PulseAudio's behavior in such cases.

I sent you the integrated patch with 9.0 update to email. If it also works for you, then I can propose it for 9.0 version. But I think, it may be applied for 8.0 also, if it really works for other people.

Added PORTREVISION bump.
Comment 7 lightside 2016-08-10 15:52:57 UTC
May be need to correct the files/pkg-message.in also.
Comment 8 Tobias Kortkamp freebsd_committer 2016-08-10 16:32:11 UTC
Comment on attachment 173433 [details]
pulseaudio.diff

I tried out the patch you send me and it seems to works fine too.
Comment 9 lightside 2016-08-10 16:42:53 UTC
Created attachment 173514 [details]
Proposed patch (since 419133 revision) for v8.0

Added possible changes to files/pkg-message.in file. They might be clarified by committer, if needed.
Comment 10 lightside 2016-08-10 16:50:11 UTC
Created attachment 173515 [details]
Proposed patch (since 419133 revision) for v9.0

(In reply to comment #8)
> I tried out the patch you send me and it seems to works fine too.
Good. Thank you.

The maintainer asked me to place the integrated patch into this PR. So, I attached it also.
Comment 11 lightside 2016-08-13 10:12:58 UTC
Created attachment 173625 [details]
Proposed patch (since 419133 revision) for v8.0

I changed parsing algorithm to find last occurrence of '>' character, which may exclude (possible) usage of "> (" along with "play" or "rec" words in device name. This conforms to current format of output in sndstat source code:
on FreeBSD:
https://github.com/freebsd/freebsd/blob/4a8d8f7fba6ae0abcfae7ee40db2bcf9b563fe01/sys/dev/sound/pcm/sndstat.c#L361
https://github.com/freebsd/freebsd/blob/4a8d8f7fba6ae0abcfae7ee40db2bcf9b563fe01/sys/dev/sound/pcm/sndstat.h#L49
and DragonFlyBSD:
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/d78ce965185bbe24349ec52eeb17e712b8a0d8dc/sys/dev/sound/pcm/sndstat.c#L399
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/d78ce965185bbe24349ec52eeb17e712b8a0d8dc/sys/dev/sound/pcm/sndstat.h#L49
Comment 12 lightside 2016-08-13 10:13:30 UTC
Created attachment 173626 [details]
Proposed patch (since 419133 revision) for v9.0
Comment 13 lightside 2016-08-21 18:42:45 UTC
Created attachment 173915 [details]
Proposed patch (since 419133 revision) for v9.0

Corrected description for AUDIO and DATABASE options (related to patch for v9.0).
Comment 14 lightside 2016-10-02 01:12:33 UTC
Created attachment 175354 [details]
Proposed patch (since 419133 revision) for v9.0

Reverted USES+=localbase proposal for v9.0 patch. See bug 210548 comment 32 for explanation.
Comment 15 lightside 2016-10-25 07:18:51 UTC
Created attachment 176133 [details]
Proposed patch (since 424411 revision) for v9.0

Updated patch for v9.0 after ports r424411 changes.
Comment 16 lightside 2016-10-25 14:02:12 UTC
Created attachment 176143 [details]
Proposed patch (since 424621 revision)

Updated patch after ports r424621 changes.
Comment 17 Jan Beich freebsd_committer 2016-11-09 13:08:17 UTC
Comment on attachment 176143 [details]
Proposed patch (since 424621 revision)

Default device parsing fails with hw.snd.verbose > 0. For one, I'm using increased verbosity for https://people.freebsd.org/~ariff/utils/appsmixer and to debug issues with low hw.snd.latency value.
Comment 18 lightside 2016-11-09 20:58:34 UTC
Created attachment 176830 [details]
Proposed patch (since 424621 revision)

(In reply to comment #17)
> Default device parsing fails with hw.snd.verbose > 0.
Not sure what you meant about "parsing fails". The parsing algorithm was developed in such a way, that recognizes concrete format of /dev/sndstat output (e.g. the "default" word at the end of line). If format of output is different, then further algorithm does nothing (as it was for "hw.snd.verbose > 0" case). Theoretically possible to break parsing algorithm (e.g. with custom /dev/sndstat output), I guess. But this is a possible drawback of such approach.

Probably, you meant, that "the parsing algorithm doesn't work for hw.snd.verbose > 0 case". Looks like, sysctl hw.snd.verbose may change format of /dev/sndstat output. I extended parsing algorithm for "hw.snd.verbose > 0" case.

Thanks for review.
Comment 19 lightside 2016-11-09 21:25:48 UTC
(In reply to comment #18)
> I extended parsing algorithm for "hw.snd.verbose > 0" case.
I expanded parsing algorithm for "hw.snd.verbose > 0" case, based on following source code:
https://github.com/freebsd/freebsd/blob/4a8d8f7fba6ae0abcfae7ee40db2bcf9b563fe01/sys/dev/sound/pcm/sndstat.h#L54
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/d78ce965185bbe24349ec52eeb17e712b8a0d8dc/sys/dev/sound/pcm/sndstat.h#L54
Comment 20 lightside 2016-11-09 21:50:02 UTC
Created attachment 176833 [details]
Proposed patch (since 424621 revision)

Simplified parsing algorithm a bit.
Comment 21 commit-hook freebsd_committer 2016-11-09 21:54:50 UTC
A commit references this bug:

Author: jbeich
Date: Wed Nov  9 21:54:30 UTC 2016
New revision: 425809
URL: https://svnweb.freebsd.org/changeset/ports/425809

Log:
  audio/pulseaudio: respect hw.snd.default_unit

  PR:		211684
  Submitted by:	lightside <lightside@gmx.com>
  Reviewed by:	Tobias Kortkamp <t@tobik.me>
  Approved by:	maintainer timeout (3 months)
  MFH:		2016Q4 (notorious for affecting www/firefox)

Changes:
  head/audio/pulseaudio/Makefile
  head/audio/pulseaudio/files/patch-src_modules_module-detect.c
  head/audio/pulseaudio/files/pkg-message.in
Comment 22 Jan Beich freebsd_committer 2016-11-09 21:55:54 UTC
Hmm, I didn't catch attachment 176833 [details] changes...
Comment 23 commit-hook freebsd_committer 2016-11-09 22:06:58 UTC
A commit references this bug:

Author: jbeich
Date: Wed Nov  9 22:06:43 UTC 2016
New revision: 425812
URL: https://svnweb.freebsd.org/changeset/ports/425812

Log:
  audio/pulseaudio: simplify hw.snd.default_unit search

  PR:		211684
  Submitted by:	lightside <lightside@gmx.com>
  MFH:		2016Q4 (r425809 bandwagon)

Changes:
  head/audio/pulseaudio/Makefile
  head/audio/pulseaudio/files/patch-src_modules_module-detect.c
Comment 24 Jan Beich freebsd_committer 2016-11-09 22:08:07 UTC
Thanks. Landed.
Comment 25 lightside 2016-11-09 22:11:07 UTC
(In reply to comment #24)
> Thanks. Landed.
Thanks for commit.
Comment 26 commit-hook freebsd_committer 2016-11-11 05:20:22 UTC
A commit references this bug:

Author: jbeich
Date: Fri Nov 11 05:19:20 UTC 2016
New revision: 425860
URL: https://svnweb.freebsd.org/changeset/ports/425860

Log:
  MFH: r425809 r425812

  audio/pulseaudio: respect hw.snd.default_unit

  PR:		211684
  Submitted by:	lightside <lightside@gmx.com>
  Reviewed by:	Tobias Kortkamp <t@tobik.me>
  Approved by:	maintainer timeout (3 months)
  Approved by:	ports-secteam (junovitch)

Changes:
_U  branches/2016Q4/
  branches/2016Q4/audio/pulseaudio/Makefile
  branches/2016Q4/audio/pulseaudio/files/patch-src_modules_module-detect.c
  branches/2016Q4/audio/pulseaudio/files/pkg-message.in