sound: Shorten channel names

[christos]

Summary

The current channel naming convention is:
pcmX:[virtual_]play|record:dspX.[v]p|rY

• pcmX and dspX share the same unit numbers
• "[v]p|r" gives us the same information as "[virtual_]play|record"

Remove the redundant information, so that the channel names become more
readable: dspX.[virtual_]play|record.Y

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Test Plan

pcm0: <Dummy Audio Device> on nexus0 (1p:1v/1r:1v) default
        snddev flags=0x2e6<AUTOVCHAN,SOFTPCMVOL,BUSY,MPSAFE,REGISTERED,VPC>
        [dsp0.play.0]: spd 48000, fmt 0x00200010, flags 0x00002100, 0x00000004
                interrupts 0, underruns 0, feed 0, ready 0 [b:2048/1024/2|bs:2048/1024/2]
                channel flags=0x2100<BUSY,HAS_VCHAN>
                {userland} -> feeder_mixer(0x00200010) -> {hardware}
        dsp0.play.0[dsp0.virtual_play.0]: spd 8000, fmt 0x00100008, flags 0x10000000, 0x00000000
                interrupts 0, underruns 0, feed 0, ready 0 [b:0/0/0|bs:0/0/0]
                channel flags=0x10000000<VIRTUAL>
                {userland} -> feeder_root(0x00000000) -> {dsp0.play.0}
        [dsp0.record.0]: spd 48000, fmt 0x00200010, flags 0x00002100, 0x00000005
                interrupts 0, overruns 0, feed 0, hfree 2048, sfree 2048 [b:2048/1024/2|bs:2048/1024/2]
                channel flags=0x2100<BUSY,HAS_VCHAN>
                {hardware} -> feeder_root(0x00200010) -> feeder_mixer(0x00200010) -> {userland}
        dsp0.record.0[dsp0.virtual_record.0]: spd 8000, fmt 0x00100008, flags 0x10000000, 0x00000000
                interrupts 0, overruns 0, feed 0, hfree 0, sfree 0 [b:0/0/0|bs:0/0/0]
                channel flags=0x10000000<VIRTUAL>
                {dsp0.record.0} -> feeder_root(0x00000000) -> {userland}
No devices installed from userspace.

[dev_submerge.ch]

This makes sysctl hw.snd.verbose=2; cat /dev/sndstat harder to read, and we lose the ability to grep for play, record and virtual there. Also I think this is shown verbatim in GUIs like VLC, do we expect normal users to understand the subtle notion of [v]p|r?

[christos]

In D46836#1067926, @dev_submerge.ch wrote:

This makes sysctl hw.snd.verbose=2; cat /dev/sndstat harder to read, and we lose the ability to grep for play, record and virtual there. Also I think this is shown verbatim in GUIs like VLC, do we expect normal users to understand the subtle notion of [v]p|r?

I made this patch explicitly to make /dev/sndstat more readable, since we have a more concise name without repeating information. :-) I will agree on the grepping issue however. Even if we do not follow this particular style, I still think we have to find a better naming scheme. Perhaps dspX.[virtual_]record|play.Y or something like this would still be better than what we have now.

[dev_submerge.ch]

If I'm not mistaken, this name field corresponds to the name entry in the SNDCTL_AUDIOINFO ioctl. OSSv4 defines it as follows:

A string that contains the "full" description of the device.

This sounds like it should be presentable to normal users and help them to identify the device.
Could you verify that this string is actually used in GUIs like VLC? I don't have Xorg running on CURRENT.

Regarding sndstat I don't care too much. But I think another indent level per channel would do wonders for readability. Subjectively ;-)

[christos]

In D46836#1067943, @dev_submerge.ch wrote:

If I'm not mistaken, this name field corresponds to the name entry in the SNDCTL_AUDIOINFO ioctl. OSSv4 defines it as follows:

A string that contains the "full" description of the device.

This sounds like it should be presentable to normal users and help them to identify the device.
Could you verify that this string is actually used in GUIs like VLC? I don't have Xorg running on CURRENT.

I tried installing VLC and it crashes right away... Strange. In any case, the naming scheme I proposed in the previous comment is more readable anyway IMHO.

Regarding sndstat I don't care too much. But I think another indent level per channel would do wonders for readability. Subjectively ;-)

I second that. :) D46857

[christos]

New naming scheme: dspX.[virtual_]play|record.Y

[dev_submerge.ch]

