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