Bug 245156 - audio/pulseaudio: parse /dev/sndstat correctly to get device name
Summary: audio/pulseaudio: parse /dev/sndstat correctly to get device name
Status: New
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-gnome (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-29 06:32 UTC by Henry Hu
Modified: 2020-07-11 17:38 UTC (History)
8 users (show)

See Also:
bugzilla: maintainer-feedback? (gnome)


Attachments
patch to fix sndstat parsing (2.90 KB, patch)
2020-03-29 06:32 UTC, Henry Hu
no flags Details | Diff
2nd versin of the patch (2.99 KB, text/plain)
2020-03-29 16:32 UTC, Henry Hu
no flags Details
Proposed patch (since 516738 revision, v1) (2.26 KB, patch)
2020-03-30 07:00 UTC, lightside
no flags Details | Diff
Proposed patch (since 516738 revision, v2) (2.23 KB, patch)
2020-03-30 07:02 UTC, lightside
no flags Details | Diff
Testcase (1.06 KB, text/plain)
2020-03-30 23:52 UTC, lightside
no flags Details
Testcase (1.01 KB, text/plain)
2020-03-31 00:07 UTC, lightside
no flags Details
Proposed patch (since 516738 revision, v1) (2.49 KB, patch)
2020-03-31 00:40 UTC, lightside
no flags Details | Diff
Proposed patch (since 516738 revision, v2) (2.27 KB, patch)
2020-03-31 00:41 UTC, lightside
no flags Details | Diff
Testcase (900 bytes, text/plain)
2020-04-03 06:35 UTC, lightside
no flags Details
Proposed patch (since 516738 revision, v1) (2.31 KB, patch)
2020-04-03 06:35 UTC, lightside
no flags Details | Diff
Proposed patch (since 516738 revision, v2) (2.27 KB, patch)
2020-04-03 06:36 UTC, lightside
no flags Details | Diff
Testcase 2 (900 bytes, text/plain)
2020-04-03 18:08 UTC, lightside
no flags Details
Proposed patch (since 516738 revision, v3) (2.37 KB, patch)
2020-04-03 18:10 UTC, lightside
no flags Details | Diff
Proposed patch (since 516738 revision, v3) (2.37 KB, patch)
2020-04-04 11:10 UTC, lightside
no flags Details | Diff
Proposed patch (since 516738 revision, v3) (2.30 KB, patch)
2020-04-04 11:33 UTC, lightside
lightside: maintainer-approval? (gnome)
Details | Diff
Testcase 3 (580 bytes, text/plain)
2020-04-04 11:43 UTC, lightside
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Henry Hu 2020-03-29 06:32:35 UTC
Created attachment 212817 [details]
patch to fix sndstat parsing

pulseaudio's oss module parses /dev/sndstat to get device name of each device. However, it cannot parse FreeBSD's sndstat correctly. As a result, devices are named "/dev/dspN" under FreeBSD, which is kind of ugly.

This patch changes the parsing logic to correctly handle sndstat produced by FreeBSD OSS implementation. As a result, device names can be correctly obtained by pulseaudio and shown in control panels.
Comment 1 Henry Hu 2020-03-29 16:32:33 UTC
Created attachment 212838 [details]
2nd versin of the patch

This is the second version of the patch. Instead of just using the card name, the new version prepends "pcmN: " to the device name.
This allows people to distinguish between devices with the same name. For example, my sndstat is:

Installed devices:
pcm0: <NVIDIA (0x0083) (HDMI/DP 8ch)> (play)
pcm1: <NVIDIA (0x0083) (HDMI/DP 8ch)> (play)
pcm2: <NVIDIA (0x0083) (HDMI/DP 8ch)> (play)
pcm3: <NVIDIA (0x0083) (HDMI/DP 8ch)> (play)
pcm4: <Realtek (0x1168) (Rear Analog 5.1/2.0)> (play/rec) default
pcm5: <Realtek (0x1168) (Front Analog)> (play/rec)
pcm6: <Realtek (0x1168) (Rear Digital)> (play)
No devices installed from userspace.

The previous format cannot distinguish between pcm0-pcm3. The new version of device names look like "pcm3: NVIDIA (0x0083) (HDMI/DP 8ch)", so people can distinguish between them.
Comment 2 Xin LI freebsd_committer 2020-03-29 19:17:07 UTC
I've tried the 2nd version of patch and it's a big improvement to the current version.
Comment 3 Jan Beich freebsd_committer 2020-03-29 20:43:19 UTC
Indeed. With the patch getUserMedia menu for microphone in Firefox changes from listing something like "/dev/dsp5" to "pcm5: <Realtek (0x1168) (Front Analog)>".

I'm not familar with the code to review. Maybe lightside can help or just submit upstream.
Comment 4 lightside 2020-03-29 22:34:58 UTC
Hello.

(In reply to Jan Beich from comment #3)
> I'm not familar with the code to review. Maybe lightside can help or just
> submit upstream.
I didn't test proposed patch, but used cases for parsing of /dev/sndstat output in attachment #212838 [details] is somehow similar to what I used for bug 211684. I guess, there is a need to mention about possibility of "hw.snd.verbose > 0" usage, which you did in bug 211684 comment #17.
Comment 5 lightside 2020-03-30 07:00:21 UTC
Created attachment 212854 [details]
Proposed patch (since 516738 revision, v1)

I created some testcase:
-8<--
#include <stdio.h>
#include <string.h>

int main() {
	const char *line = "pcm4: <Realtek (0x1168) (Rear Analog 5.1/2.0)> (play/rec) default";
	int device;

	if (sscanf(line, "pcm%i: ", &device) != 1)
		return 0;
	{
		char *k = strchr(line, ':');
		k++;
		k += strspn(k, " <");
		char *e = strrchr(k, '>');

		if (!e)
			return 0;

		const size_t len = strlen(k) - strlen(e);
		char m[len];
		strncpy(m, k, len);
	
		printf("pcm%d: %s\n", device, m);
		// pcm4: Realtek (0x1168) (Rear Analog 5.1/2.0)
	}
	{
		char *k = strrchr(line, '>');

		if (!k)
			return 0;

		const size_t len = strlen(line) - strlen(k) + 1;
		char m[len];
		strncpy(m, line, len);

		printf("%s\n", m);
		// pcm4: <Realtek (0x1168) (Rear Analog 5.1/2.0)>
	}

	return 0;
}
-->8-

which outputs two variants for "pcm4: <Realtek (0x1168) (Rear Analog 5.1/2.0)> (play/rec) default" line:
-8<--
pcm4: Realtek (0x1168) (Rear Analog 5.1/2.0)
pcm4: <Realtek (0x1168) (Rear Analog 5.1/2.0)>
-->8-

Attached patch for first variant.
Comment 6 lightside 2020-03-30 07:02:10 UTC
Created attachment 212855 [details]
Proposed patch (since 516738 revision, v2)

(In reply to comment #5)
Attached patch for second variant.
Please test and/or propose other solution.
Comment 7 lightside 2020-03-30 23:52:50 UTC
Created attachment 212875 [details]
Testcase

(In reply to comment #5)
Attached another (more correct) test case.

The parsing is based on the following source code:
https://github.com/freebsd/freebsd/blob/ad355b0a9dbd6a8aabe7c081a731d24904a0f2c1/sys/dev/sound/pcm/sndstat.c#L362-L367
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/b809e1148793bbe33af2adb09af395668a8dae22/sys/dev/sound/pcm/sndstat.c#L390-L407

If I'm not wrong, there are other possible audio device types, other than "pcm", such as "midi" and "sequencer": 
https://github.com/freebsd/freebsd/blob/ad355b0a9dbd6a8aabe7c081a731d24904a0f2c1/sys/dev/sound/pcm/sndstat.c#L243-L257
https://github.com/freebsd/freebsd/blob/ad355b0a9dbd6a8aabe7c081a731d24904a0f2c1/sys/dev/sound/pci/hda/hdaa.c#L7151-L7152
https://github.com/freebsd/freebsd/blob/1537078d8f2e62e82de3c08bdcae0fd79dc35a4a/sys/dev/sound/pci/emu10kx-midi.c#L247-L248
https://github.com/freebsd/freebsd/blob/ad355b0a9dbd6a8aabe7c081a731d24904a0f2c1/sys/dev/sound/midi/sequencer.c#L110-L118

https://github.com/DragonFlyBSD/DragonFlyBSD/blob/b809e1148793bbe33af2adb09af395668a8dae22/sys/dev/sound/pcm/sndstat.c#L289-L303
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/ed183f8c2f9bb14cbccb8377f3cdd29e0971d8a0/sys/dev/sound/pci/hda/hdaa.c#L7177-L7178
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/17975de10935ff2c43fe8eb641c048974acb07c0/sys/dev/sound/pci/emu10kx-midi.c#L243-L244
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/d147c94391cf5cc415970d2b885fcc931026c34e/sys/dev/sound/midi/sequencer.c#L108-L109

Therefore, I also added some format specifiers for sscanf function:
https://www.cplusplus.com/reference/cstdio/sscanf/
https://www.cplusplus.com/reference/cstdio/scanf/
Comment 8 lightside 2020-03-31 00:07:43 UTC
Created attachment 212876 [details]
Testcase

Cosmetic fixes, i.e. converted line endings to LF.
Comment 9 lightside 2020-03-31 00:40:03 UTC
Created attachment 212877 [details]
Proposed patch (since 516738 revision, v1)

Attached patch for first variant, e.g. "pcm4: Realtek (0x1168) (Rear Analog 5.1/2.0)".
Comment 10 lightside 2020-03-31 00:41:26 UTC
Created attachment 212878 [details]
Proposed patch (since 516738 revision, v2)

Attached patch for second variant, e.g. "pcm4: <Realtek (0x1168) (Rear Analog 5.1/2.0)>".

Please test and/or propose other solution.
Comment 11 lightside 2020-04-03 06:35:08 UTC
Created attachment 213004 [details]
Testcase

(In reply to comment #7)
Probably idea about other than "pcm" audio devices for this pulseaudio case was wrong.
Proposed other testcase and patches.
Comment 12 lightside 2020-04-03 06:35:53 UTC
Created attachment 213005 [details]
Proposed patch (since 516738 revision, v1)
Comment 13 lightside 2020-04-03 06:36:53 UTC
Created attachment 213006 [details]
Proposed patch (since 516738 revision, v2)
Comment 14 Greg V 2020-04-03 11:40:38 UTC
Hi, I'm working on an upstream merge request: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/277

I've included a solution for this that hopefully doesn't break Linux OSS or other BSDs (?), and does not use VLAs (variable-length arrays, which are not standard C) like `char desc[len + 1]` in attachment 213006 [details]:

https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/commit/94a781060e64cb871c0f2d3ee768123dd904f0a8?merge_request_iid=277
Comment 15 lightside 2020-04-03 18:08:33 UTC
Created attachment 213025 [details]
Testcase 2

Hello, Greg V.

(In reply to Greg V from comment #14)
> I've included a solution for this that hopefully doesn't break Linux OSS or
> other BSDs (?), and does not use VLAs (variable-length arrays, which are not
> standard C) like `char desc[len + 1]` in attachment 213006 [details]
Probably you are right, if VLAs is optional in C11, and PulseAudio  changed used C standard from C99 to C11:
https://github.com/pulseaudio/pulseaudio/blob/9562e898ddbfe61b29a9914cb7378bb0f01cc527/NEWS#L265
Changed the C standard from C99 to C11.
https://github.com/pulseaudio/pulseaudio/blob/v13.0/configure.ac#L172
AX_CHECK_COMPILE_FLAG([-std=gnu11],

But it compiled ok with using base Clang compiler on FreeBSD, in my case.

(In reply to Greg V from comment #14)
> I'm working on an upstream merge request:
> https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/277
As about your proposed code for upstreaming, the strcspn function doesn't search in reverse for '>' character. Maybe this is not so important, but attached testcase may show this for "Testcase 1" and how I tried to fix it with using strrchr function in "Testcase 2":
-8<--
char line[] = "pcm4: <Realtek (0x1168) (Rear Analog 5.1/2.0)> (play/rec) default";
Testcase 1:
4 - Realtek (0x1168) (Rear Analog 5.1/2.0)
Testcase 2:
4 - Realtek (0x1168) (Rear Analog 5.1/2.0)

char line[] = "pcm4: <Realtek (0x1168) <#> (Rear Analog 5.1/2.0)> (play/rec) default";
Testcase 1:
4 - Realtek (0x1168) <#
Testcase 2:
4 - Realtek (0x1168) <#> (Rear Analog 5.1/2.0)
-->8-
Comment 16 lightside 2020-04-03 18:10:11 UTC
Created attachment 213026 [details]
Proposed patch (since 516738 revision, v3)

Attached patch for third variant, based on comment #15 and Greg V proposal on:
https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/commit/94a781060e64cb871c0f2d3ee768123dd904f0a8?merge_request_iid=277

I also modified output format, based on Henry Hu proposal in attachment #212838 [details], i.e.
from "pa_snprintf(name, l, "%d - %s", device, k);" to "pa_snprintf(name, l, "%d: %s", device, k);". But I guess, developers may decide about what to use in this case.

Thanks for attention.
Comment 17 lightside 2020-04-03 18:18:31 UTC
Comment on attachment 213025 [details]
Testcase 2

Renamed file name of "Testcase 2" to testcase2.c, just in case.
Comment 18 lightside 2020-04-04 11:10:55 UTC
Created attachment 213050 [details]
Proposed patch (since 516738 revision, v3)

(In reply to comment #16)
Changed "k[0]" to "*k".
Comment 19 lightside 2020-04-04 11:33:38 UTC
Created attachment 213051 [details]
Proposed patch (since 516738 revision, v3)

(In reply to comment #18)
Simplified algorithm.
Comment 20 lightside 2020-04-04 11:43:03 UTC
Created attachment 213052 [details]
Testcase 3

Attached testcase for attachment #213051 [details], just in case.