[alsa-devel] Getting pcm_usb_stream plugin to know its limits.

newer
[alsa-devel] M-Audio Fast Track...

michael＠cadilhac.name

30 Dec 2009 30 Dec '09

6:48 p.m.

Hi there everyone,

I've been fighting for a few hours with a USB sound device (namely, Tascam US-144), to get it to work with Audacity (this not being the relevant part).

At first, Audacity died in a painful segfault, without any explanation, when associated with the usb_stream plugin[1].

Audacity uses PortAudio, which checks for a few possible parameters of audio rate to find the list of accepted ones. It tries to set a parameter (snd_pcm_hw_params), and if it fails, closes the device, and continues its way.

Suppose the `prepare` act of the usb_stream is aborted because of wrong parameters. Then, PortAudio tries to close the device, by calling snd_pcm_close. This in turn calls snd_pcm_drop, which calls the fast_op drop (that of ioplug), which is snd_pcm_ioplug_drop, which then calls the *stop* callback of the usb_stream (snd_pcm_us_stop). The stream not being properly initialized, us->uus->write_area is nil, and the memset fails.

This is the modification I did for that purpose :

Next, Audacity stopped segfaulting but still didn't work: it couldn't manage to configure the plugin properly. Here's how PortAudio searches for an acceptable audio rate. For each possible sample rate, it gets a full hw_params for the device: snd_pcm_hw_params_any( pcm, hwParams ); then set the sample rate to the hwParams. It then tries to set those params to the device: snd_pcm_hw_params( pcm, hwParams );

The period_size parameter is left untouched, and by default, the pcm inherits of a high value (maybe the highest). Now the problem is the following. pcm_usb_stream says that the maximum accepted period_size is 64*4096: (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_PERIOD_BYTES, 128, 64*4096)

BUT the us122l driver[2] (from 2.6.32) explicitly prohibits period_size greater than 0x3000:

if ((cfg->sample_rate != 44100 && cfg->sample_rate != 48000 && (!high_speed || (cfg->sample_rate != 88200 && cfg->sample_rate != 96000))) || cfg->frame_size != 6 || cfg->period_frames > 0x3000) err = -EINVAL;

This is now what happens in PortAudio: PortAudio tries each and every sound rate with the "default" period_size, this being greater than 0x3000. It follows that no sound rate will be accepted, because of the period_size, and the device fails to initialize.

I'm not sure whose fault is this, and for the time being, I just did the following:

Well, that's all folks. Happy new year to everyone.

Footnotes: [1] Configured as pcm.!usb_stream { type usb_stream card "1" } which works with a fine-tuned aplay.

[2] linux/sound/usb/usx2y/us122l.c

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || La culture, c'est comme l'amour. || Faut y aller à petits coups au début pour bien en jouir plus tard. || -- Pierre Desproges

Attachments:

memset.patch (text/x-patch — 392 bytes)
period_size.patch (text/x-patch — 511 bytes)

Show replies by date

michael＠cadilhac.name

30 Dec 30 Dec

6:58 p.m.

michael@cadilhac.name (Michaël Cadilhac) writes:

...

--- pcm_usb_stream.c 2008-11-21 18:08:16.000000000 -0500 +++ pcm_usb_stream.c 2009-12-30 12:36:00.654452577 -0500 @@ -258,7 +258,7 @@ snd_pcm_us_t *us = io->private_data; VDBG("%u", us->uus->s->periods_done);

if (io->stream == SND_PCM_STREAM_PLAYBACK)

if (io->stream == SND_PCM_STREAM_PLAYBACK && us->uus->s) memset(us->uus->write_area, 0, us->uus->s->write_size);

return 0;

Just noticed that it would be better phrased as:

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || Un certain Blaise Pascal || etc... etc... || -- Prévert (Les paris stupides)

michael＠cadilhac.name

31 Dec 31 Dec

5:15 a.m.

michael@cadilhac.name (Michaël Cadilhac) writes:

...

Next, Audacity stopped segfaulting but still didn't work: it couldn't manage to configure the plugin properly.

My second idea was to allow usb_stream to fix its own rate and period_size, as in:

pcm.!usb_stream { type usb_stream period_size 128 rate 44100 card "1" }

This would be great because it would prevent PortAudio from doing lots of test (which are pretty slow). This is what I thought would work:

Basically, I tweaked the argument parsing to accept "rate" and "period_size", stored them in the snd_pcm_us_t, and used them in us_set_hw_constraint. It looks to work great with the rate, but when I set the period_size, aplay can't find a valid configuration (indeed, snd_pcm_hw_params_any(handle, params); returns an error).

Any help appreciated again!

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || Free jazz is the Vegemite of the the musical world. || It's an aquired taste. || -- Marten, QC

michael＠cadilhac.name

5:57 a.m.

New subject: [alsa-devel] Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

michael@cadilhac.name (Michaël Cadilhac) writes:

...

michael@cadilhac.name (Michaël Cadilhac) writes:

...
Next, Audacity stopped segfaulting but still didn't work: it couldn't manage to configure the plugin properly.

My second idea was to allow usb_stream to fix its own rate and period_size, as in:

pcm.!usb_stream { type usb_stream period_size 128 rate 44100 card "1" }

This would be great because it would prevent PortAudio from doing lots of test (which are pretty slow). This is what I thought would work:

[...]

...

unsigned rate_min = us->rate ? us->rate : 44100,

rate_max = us->rate ? us->rate : 96000,

period_size_min = us->period_size ? us->period_size : 128,

period_size_max = us->period_size ? us->period_size : 64*4096;

[...]

Ah! Sorry, I got mixed up between period_size and period_bytes (I'm new to all that stuff). Multiplying period_size by the frame_size (which is hardcoded as 6) get it to work.

So, my final saying is the following patch. It fixes the segfault on stopping a non-started usb_stream, plus it adds the ability to set a period size and sound rate. The latter offers a workaround for the second issue I came with (that the default values for period bytes, and hence period size, were too high for us122l to work).

Again, happy new year to everyone.

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || Emacs is GNU, Emacs is free || For more information, || Type C-h C-p

michael＠cadilhac.name

5 Jan 5 Jan

4:26 a.m.

New subject: [alsa-devel] Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

michael@cadilhac.name (Michaël Cadilhac) writes:

...

So, my final saying is the following patch. It fixes the segfault on stopping a non-started usb_stream, plus it adds the ability to set a period size and sound rate. The latter offers a workaround for the second issue I came with (that the default values for period bytes, and hence period size, were too high for us122l to work).

Any comment on that patch would be greatly appreciated. I'm just wondering if it's safe for me to give it to some people.

...

--- /dd/alsa-plugins/usb_stream/pcm_usb_stream.c 2008-11-21 18:08:16.000000000 -0500 +++ pcm_usb_stream.c 2009-12-30 23:40:46.467973740 -0500 @@ -48,6 +48,9 @@ #define VDBG(f, ...) #endif

+#define FRAME_SIZE 6

#define LCARD 32 struct user_usb_stream { char card[LCARD]; @@ -70,6 +73,8 @@ unsigned periods_done;

unsigned channels;

snd_pcm_uframes_t period_size;

unsigned int rate;

} snd_pcm_us_t;

static struct user_usb_stream *uus; @@ -177,7 +182,7 @@ VDBG("");

us_cfg.version = USB_STREAM_INTERFACE_VERSION;

us_cfg.frame_size = 6;

us_cfg.frame_size = FRAME_SIZE; us_cfg.sample_rate = io->rate; us_cfg.period_frames = io->period_size;

@@ -256,8 +261,11 @@ static int snd_pcm_us_stop(snd_pcm_ioplug_t *io) { snd_pcm_us_t *us = io->private_data;

VDBG("%u", us->uus->s->periods_done);
if (!us->uus->s)
 return 0;
VDBG("%u", us->uus->s->periods_done); if (io->stream == SND_PCM_STREAM_PLAYBACK) memset(us->uus->write_area, 0, us->uus->s->write_size);
@@ -370,6 +378,10 @@ };

int err;
unsigned int rate_min = us->rate ? us->rate : 44100,
rate_max = us->rate ? us->rate : 96000,
period_bytes_min = us->period_size ? FRAME_SIZE * us->period_size : 128,
period_bytes_max = us->period_size ? FRAME_SIZE * us->period_size : 64*4096;
if ((err = snd_pcm_ioplug_set_param_list(&us->io, SND_PCM_IOPLUG_HW_ACCESS, ARRAY_SIZE(access_list), access_list)) < 0 ||
@@ -378,19 +390,20 @@ (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_CHANNELS, us->channels, us->channels)) < 0 || (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_RATE,
				   44100, 96000)) < 0 ||
				   rate_min, rate_max)) < 0 ||
