| Message ID | CAGoCfixa0pr048=-P3OUkZ2HMaY471eNO79BON0vjSVa1eRcTw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em 07-09-2011 00:20, Devin Heitmueller escreveu: > On Tue, Sep 6, 2011 at 10:58 PM, Devin Heitmueller > <dheitmueller@kernellabs.com> wrote: >> On Tue, Sep 6, 2011 at 11:29 AM, Mauro Carvalho Chehab >> <mchehab@redhat.com> wrote: >>> There are several issues with the original alsa_stream code that got >>> fixed on xawtv3, made by me and by Hans de Goede. Basically, the >>> code were re-written, in order to follow the alsa best practises. >>> >>> Backport the changes from xawtv, in order to make it to work on a >>> wider range of V4L and sound adapters. >>> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> >> >> Mauro, >> >> What tuners did you test this patch with? I went ahead and did a git >> pull of your patch series into my local git tree, and now my DVC-90 >> (an em28xx device) is capturing at 32 KHz instead of 48 (this is one >> of the snd-usb-audio based devices, not em28xx-alsa). >> >> Note I tested immediately before pulling your patch series and the >> audio capture was working fine. >> >> I think this patch series is going in the right direction in general, >> but this patch in particular seems to cause a regression. Is this >> something you want to investigate? I think we need to hold off on >> pulling this series into the new tvtime master until this problem is >> resolved. >> >> Devin >> >> -- >> Devin J. Heitmueller - Kernel Labs >> http://www.kernellabs.com >> > > Spent a few minutes digging into this. Looks like the snd-usb-audio > driver advertises 8-48KHz. However, it seems that it only captures > successfully at 48 KHz. > > I made the following hack and it started working: > > diff --git a/src/alsa_stream.c b/src/alsa_stream.c > index b6a41a5..57e3c3d 100644 > --- a/src/alsa_stream.c > +++ b/src/alsa_stream.c > @@ -261,7 +261,7 @@ static int setparams(snd_pcm_t *phandle, snd_pcm_t *chandle, > fprintf(error_fp, "alsa: Will search a common rate between %u and %u\n", > ratemin, ratemax); > > - for (i = ratemin; i <= ratemax; i+= 100) { > + for (i = ratemax; i >= ratemin; i-= 100) { > err = snd_pcm_hw_params_set_rate_near(chandle, c_hwparams, &i, 0); > if (err) > continue; > > Basically the above starts at the *maximum* capture resolution and > works its way down. One might argue that this heuristic makes more > sense anyway - why *wouldn't* you want the highest quality audio > possible by default (rather than the lowest)? That change makes sense to me. Yet, you should try to disable pulseaudio and see what's the _real_ speed that the audio announces. On Fedora, just removing pulsaudio-oss-plugin (or something like that) is enough. It seems doubtful that my em2820 WinTV USB2 is different than yours. I suspect that pulseaudio is passing the "extra" range, offering to interpolate the data. > Even with that patch though, I hit severe underrun/overrun conditions > at 30ms of latency (to the point where the audio is interrupted dozens > of times per second). Yes, it is the same here: 30ms on my notebook is not enough for WinTV USB2 (it is OK with HVR-950). > Turned it up to 50ms and it's much better. > That said, of course such a change would impact lipsync, so perhaps we > need to be adjusting the periods instead. We've added a parameter for that on xawtv3 (--alsa-latency). We've parametrized it at the alsa stream function call. So, all it needs is to add a new parameter at tvtime config file. > ALSA has never been my area of expertise, so I look to you and Hans to > offer some suggestions. > > Devin > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 6, 2011 at 11:29 PM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: >> Basically the above starts at the *maximum* capture resolution and >> works its way down. One might argue that this heuristic makes more >> sense anyway - why *wouldn't* you want the highest quality audio >> possible by default (rather than the lowest)? > > That change makes sense to me. Yet, you should try to disable pulseaudio > and see what's the _real_ speed that the audio announces. On Fedora, > just removing pulsaudio-oss-plugin (or something like that) is enough. > > It seems doubtful that my em2820 WinTV USB2 is different than yours. > I suspect that pulseaudio is passing the "extra" range, offering to > interpolate the data. I disabled pulseaudio and the capture device is advertising the exact same range (8-48 KHz). Seems to be behaving the same way as well. So while I'm usually willing to blame things on Pulse, this doesn't look like the case here. >> Even with that patch though, I hit severe underrun/overrun conditions >> at 30ms of latency (to the point where the audio is interrupted dozens >> of times per second). > > Yes, it is the same here: 30ms on my notebook is not enough for WinTV > USB2 (it is OK with HVR-950). > >> Turned it up to 50ms and it's much better. >> That said, of course such a change would impact lipsync, so perhaps we >> need to be adjusting the periods instead. > > We've added a parameter for that on xawtv3 (--alsa-latency). We've parametrized > it at the alsa stream function call. So, all it needs is to add a new parameter > at tvtime config file. Ugh. We really need some sort of heuristic to do this. It's unreasonable to expect users to know about some magic parameter buried in a config file which causes it to start working. Perhaps a counter that increments whenever an underrun is hit, and after a certain number it automatically restarts the stream with a higher latency. Or perhaps we're just making some poor choice in terms of the buffers/periods for a given rate. Devin
On Tue, Sep 6, 2011 at 11:37 PM, Devin Heitmueller <dheitmueller@kernellabs.com> wrote: > On Tue, Sep 6, 2011 at 11:29 PM, Mauro Carvalho Chehab > <mchehab@redhat.com> wrote: >>> Basically the above starts at the *maximum* capture resolution and >>> works its way down. One might argue that this heuristic makes more >>> sense anyway - why *wouldn't* you want the highest quality audio >>> possible by default (rather than the lowest)? >> >> That change makes sense to me. Yet, you should try to disable pulseaudio >> and see what's the _real_ speed that the audio announces. On Fedora, >> just removing pulsaudio-oss-plugin (or something like that) is enough. >> >> It seems doubtful that my em2820 WinTV USB2 is different than yours. >> I suspect that pulseaudio is passing the "extra" range, offering to >> interpolate the data. > > I disabled pulseaudio and the capture device is advertising the exact > same range (8-48 KHz). Seems to be behaving the same way as well. > > So while I'm usually willing to blame things on Pulse, this doesn't > look like the case here. > >>> Even with that patch though, I hit severe underrun/overrun conditions >>> at 30ms of latency (to the point where the audio is interrupted dozens >>> of times per second). >> >> Yes, it is the same here: 30ms on my notebook is not enough for WinTV >> USB2 (it is OK with HVR-950). >> >>> Turned it up to 50ms and it's much better. >>> That said, of course such a change would impact lipsync, so perhaps we >>> need to be adjusting the periods instead. >> >> We've added a parameter for that on xawtv3 (--alsa-latency). We've parametrized >> it at the alsa stream function call. So, all it needs is to add a new parameter >> at tvtime config file. > > Ugh. We really need some sort of heuristic to do this. It's > unreasonable to expect users to know about some magic parameter buried > in a config file which causes it to start working. Perhaps a counter > that increments whenever an underrun is hit, and after a certain > number it automatically restarts the stream with a higher latency. Or > perhaps we're just making some poor choice in terms of the > buffers/periods for a given rate. > > Devin > > -- > Devin J. Heitmueller - Kernel Labs > http://www.kernellabs.com > One more thing worth noting before I quit for the night: What audio processor is on your WinTV USB 2 device? The DVC-90 has an emp202. Perhaps the WInTV uses a different audio processor (while still using an em2820 as the bridge)? That might explain why your device advertises effectively only one capture rate (32), while mine advertises a whole range (8-48). Devin
On Tue, Sep 6, 2011 at 11:42 PM, Devin Heitmueller <dheitmueller@kernellabs.com> wrote: > One more thing worth noting before I quit for the night: > > What audio processor is on your WinTV USB 2 device? The DVC-90 has an > emp202. Perhaps the WInTV uses a different audio processor (while > still using an em2820 as the bridge)? That might explain why your > device advertises effectively only one capture rate (32), while mine > advertises a whole range (8-48). Just took a look at the driver code. Seems we are calling em28xx_analog_audio_set() even if it's not using vendor audio. And that function actually hard-codes the rate to 48KHz. So here's the question: if using snd-usb-audio, should we really be poking at the AC97 registers at all? It seems that doing such can just get the audio processor state out of sync with however snd-usb-audio set it up. For example, the snd-usb-audio driver may very well be configuring the audio to 32 KHz, and then we reset the chip's state to 48Khz when we start streaming without the snd-usb-audio driver's knowledge. It seems like we should only be setting up the AC97 registers if it's an AC97 audio processor *and* it's using vendor audio. Devin
Hi, Lots of good stuff in this thread! It seems Mauro has answered most things, so I'm just going to respond to this bit. On 09/07/2011 05:37 AM, Devin Heitmueller wrote: <Snip> >> We've added a parameter for that on xawtv3 (--alsa-latency). We've parametrized >> it at the alsa stream function call. So, all it needs is to add a new parameter >> at tvtime config file. > > Ugh. We really need some sort of heuristic to do this. It's > unreasonable to expect users to know about some magic parameter buried > in a config file which causes it to start working. Perhaps a counter > that increments whenever an underrun is hit, and after a certain > number it automatically restarts the stream with a higher latency. Or > perhaps we're just making some poor choice in terms of the > buffers/periods for a given rate. This may have something to do with usb versus pci capture, on my bttv card 30 ms works fine, but I can imagine it being a bit on the low side when doing video + audio capture over USB. So maybe should default to say 50 for usb capture devices and 30 for pci capture devices? In the end if people load there system enough / have a slow enough system our default will always be wrong for them. All we can do is try to get a default which is sane for most setups ... Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 07-09-2011 00:37, Devin Heitmueller escreveu: > On Tue, Sep 6, 2011 at 11:29 PM, Mauro Carvalho Chehab > <mchehab@redhat.com> wrote: >>> Basically the above starts at the *maximum* capture resolution and >>> works its way down. One might argue that this heuristic makes more >>> sense anyway - why *wouldn't* you want the highest quality audio >>> possible by default (rather than the lowest)? >> >> That change makes sense to me. Yet, you should try to disable pulseaudio >> and see what's the _real_ speed that the audio announces. On Fedora, >> just removing pulsaudio-oss-plugin (or something like that) is enough. >> >> It seems doubtful that my em2820 WinTV USB2 is different than yours. >> I suspect that pulseaudio is passing the "extra" range, offering to >> interpolate the data. > > I disabled pulseaudio and the capture device is advertising the exact > same range (8-48 KHz). Seems to be behaving the same way as well. Hmm... just checked with WinTV USB2: [ 3.819236] msp3400 15-0044: MSP3445G-B8 found @ 0x88 (em28xx #0) While this device uses snd-usb-audio, it is a non-AC97 adapter. So, we may expect it to be different from yours. Its A/D works at a fixed rate of 32 kHZ, according with MSP34x5G datasheet, so snd-usb-audio is working properly here. > So while I'm usually willing to blame things on Pulse, this doesn't > look like the case here. That's good. One less problem to deal with. >>> Even with that patch though, I hit severe underrun/overrun conditions >>> at 30ms of latency (to the point where the audio is interrupted dozens >>> of times per second). >> >> Yes, it is the same here: 30ms on my notebook is not enough for WinTV >> USB2 (it is OK with HVR-950). >> >>> Turned it up to 50ms and it's much better. >>> That said, of course such a change would impact lipsync, so perhaps we >>> need to be adjusting the periods instead. >> >> We've added a parameter for that on xawtv3 (--alsa-latency). We've parametrized >> it at the alsa stream function call. So, all it needs is to add a new parameter >> at tvtime config file. > > Ugh. We really need some sort of heuristic to do this. It's > unreasonable to expect users to know about some magic parameter buried > in a config file which causes it to start working. Agreed, but still it makes sense to allow users to adjust, as extra delays might be needed on really slow machines. > Perhaps a counter > that increments whenever an underrun is hit, and after a certain > number it automatically restarts the stream with a higher latency. Or > perhaps we're just making some poor choice in terms of the > buffers/periods for a given rate. > > Devin > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/alsa_stream.c b/src/alsa_stream.c index b6a41a5..57e3c3d 100644 --- a/src/alsa_stream.c +++ b/src/alsa_stream.c @@ -261,7 +261,7 @@ static int setparams(snd_pcm_t *phandle, snd_pcm_t *chandle, fprintf(error_fp, "alsa: Will search a common rate between %u and %u\n", ratemin, ratemax); - for (i = ratemin; i <= ratemax; i+= 100) { + for (i = ratemax; i >= ratemin; i-= 100) { err = snd_pcm_hw_params_set_rate_near(chandle, c_hwparams, &i, 0); if (err) continue;