I don't care too much about the device names, but I think we should conceptually separate them from what we display to the end user. As a future goal we may want to have something more elaborate in the OSS audio info name field, e.g. dsp.virtual_play.1 <Dummy Audio Device>. And for /dev/sndstat we could also add the "play", "record" and "virtual" in the buffer printing there, without changing the device names.

sys/dev/sound/pcm/channel.c

1238

Doesn't that make the name field completely redundant, with dsp_unit2name() available everywhere?

[christos]

In D46836#1069273, @dev_submerge.ch wrote:

I don't care too much about the device names, but I think we should conceptually separate them from what we display to the end user. As a future goal we may want to have something more elaborate in the OSS audio info name field, e.g. dsp.virtual_play.1 <Dummy Audio Device>.

I agree with printing something more elaborate, although I'd prefer a different naming scheme than this one. But we can discuss it in the future. :-)

And for /dev/sndstat we could also add the "play", "record" and "virtual" in the buffer printing there, without changing the device names.

I don't think I follow.

sys/dev/sound/pcm/channel.c

1238

Yes and no. I think it'd be better to move dsp_unit2name() to channel.c, or simply merge it with chn_init(). Since 25723d66369f dsp_unit2name() no longer creates names for actual devices, but for channels, so IMHO this functionality is no longer dsp-related. For this change to work, we'd need to expose dsp_cdevs[] to a header file (dsp_clone() uses this array as well) but I don't think that's a big issue anyway.

[dev_submerge.ch]

In D46836#1069394, @christos wrote:

In D46836#1069273, @dev_submerge.ch wrote:

I don't care too much about the device names, but I think we should conceptually separate them from what we display to the end user. As a future goal we may want to have something more elaborate in the OSS audio info name field, e.g. dsp.virtual_play.1 <Dummy Audio Device>.

I agree with printing something more elaborate, although I'd prefer a different naming scheme than this one. But we can discuss it in the future. :-)

You are changing the content of the name field here. We have to assess where and what it is used for. As I try to explain, the different use cases may demand different formatting and content, thus I want to know what we cater to here.

And for /dev/sndstat we could also add the "play", "record" and "virtual" in the buffer printing there, without changing the device names.

I don't think I follow.

We don't necessarily have to change the device names, or the name field content. For /dev/sndstat we could just change the formatting there, same goes for the OSS audio info.

[zlei]

@christos You tagged me to review, but I do not have strong opinion for this change, really :)

[christos]

In D46836#1069907, @dev_submerge.ch wrote:

In D46836#1069394, @christos wrote:

In D46836#1069273, @dev_submerge.ch wrote:

I don't care too much about the device names, but I think we should conceptually separate them from what we display to the end user. As a future goal we may want to have something more elaborate in the OSS audio info name field, e.g. dsp.virtual_play.1 <Dummy Audio Device>.

I agree with printing something more elaborate, although I'd prefer a different naming scheme than this one. But we can discuss it in the future. :-)

You are changing the content of the name field here. We have to assess where and what it is used for. As I try to explain, the different use cases may demand different formatting and content, thus I want to know what we cater to here.

From a quick GitHub search on SNDCTL_ENGINEINFO (which is what will populate oss_audioinfo with the channel name), what I see is that this field is not used much, and where it is used, it's not only for printing it later on, so there shouldn't be any problem. Note that up until e07f9178502b, SNDCTL_AUDIOINFO and SNDCTL_ENGINEINFO were the same thing, so I think that if there was a breakage to happen by changing the name field, we'd have run into trouble already. Also, the change I'm proposing includes the same information in the channel's name, just with less redundancy.

[dev_submerge.ch]

In D46836#1073435, @christos wrote:

In D46836#1069907, @dev_submerge.ch wrote:

In D46836#1069394, @christos wrote:

In D46836#1069273, @dev_submerge.ch wrote:

I don't care too much about the device names, but I think we should conceptually separate them from what we display to the end user. As a future goal we may want to have something more elaborate in the OSS audio info name field, e.g. dsp.virtual_play.1 <Dummy Audio Device>.

I agree with printing something more elaborate, although I'd prefer a different naming scheme than this one. But we can discuss it in the future. :-)

You are changing the content of the name field here. We have to assess where and what it is used for. As I try to explain, the different use cases may demand different formatting and content, thus I want to know what we cater to here.