(err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_PERIOD_BYTES,
				   128, 64*4096)) < 0 ||
				   period_bytes_min, period_bytes_max)) < 0 ||
(err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_PERIODS, 2, 2)) < 0) return err;
return 0;

}

static int snd_pcm_us_open(snd_pcm_t **pcmp, const char *name, const char *card,
		   snd_pcm_stream_t stream, int mode)
	   snd_pcm_stream_t stream, int mode,
	   snd_pcm_uframes_t period_size,
	   unsigned int rate)
{ snd_pcm_us_t *us; int err; @@ -421,6 +434,8 @@ snd_hwdep_poll_descriptors(us->hwdep, &us->pfd, 1);

us->channels = 2;

us->period_size = period_size;

us->rate = rate;

us->io.version = SND_PCM_IOPLUG_VERSION; us->io.name = "ALSA <-> USB_STREAM PCM I/O Plugin";

@@ -455,6 +470,7 @@ snd_config_iterator_t i, next; const char *card; int err;

long period_size = 0, rate = 0;

snd_config_for_each(i, next, conf) { snd_config_t *n = snd_config_iterator_entry(i);

@@ -472,12 +488,27 @@ snd_config_get_string(n, &card); continue; }
if (strcmp(id, "period_size") == 0) {
	if (snd_config_get_type(n) != SND_CONFIG_TYPE_INTEGER) {
		SNDERR("Invalid type for %s", id);
		return -EINVAL;
	}
	snd_config_get_integer(n, &period_size);
	continue;
}
if (strcmp(id, "rate") == 0) {
	if (snd_config_get_type(n) != SND_CONFIG_TYPE_INTEGER) {
		SNDERR("Invalid type for %s", id);
		return -EINVAL;
	}
	snd_config_get_integer(n, &rate);
	continue;
}
SNDERR("Unknown field %s", id); return -EINVAL; }
err = snd_pcm_us_open(pcmp, name, card, stream, mode);

err = snd_pcm_us_open(pcmp, name, card, stream, mode, period_size, rate); return err;

}

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || <ESC>ape this <COLON> thing, || <Q>uit and do <NOT> <RET>urn. || -- VI

michael＠cadilhac.name

11 Jan 11 Jan

7:35 p.m.

New subject: [alsa-devel] Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

michael@cadilhac.name (Michaël Cadilhac) writes:

...

michael@cadilhac.name (Michaël Cadilhac) writes:

...
So, my final saying is the following patch. It fixes the segfault on stopping a non-started usb_stream, plus it adds the ability to set a period size and sound rate. The latter offers a workaround for the second issue I came with (that the default values for period bytes, and hence period size, were too high for us122l to work).

Any comment on that patch would be greatly appreciated. I'm just wondering if it's safe for me to give it to some people.

Anyone? Note that this also fixes a real bug, so it might be of interest to include it in the trunk.

...

...
--- /dd/alsa-plugins/usb_stream/pcm_usb_stream.c 2008-11-21 18:08:16.000000000 -0500 +++ pcm_usb_stream.c 2009-12-30 23:40:46.467973740 -0500 @@ -48,6 +48,9 @@ #define VDBG(f, ...) #endif

+#define FRAME_SIZE 6

#define LCARD 32 struct user_usb_stream { char card[LCARD]; @@ -70,6 +73,8 @@ unsigned periods_done;

unsigned channels;

snd_pcm_uframes_t period_size;

unsigned int rate;

} snd_pcm_us_t;

static struct user_usb_stream *uus; @@ -177,7 +182,7 @@ VDBG("");

us_cfg.version = USB_STREAM_INTERFACE_VERSION;

us_cfg.frame_size = 6;

us_cfg.frame_size = FRAME_SIZE; us_cfg.sample_rate = io->rate; us_cfg.period_frames = io->period_size;

@@ -256,8 +261,11 @@ static int snd_pcm_us_stop(snd_pcm_ioplug_t *io) { snd_pcm_us_t *us = io->private_data;

VDBG("%u", us->uus->s->periods_done);
if (!us->uus->s)
 return 0;
VDBG("%u", us->uus->s->periods_done); if (io->stream == SND_PCM_STREAM_PLAYBACK) memset(us->uus->write_area, 0, us->uus->s->write_size);
@@ -370,6 +378,10 @@ };

