Getting pcm_usb_stream plugin to know its limits.

Getting pcm_usb_stream plugin to know its limits.

[Michaël Cadilhac]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

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 :


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: memset.patch --]
[-- Type: text/x-patch, Size: 392 bytes --]

--- 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;

[-- Attachment #3: Type: text/plain, Size: 1479 bytes --]


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:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: period_size.patch --]
[-- Type: text/x-patch, Size: 511 bytes --]

--- pcm_usb_stream.c	2008-11-21 18:08:16.000000000 -0500
+++ pcm_usb_stream.c	2009-12-30 12:36:00.654452577 -0500
@@ -380,7 +380,7 @@
 	    (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_RATE,
 						   44100, 96000)) < 0 ||
 	    (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_PERIOD_BYTES,
-						   128, 64*4096)) < 0 ||
+						   128, 2*4096)) < 0 ||
 	    (err = snd_pcm_ioplug_set_param_minmax(&us->io, SND_PCM_IOPLUG_HW_PERIODS,
 						   2, 2)) < 0)
 		return err;

[-- Attachment #5: Type: text/plain, Size: 455 bytes --]



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

[-- Attachment #6: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Getting pcm_usb_stream plugin to know its limits.

[Michaël Cadilhac]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

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:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: memset.patch --]
[-- Type: text/x-patch, Size: 476 bytes --]

--- /dd/alsa-plugins/usb_stream/pcm_usb_stream.c	2008-11-21 18:08:16.000000000 -0500
+++ pcm_usb_stream.c	2009-12-30 12:57:13.779349630 -0500
@@ -256,7 +256,10 @@
 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);

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]


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

[-- Attachment #4: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Getting pcm_usb_stream plugin to know its limits.

[Michaël Cadilhac]

[-- Attachment #1: Type: text/plain, Size: 486 bytes --]

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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: allow.patch --]
[-- Type: text/x-patch, Size: 3482 bytes --]

--- pcm_usb_stream.c	2008-11-21 18:08:16.000000000 -0500
+++ /tmp/alsa-plugins-1.0.21/usb_stream/pcm_usb_stream.c	2009-12-30 22:59:45.884129907 -0500
@@ -1,7 +1,7 @@
 /*
  *  PCM - USB_STREAM plugin
  *
- *  Copyright (c) 2008 by Karsten Wiese <fzu@wemgehoertderstaat.de>
+ *  Copyright (c) 2008, 2009 by Karsten Wiese <fzu@wemgehoertderstaat.de>
  *
  * This library is free software; you can redistribute it and/or modify
  * it under the terms of the GNU Lesser General Public License as
@@ -70,6 +70,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;
@@ -256,8 +258,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 +375,10 @@
 	};
 
 	int err;
+	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;
 
 	if ((err = snd_pcm_ioplug_set_param_list(&us->io, SND_PCM_IOPLUG_HW_ACCESS,
 						 ARRAY_SIZE(access_list), access_list)) < 0 ||
@@ -378,19 +387,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_size_min, period_size_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 +431,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 +467,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 +485,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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 568 bytes --]


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

[-- Attachment #4: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

[Michaël Cadilhac]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

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).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: all.patch --]
[-- Type: text/x-patch, Size: 3528 bytes --]

--- /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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 216 bytes --]


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

[-- Attachment #4: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

[Michaël Cadilhac]

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

Re: Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

[Michaël Cadilhac]

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

Re: Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

[Michaël Cadilhac]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 00fix-deref.patch --]
[-- Type: text/x-patch, Size: 813 bytes --]

--- usb_stream/pcm_usb_stream.c	2009-12-16 10:18:55.000000000 -0500
+++ usb_stream/pcm_usb_stream.c	2010-01-23 10:19:09.032559296 -0500
@@ -1,7 +1,7 @@
 /*
  *  PCM - USB_STREAM plugin
  *
- *  Copyright (c) 2008 by Karsten Wiese <fzu@wemgehoertderstaat.de>
+ *  Copyright (c) 2008, 2010 by Karsten Wiese <fzu@wemgehoertderstaat.de>
  *
  * This library is free software; you can redistribute it and/or modify
  * it under the terms of the GNU Lesser General Public License as
@@ -256,8 +256,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);
 

[-- Attachment #3: Type: text/plain, Size: 72 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 01add-tweaking.patch --]
[-- Type: text/x-patch, Size: 3185 bytes --]

--- usb_stream/pcm_usb_stream.c	2010-01-23 10:19:09.032559296 -0500
+++ usb_stream/pcm_usb_stream.c	2010-01-23 10:20:36.006628021 -0500
@@ -48,6 +48,8 @@
 #define VDBG(f, ...)
 #endif
 
+#define FRAME_SIZE 6
+
 #define LCARD 32
 struct user_usb_stream {
 	char			card[LCARD];
@@ -70,6 +72,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 +181,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;
 
@@ -373,6 +377,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 ||
@@ -381,9 +389,9 @@
 	    (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;
@@ -393,7 +401,9 @@
 
 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;
@@ -424,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";
@@ -458,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);
@@ -475,11 +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;
 }

[-- Attachment #5: Type: text/plain, Size: 325 bytes --]


  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.

[-- Attachment #6: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

[Michaël Cadilhac]

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

Re: Getting pcm_usb_stream plugin to know its limits. [Kind of SOLVED]

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel