Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only

2009-12-10 Thread John S Gruber
 After reviewing the patch as well as the spec, your change looks
 pretty reasonable (aside from the fact that you need the other USB
 IDs).  It seems pretty clear that the au0828 violates the spec, but
 the spec does indicate how to handle that case, which is what your
 code addresses.


I think you found something in the specification I haven't found. What did you
see that indicated how to deal with equipment misbehaving in this way?

I found the following list of USB ID's. Just double checking, but is
the 850 enough like the
950 line for me to include it?

case 72000: /* WinTV-HVR950q (Retail, IR, ATSC/QAM */
case 72001: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */
case 72211: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
case 72221: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
case 72231: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
case 72241: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM and analog video */
case 72251: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */
--   case 72301: /* WinTV-HVR850 (Retail, IR, ATSC and analog video */
case 72500: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM */


Thanks again, not only for your help with my change, but with all you did to
provide support for the Hauppage equipment.

I'd add that being the beginner at making kernel changes your review
is particularly
helpful to me (and to my confidence).

John
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only

2009-12-10 Thread Devin Heitmueller
Hello John,

On Thu, Dec 10, 2009 at 8:38 PM, John S Gruber johnsgru...@gmail.com wrote:
 I think you found something in the specification I haven't found. What did you
 see that indicated how to deal with equipment misbehaving in this way?

I'm referring to section 2.3.2.3 of Universal Serial Bus Device Class
Definition for Audio Data Formats

 I found the following list of USB ID's. Just double checking, but is
 the 850 enough like the
 950 line for me to include it?

        case 72000: /* WinTV-HVR950q (Retail, IR, ATSC/QAM */
        case 72001: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */
        case 72211: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
        case 72221: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
        case 72231: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
        case 72241: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM and analog video */
        case 72251: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */
 --   case 72301: /* WinTV-HVR850 (Retail, IR, ATSC and analog video */
        case 72500: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM */

Yes, the HVR-850 should be included in the list.

 I'd add that being the beginner at making kernel changes your review
 is particularly
 helpful to me (and to my confidence).

You are likely to receive a more thorough/critical review when you
send to the alsa-devel mailing list, as they have considerably more
expertise in this area than I do.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only

2009-12-10 Thread John S Gruber
On Thu, Dec 10, 2009 at 8:49 PM, Devin Heitmueller
dheitmuel...@kernellabs.com wrote:
 Hello John,

 On Thu, Dec 10, 2009 at 8:38 PM, John S Gruber johnsgru...@gmail.com wrote:
 I think you found something in the specification I haven't found. What did 
 you
 see that indicated how to deal with equipment misbehaving in this way?

 I'm referring to section 2.3.2.3 of Universal Serial Bus Device Class
 Definition for Audio Data Formats

 I found the following list of USB ID's. Just double checking, but is
 the 850 enough like the
 950 line for me to include it?

        case 72000: /* WinTV-HVR950q (Retail, IR, ATSC/QAM */
        case 72001: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */
        case 72211: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
        case 72221: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
        case 72231: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */
        case 72241: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM and analog video */
        case 72251: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */
 --   case 72301: /* WinTV-HVR850 (Retail, IR, ATSC and analog video */
        case 72500: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM */

 Yes, the HVR-850 should be included in the list.

 I'd add that being the beginner at making kernel changes your review
 is particularly
 helpful to me (and to my confidence).

 You are likely to receive a more thorough/critical review when you
 send to the alsa-devel mailing list, as they have considerably more
 expertise in this area than I do.

 Devin

 --
 Devin J. Heitmueller - Kernel Labs
 http://www.kernellabs.com


I'll do that.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only

2009-12-07 Thread Devin Heitmueller
On Fri, Dec 4, 2009 at 6:26 PM, Devin Heitmueller
dheitmuel...@kernellabs.com wrote:
 On Fri, Dec 4, 2009 at 5:15 PM, John S Gruber johnsgru...@gmail.com 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.
 snip

 Hello John,

 Thanks for taking the time to dig into this.  I will try to review
 your patch this weekend (in conjunction with the spec).

 It's worth noting that there are actually nine different USB IDs that
 would need this change (see au0828-cards.c), so it might be nice to
 see if we can figure out a way for the au0828 driver to tell the
 usbaudio driver about the quirk without relying on embedding USB ids
 in the usbaudio driver.

Hi John,

After reviewing the patch as well as the spec, your change looks
pretty reasonable (aside from the fact that you need the other USB
IDs).  It seems pretty clear that the au0828 violates the spec, but
the spec does indicate how to handle that case, which is what your
code addresses.

All that said, the driver in question is managed by the ALSA project.
I would suggest posting a message with your patch on the alsa-devel
mailing list, so that it can be merged (and in fairness, those guys
are much better suited to review your patch).

On a separate note, I've been doing some testing with the device in a
low-latency application, and I think I've spotted a separate bug in
the usbaudio driver that causes the audio capture to stall.  I'll be
sending some email myself to alsa-devel as soon as I can test a patch.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only

2009-12-04 Thread Devin Heitmueller
On Fri, Dec 4, 2009 at 5:15 PM, John S Gruber johnsgru...@gmail.com 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.
snip

Hello John,

Thanks for taking the time to dig into this.  I will try to review
your patch this weekend (in conjunction with the spec).

It's worth noting that there are actually nine different USB IDs that
would need this change (see au0828-cards.c), so it might be nice to
see if we can figure out a way for the au0828 driver to tell the
usbaudio driver about the quirk without relying on embedding USB ids
in the usbaudio driver.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html