At Wed, 23 Dec 2009 23:32:56 -0500, John S Gruber wrote: > > [PATCH 1/2] sound: usb-audio: relax urb data align. restriction > HVR-950Q and HVR-850 only > > Addressing audio quality problem. > > In sound/usb/usbaudio.c, for the Hauppage HVR-950Q and HVR-850 only, change > retire_capture_urb to allow transfers on audio sub-slot boundaries rather > than audio slots boundaries. > > With these devices 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, perhaps many times per second. > > Urbs unaligned on sub-slot boundaries are still truncated to the next > lowest stride (audio slot) to retain synchronization on samples even > though left/right channel synchronization may be lost in this case. > > Detect the quirk using a case statement in snd_usb_audio_probe. > > BugLink: https://bugs.launchpad.net/ubuntu/+bug/495745 > > Signed-off-by: John S. Gruber <JohnSGruber@xxxxxxxxx> > --- > Clemens: > > Thank you for your patch. It certainly makes my patch much more straightforward. > > >USB packets are checksummed, so in case of data corruption, the entire > >packets would be dropped by the controller, so you'll never get partial > >samples. > > >With your device, one lost packet could potentially corrupt the entire > >following stream. There is no good error handling strategy for that. > > You are certainly correct that a dropped packet corrupting the entire > stream. The misaligned packets occur about 1/40 - 1/80 packets, so > that's the odds that a dropped frame will cause the left and right channels > to swap. Much better than 12-24 channel swaps per second, though. > > WRT checksum errors, during my last test I saw that the urb's with an odd > byte length coincided with the debugging message produced a few lines up: > > Dec 21 22:52:54 gruber-laptop kernel: [178695.608453] ALSA > usbaudio.c:361: frame 0 active: -71 > Dec 21 22:52:54 gruber-laptop kernel: [178695.608470] ALSA > usbaudio.c:372: Corrected urb data len. 191->188 > Dec 21 22:53:25 gruber-laptop kernel: [178726.396561] ALSA > usbaudio.c:361: frame 0 active: -71 > Dec 21 22:53:25 gruber-laptop kernel: [178726.396576] ALSA > usbaudio.c:372: Corrected urb data len. 191->188 > > While every odd byte length coincided with a -71 packet status (EPROTO?), > there also were -71 packet status messages appearing alone. At other times > there were uncorrelated -18 (-EXDEV?) messages too. > > In usb/usbaudio.c retire_capture_urb() I note that the continue statement > that would drop these packets right after the snd_printd() is commented out. > > Takashi: > > >> +#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. > > I've switched to using bit-field flags instead and moved the format > occurrance of the flag to the more general snd_usb_audio structure. > The flag is copied to the substream structure as an optimization, > of course. > > >The rest are just trivial coding-style issues. For example... > > I'm sorry about the style problems. After reworking the patch I've looked again > for style problems and hope that I've found and fixed everything, but I'm new > at Linux kernel changes. I've run the patches through a recent version > checkpatch.pl > and I've tried to check them more thoroughly by hand, too. > > >> + 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. > > Although the debugging message is produced rather rarely with my device, > I don't know for sure that it won't occur more often on the 850 or other > 950Q types. I've changed the snd_printd() to a snd_printdd() at > your suggestion. > > ********* > > I don't know whether a case statement or a quirk in usbquirk.h is more > maintainable. I've added a second patch in case usbquirk.h is preferable. It > uses an unusual positive return code to indicate to the probe code that > processing should continue as normal. > > Please review or ignore the second patch as deemed best. > > The first patch follows immediately. The prerequisite is Clemens' > patch sound: "usb-audio: > make buffer pointer based on bytes instead on frames". Thanks for your work. It all looks good to me now, but it couldn't be applied as is. I guess your patch isn't based on the very latest version. Could you rebase it to the latest sound git tree git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git Also, the embedded patch in your post was broken, maybe due to MUA. If it's difficult to fix your MUA, just use an attachment. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