int err;
unsigned int rate_min = us->rate ? us->rate : 44100,
rate_max = us->rate ? us->rate : 96000,
period_bytes_min = us->period_size ? FRAME_SIZE * us->period_size : 128,
period_bytes_max = us->period_size ? FRAME_SIZE * us->period_size : 64*4096;
if ((err = snd_pcm_ioplug_set_param_list(&us->io, SND_PCM_IOPLUG_HW_ACCESS, ARRAY_SIZE(access_list), access_list)) < 0 ||
@@ -378,19 +390,20 @@ (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_CHANNELS, us->channels, us->channels)) < 0 || (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_RATE,
				   44100, 96000)) < 0 ||
				   rate_min, rate_max)) < 0 ||
(err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_PERIOD_BYTES,
				   128, 64*4096)) < 0 ||
				   period_bytes_min, period_bytes_max)) < 0 ||
(err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_PERIODS, 2, 2)) < 0) return err;
return 0;

}

static int snd_pcm_us_open(snd_pcm_t **pcmp, const char *name, const char *card,
		   snd_pcm_stream_t stream, int mode)
	   snd_pcm_stream_t stream, int mode,
	   snd_pcm_uframes_t period_size,
	   unsigned int rate)
{ snd_pcm_us_t *us; int err; @@ -421,6 +434,8 @@ snd_hwdep_poll_descriptors(us->hwdep, &us->pfd, 1);

us->channels = 2;

us->period_size = period_size;

us->rate = rate;

us->io.version = SND_PCM_IOPLUG_VERSION; us->io.name = "ALSA <-> USB_STREAM PCM I/O Plugin";

@@ -455,6 +470,7 @@ snd_config_iterator_t i, next; const char *card; int err;

long period_size = 0, rate = 0;

snd_config_for_each(i, next, conf) { snd_config_t *n = snd_config_iterator_entry(i);

@@ -472,12 +488,27 @@ snd_config_get_string(n, &card); continue; }
if (strcmp(id, "period_size") == 0) {
	if (snd_config_get_type(n) != SND_CONFIG_TYPE_INTEGER) {
		SNDERR("Invalid type for %s", id);
		return -EINVAL;
	}
	snd_config_get_integer(n, &period_size);
	continue;
}
if (strcmp(id, "rate") == 0) {
	if (snd_config_get_type(n) != SND_CONFIG_TYPE_INTEGER) {
		SNDERR("Invalid type for %s", id);
		return -EINVAL;
	}
	snd_config_get_integer(n, &rate);
	continue;
}
SNDERR("Unknown field %s", id); return -EINVAL; }
err = snd_pcm_us_open(pcmp, name, card, stream, mode);

err = snd_pcm_us_open(pcmp, name, card, stream, mode, period_size, rate); return err;

}

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || 21 seems like a good value: half of 42, || and just old enough to drink! Why would you want to change it? || -- Stefan Monnier

Takashi Iwai

12 Jan 12 Jan

10:52 a.m.

New subject: [alsa-devel] Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

At Mon, 11 Jan 2010 13:35:19 -0500, Michaël Cadilhac wrote:

...

michael@cadilhac.name (Michaël Cadilhac) writes:

...
michael@cadilhac.name (Michaël Cadilhac) writes:

...
So, my final saying is the following patch. It fixes the segfault on stopping a non-started usb_stream, plus it adds the ability to set a period size and sound rate. The latter offers a workaround for the second issue I came with (that the default values for period bytes, and hence period size, were too high for us122l to work).

Any comment on that patch would be greatly appreciated. I'm just wondering if it's safe for me to give it to some people.

Anyone? Note that this also fixes a real bug, so it might be of interest to include it in the trunk.

Through a quick glance, the patch looks OK to me. Could you give a proper subject and changelog to merge your patch to git tree?

thanks,

Takashi

michael＠cadilhac.name

2 Feb 2 Feb

6:24 a.m.

New subject: [alsa-devel] Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

Takashi Iwai tiwai@suse.de writes:

...

At Mon, 11 Jan 2010 13:35:19 -0500, Michaël Cadilhac wrote:

...
michael@cadilhac.name (Michaël Cadilhac) writes:

...
michael@cadilhac.name (Michaël Cadilhac) writes:

...
So, my final saying is the following patch. It fixes the segfault on stopping a non-started usb_stream, plus it adds the ability to set a period size and sound rate. The latter offers a workaround for the second issue I came with (that the default values for period bytes, and hence period size, were too high for us122l to work).

Any comment on that patch would be greatly appreciated. I'm just wondering if it's safe for me to give it to some people.

Anyone? Note that this also fixes a real bug, so it might be of interest to include it in the trunk.

Through a quick glance, the patch looks OK to me. Could you give a proper subject and changelog to merge your patch to git tree?

Hi there Takashi, and sorry for the delay. Here are the patch, subdivised in two distinct logical changes. I hope the format is what you asked for.

* usb_stream/pcm_usb_stream.c (snd_pcm_us_stop): Prevent dereferencing when structure is not initialized.

* usb_stream/pcm_usb_stream.c: Allow user-set period-size and rate.

I can write the according documentation, if needed.

Have a great day.

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || This .sig has been generated by Outlook Express 98 || $@(%@#%$*@(#)%$!%)(%$#(@#$%**!@#$+!#?% || and triple-checked on Windows Mail Vista.

michael＠cadilhac.name

10 Feb 10 Feb

11:56 p.m.

New subject: [alsa-devel] Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

michael@cadilhac.name (Michaël Cadilhac) writes:

...

Takashi Iwai tiwai@suse.de writes:

...
At Mon, 11 Jan 2010 13:35:19 -0500, Michaël Cadilhac wrote:

...
michael@cadilhac.name (Michaël Cadilhac) writes:

...
michael@cadilhac.name (Michaël Cadilhac) writes:

...
So, my final saying is the following patch. It fixes the segfault on stopping a non-started usb_stream, plus it adds the ability to set a period size and sound rate. The latter offers a workaround for the second issue I came with (that the default values for period bytes, and hence period size, were too high for us122l to work).

Any comment on that patch would be greatly appreciated. I'm just wondering if it's safe for me to give it to some people.

Anyone? Note that this also fixes a real bug, so it might be of interest to include it in the trunk.

Through a quick glance, the patch looks OK to me. Could you give a proper subject and changelog to merge your patch to git tree?

Hi there Takashi, and sorry for the delay. Here are the patch, subdivised in two distinct logical changes. I hope the format is what you asked for.

Pinging. IIUC, I forgot to include "subject" lines. The first one should be "usb_stream: Check for NULL-ness before dereferencing", and the second one "usb_stream: Allow user-set period-size and rate.".

-- Michaël `Micha' Cadilhac (LITQ, U. de Montréal) -- http://michael.cadilhac.name || Perhaps we should start making protest releases || before doing a fall release. || -- David Kastrup

Takashi Iwai

22 Feb 22 Feb

10:04 a.m.

New subject: [alsa-devel] Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

At Wed, 10 Feb 2010 17:56:26 -0500, Michaël Cadilhac wrote:

...

michael@cadilhac.name (Michaël Cadilhac) writes:

...
Takashi Iwai tiwai@suse.de writes:

...
At Mon, 11 Jan 2010 13:35:19 -0500, Michaël Cadilhac wrote:

...
michael@cadilhac.name (Michaël Cadilhac) writes:

...
michael@cadilhac.name (Michaël Cadilhac) writes:

...
So, my final saying is the following patch. It fixes the segfault on stopping a non-started usb_stream, plus it adds the ability to set a period size and sound rate. The latter offers a workaround for the second issue I came with (that the default values for period bytes, and hence period size, were too high for us122l to work).

Any comment on that patch would be greatly appreciated. I'm just wondering if it's safe for me to give it to some people.

Anyone? Note that this also fixes a real bug, so it might be of interest to include it in the trunk.

Through a quick glance, the patch looks OK to me. Could you give a proper subject and changelog to merge your patch to git tree?

Hi there Takashi, and sorry for the delay. Here are the patch, subdivised in two distinct logical changes. I hope the format is what you asked for.

Pinging. IIUC, I forgot to include "subject" lines. The first one should be "usb_stream: Check for NULL-ness before dereferencing", and the second one "usb_stream: Allow user-set period-size and rate.".

Thanks, applied now.

But, at the next time, please make each patch as directly applicable. That is, put subject and from lines to the patch, together with the patch description (and preferably with your sign-off) embedded to the patch itself. That is, the same style for linux kernel patches.

See $LINUX/Documentation/SubmittingPatches for details.

thanks,

Takashi

5417

Age (days ago)

5471

Last active (days ago)

List overview

Download

9 comments

2 participants

tags (0)

participants (2)

michael＠cadilhac.name
Takashi Iwai