At Tue, 21 Aug 2012 13:24:53 +0200, David Henningsson wrote:
On 08/21/2012 12:31 PM, Takashi Iwai wrote:
Hi,
this is a progress report of my longstanding TODO, the channel map API implementation. I'm going to cover this at Plumbers audio uconf, so we can discuss details there, too.
The channel mapping API provides a method for user-space to query, get and set the channel map of a PCM stream. It's required for assigning channels properly for multi-channel streams.
- KERNEL IMPLEMENTATION
In my latest attempt, I implemented with control elements. A control element is created for each PCM substream with the corresponding device and substream index. Then it gives a TLV for querying maps, a read op for obtaining the current map, and optionally a write op for setting the map. The obvious merit by this way is that no extra kernel ABI is required.
A couple of new helper functions are provided for assigning standard channel maps. Currently, HD-audio and AC97 drivers has some implementation.
- ALSA-LIB IMPLEMENTATION
The additional alsa-lib API functions look like:
int **snd_pcm_query_chmaps(snd_pcm_t *pcm); int *snd_pcm_get_chmap(snd_pcm_t *pcm); int snd_pcm_set_chmap(snd_pcm_t *pcm, const int *map);
snd_pcm_query_chmaps() returns the list of channel maps. A channel map is represented by an integer array, beginning with the channel map type, followed by the number of channels, and the position of each channel, e.g. { SND_CHMAP_FIXED, 4, SND_CHMAP_FL, SND_CHMAP_FR, SND_CHMAP_RL, SND_CHMAP_RR }
snd_pcm_get_chmap() returns the currently assigned channel map for the given PCM stream. If the PCM is before prepared, it fills UNKNOWN.
When a driver allows user to change the channel map, user can call snd_pcm_set_chmap(). For example, HDMI allows you to choose whether it's 4.0 or 3.1 output.
Interesting. A few thoughts here:
- You seem to have invented new constants for different channels
(SND_CHMAP_FL for "front left" etc). There is already a channel enumeration in include/mixer.h, _snd_mixer_selem_channel_id. Is there a reason we can't just reuse it for this purpose?
No big reason, but a minor reason would be that UNKNOWN = -1 is a bit impractical and the mixer id definition lacks some new positions. And, this is for mixer, and another for PCM, so using SND_MIXER_* doesn't sound right.
But adjusting to follow the same values won't be a big issue. Let's see.
If we can't, we should at least make sure that everywhere we have these declarations, they always correspond to the same value and add comments that they need to be kept in sync.
- a chmap struct would probably be more elegant, something like:
struct snd_chmap { int type; /* or snd_pcm_chmap_type? */ int count; int channels[SND_MIXER_SCHN_LAST+1]; /* Or variable length? */ }
It should be a variable length. It could be with zero-size array, though: struct snd_chmap { int type; int count; int channels[0]; };
Though, the type would be used only for queries (see below), so it should be named like struct snd_chmap_query.
- Can you use snd_pcm_set_chmap before snd_pcm_prepare, and if so, it
seems a bit strange that snd_pcm_get_chmap does not return what you have previously set.
Setting or getting the channel map doesn't work before prepare (or more specifically hw_params) because the number of channels isn't determined yet. It's the only reason.
- The question of memory allocation for _get and _query, who is
supposed to allocate and free the snd_chmap array?
The caller. The helper function snd_pcm_free_chmaps() is provided, too.
- I'm not sure I'm getting the _VAR and _PAIRED types, even though I
read the documentation. Is this for the query only? Or if not, what would it mean to get/set a channelmap with _VAR type?
Oh sorry, I must correct the description: get_chmap() returns only the number of channels followed by channel elements. Ditto for set_chmap(). The type is used only for query.
thanks,
Takashi