[alsa-devel] [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[Takashi Iwai]

At Sun, 13 Dec 2009 19:51:01 -0500,
John S Gruber wrote:
> 
> Addressing audio quality problem.
> 
> In sound/usb/usbaudio.c, for the Hauppage HVR-950Q only, change
> retire_capture_urb to copy the entire byte stream while still counting
> entire audio frames. urbs unaligned on channel sample boundaries are
> still truncated to the next lowest stride (audio slot) size to try to
> retain channel alignment in cases of data loss over usb.
> 
> With the HVR950Q the left and right channel samples can be split between
> two different urbs. Throwing away extra channel samples causes a sound
> quality problem for stereo streams as the left and right channels are
> swapped repeatedly.

Through a quick glance, the patch looks good and safe to apply.

Just a few comments below.

> diff --git a/usb/usbaudio.c b/usb/usbaudio.c
> index b074a59..d450b23 100644
> --- a/usb/usbaudio.c
> +++ b/usb/usbaudio.c
> @@ -108,6 +108,7 @@ MODULE_PARM_DESC(ignore_ctl_error,
>  #define MAX_URBS	8
>  #define SYNC_URBS	4	/* always four urbs for sync */
>  #define MAX_QUEUE	24	/* try not to exceed this queue length, in ms */
> +#define ALLOW_SUBSLOT_BOUNDARIES 0x01	/* quirk */

If it's just a boolean flag, we don't need to use bits.
Simply use a char flag or a bit-field flag instead of bit-and operation.


>  struct audioformat {
>  	struct list_head list;
> @@ -127,6 +128,7 @@ struct audioformat {
>  	unsigned int rate_min, rate_max;	/* min/max rates */
>  	unsigned int nr_rates;		/* number of rate table entries */
>  	unsigned int *rate_table;	/* rate table */
> +	unsigned int txfr_quirks;	/* transfer quirks */

I'm not sure whether this flag should belong to audiofmt.
Isn't it rather controller-wide?  If so, it can be better fit to another,
more global struct.

The rest are just trivial coding-style issues.  For example...

> @@ -354,29 +358,39 @@ static int retire_capture_urb(struct
> snd_usb_substream *subs,
>  			snd_printd(KERN_ERR "frame %d active: %d\n", i,
> urb->iso_frame_desc[i].status);
>  			// continue;
>  		}
> -		len = urb->iso_frame_desc[i].actual_length / stride;
> -		if (! len)
> +		bytelen = (urb->iso_frame_desc[i].actual_length);

No need of parentheses.

> +		if (!bytelen)
>  			continue;
> +		if (bytelen%(runtime->sample_bits>>3) != 0) {

Please put spaces around operators.

> +			int oldbytelen = bytelen;
> +			bytelen = ((bytelen/stride)*stride);

Ditto.  Try to run scripts/checkpatch.pl once and fix the complains
there.

> +			snd_printd(KERN_ERR "Corrected urb data len. %d->%d\n",
> +				oldbytelen, bytelen);

Hm, how often would it be printed?  If it's really verbose, use
snd_printdd() instead.


thanks,

Takashi