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