From a quick GitHub search on SNDCTL_ENGINEINFO (which is what will populate oss_audioinfo with the channel name), what I see is that this field is not used much, and where it is used, it's not only for printing it later on, so there shouldn't be any problem. Note that up until e07f9178502b, SNDCTL_AUDIOINFO and SNDCTL_ENGINEINFO were the same thing, so I think that if there was a breakage to happen by changing the name field, we'd have run into trouble already. Also, the change I'm proposing includes the same information in the channel's name, just with less redundancy.

I'm not against any changes. But I'd like you to clarify the following points:

1. What is the intended meaning of the "name" field? It was sort of descriptive before, but now you're tying it to the device path.
2. What are the implications of changing the name field? It's not just sndstat, where is it used?
3. Your update of this patch also changes the device paths. What are the consequences of that, does it affect applications?

[christos]

In D46836#1073676, @dev_submerge.ch wrote:

1. What is the intended meaning of the "name" field? It was sort of descriptive before, but now you're tying it to the device path.

As mentioned before, the meaning remains exactly the same. pcmX and dspX have a 1:1 relationship, so we do not omit any information.

1. What are the implications of changing the name field? It's not just sndstat, where is it used?

I am 99% sure there are no implications at all. On GitHub I only see some small projects using this field through SNDCTL_ENGINEINFO, but their use does not require this current naming scheme (or a specific naming scheme in general) we have.

1. Your update of this patch also changes the device paths. What are the consequences of that, does it affect applications?

It does not change any device paths; the only device paths we have are /dev/dspX. The dsp_cdevs[] array no longer corresponds to device paths, since we do not create a device for each channel anymore. The name "cdevs" is a leftover from the old use of this array. dsp_cdevs[] is now only used to create channel names in dsp_unit2name(), or to match any of the OSSv4 aliases in dsp_clone()

[dev_submerge.ch]

In D46836#1074859, @christos wrote:

In D46836#1073676, @dev_submerge.ch wrote:

1. What is the intended meaning of the "name" field? It was sort of descriptive before, but now you're tying it to the device path.

As mentioned before, the meaning remains exactly the same.

The "name" field was meant as a descriptive label before, now it's a copy of an internal identifier that looks like a device path but isn't. It's not obvious to me whether you want to unify it with dsp_unit2name() everywhere or not. And without knowing all uses it's hard to tell whether that's a good idea.

pcmX and dspX have a 1:1 relationship, so we do not omit any information.

That's a (separate) pain point, I don't see that 1:1 relationship documented anywhere. The dspX don't appear in dmesg, man dsp doesn't show sound(4), cat /dev/sndstat doesn't print them by default, the channel settings are under sysctl dev.pcm.

1. What are the implications of changing the name field? It's not just sndstat, where is it used?

I am 99% sure there are no implications at all. On GitHub I only see some small projects using this field through SNDCTL_ENGINEINFO, but their use does not require this current naming scheme (or a specific naming scheme in general) we have.

That wasn't my question. AFAICT the name field ends up in SNDCTL_ENGINEINFO and SNDCTL_DSP_GET_PLAYTGT_NAMES ioctls, the sndstat print and nvlist interfaces, debug messages and internal labels of mutexes and buffers. Did you check all these?

1. Your update of this patch also changes the device paths. What are the consequences of that, does it affect applications?

It does not change any device paths; the only device paths we have are /dev/dspX. The dsp_cdevs[] array no longer corresponds to device paths, since we do not create a device for each channel anymore. The name "cdevs" is a leftover from the old use of this array. dsp_cdevs[] is now only used to create channel names in dsp_unit2name(), or to match any of the OSSv4 aliases in dsp_clone()

Ok, got it. The implementation and uses of dsp_cdevs[] and dsp_unit2name() are a bit confusing at the moment.

[christos]

In D46836#1075056, @dev_submerge.ch wrote:

In D46836#1074859, @christos wrote:

In D46836#1073676, @dev_submerge.ch wrote:

1. What is the intended meaning of the "name" field? It was sort of descriptive before, but now you're tying it to the device path.

As mentioned before, the meaning remains exactly the same.

The "name" field was meant as a descriptive label before, now it's a copy of an internal identifier that looks like a device path but isn't. It's not obvious to me whether you want to unify it with dsp_unit2name() everywhere or not. And without knowing all uses it's hard to tell whether that's a good idea.

I think we are missing the point a bit. The only change this patch brings is that it omits redundant information; the label is still equally descriptive. I do not get what the point of contention is exactly...

pcmX and dspX have a 1:1 relationship, so we do not omit any information.

That's a (separate) pain point, I don't see that 1:1 relationship documented anywhere. The dspX don't appear in dmesg, man dsp doesn't show sound(4), cat /dev/sndstat doesn't print them by default, the channel settings are under sysctl dev.pcm.

I agree completely. In fact there was a discussion regarding this confusion a couple of days ago. I think as a future goal the naming should be unified to either "dsp", "pcm" or "snd".

1. What are the implications of changing the name field? It's not just sndstat, where is it used?

I am 99% sure there are no implications at all. On GitHub I only see some small projects using this field through SNDCTL_ENGINEINFO, but their use does not require this current naming scheme (or a specific naming scheme in general) we have.

That wasn't my question. AFAICT the name field ends up in SNDCTL_ENGINEINFO and SNDCTL_DSP_GET_PLAYTGT_NAMES ioctls, the sndstat print and nvlist interfaces, debug messages and internal labels of mutexes and buffers. Did you check all these?

I did check, and this field in OSS applications is largely not used at all, and in the few cases that it is, it's just printed as-is. I cannot find any application that expects the "name" field to be in a specific format. Debug messages, mutexes and buffers shouldn't matter because, as mentioned multiple times, the meaning this scheme conveys is exactly the same as before, so it's not like we are going to hide any information with this patch.

1. Your update of this patch also changes the device paths. What are the consequences of that, does it affect applications?

It does not change any device paths; the only device paths we have are /dev/dspX. The dsp_cdevs[] array no longer corresponds to device paths, since we do not create a device for each channel anymore. The name "cdevs" is a leftover from the old use of this array. dsp_cdevs[] is now only used to create channel names in dsp_unit2name(), or to match any of the OSSv4 aliases in dsp_clone()

Ok, got it. The implementation and uses of dsp_cdevs[] and dsp_unit2name() are a bit confusing at the moment.

I'll address this in a future patch.

[dev_submerge.ch]

In D46836#1075170, @christos wrote:

In D46836#1075056, @dev_submerge.ch wrote:

The "name" field was meant as a descriptive label before, now it's a copy of an internal identifier that looks like a device path but isn't. It's not obvious to me whether you want to unify it with dsp_unit2name() everywhere or not. And without knowing all uses it's hard to tell whether that's a good idea.

I think we are missing the point a bit. The only change this patch brings is that it omits redundant information; the label is still equally descriptive. I do not get what the point of contention is exactly...

Yeah, this is somewhat bikesheddy, I see neither strong reason for nor against this change. In it's current form the redundancy it omits is minuscule, and it duplicates dsp_unit2name(), which is why I expected some plan about when to use one or the other. Alas, the least I wanted to make sure is that it isn't shown to users in a context where they could try to parse it, or where the similarity to device paths would be confusing.

pcmX and dspX have a 1:1 relationship, so we do not omit any information.

That's a (separate) pain point, I don't see that 1:1 relationship documented anywhere. The dspX don't appear in dmesg, man dsp doesn't show sound(4), cat /dev/sndstat doesn't print them by default, the channel settings are under sysctl dev.pcm.

I agree completely. In fact there was a discussion regarding this confusion a couple of days ago. I think as a future goal the naming should be unified to either "dsp", "pcm" or "snd".

That's an interesting thread, thanks! Actually, having a 1:n relationship would make sense for certain sound cards, but that's not a priority. What I think would be really helpful is a unique and persistent device path scheme for sound cards / channels. Something like /dev/audio/hda-HDMI or whatever is feasible, maybe as an alias to the current /dev/dspX device paths.

That wasn't my question. AFAICT the name field ends up in SNDCTL_ENGINEINFO and SNDCTL_DSP_GET_PLAYTGT_NAMES ioctls, the sndstat print and nvlist interfaces, debug messages and internal labels of mutexes and buffers. Did you check all these?

I did check, and this field in OSS applications is largely not used at all, and in the few cases that it is, it's just printed as-is. I cannot find any application that expects the "name" field to be in a specific format. Debug messages, mutexes and buffers shouldn't matter because, as mentioned multiple times, the meaning this scheme conveys is exactly the same as before, so it's not like we are going to hide any information with this patch.

That wasn't obvious from the commit message or your comments, hence my question where that name is used and what is affected. Could have just answered that. Same goes for dsp_cdevs[] / dsp_unit2name().

Anyway, no harm done, go ahead.