Message ID | CAGoCfixa0pr048=-P3OUkZ2HMaY471eNO79BON0vjSVa1eRcTw@mail.gmail.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Wed, 07 Sep 2011 03:21:51 +0000 Received: from casper.infradead.org [85.118.1.10] by localhost.localdomain with IMAP (fetchmail-6.3.17) for <mchehab@localhost> (single-drop); Wed, 07 Sep 2011 00:22:40 -0300 (BRT) Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1R18hf-0001zZ-7L; Wed, 07 Sep 2011 03:21:31 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755077Ab1IGDVA (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Tue, 6 Sep 2011 23:21:00 -0400 Received: from mail-bw0-f46.google.com ([209.85.214.46]:45530 "EHLO mail-bw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754714Ab1IGDVA convert rfc822-to-8bit (ORCPT <rfc822; linux-media@vger.kernel.org>); Tue, 6 Sep 2011 23:21:00 -0400 Received: by bke11 with SMTP id 11so6029321bke.19 for <linux-media@vger.kernel.org>; Tue, 06 Sep 2011 20:20:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.204.130.3 with SMTP id q3mr3486099bks.17.1315365658616; Tue, 06 Sep 2011 20:20:58 -0700 (PDT) Received: by 10.204.152.18 with HTTP; Tue, 6 Sep 2011 20:20:58 -0700 (PDT) In-Reply-To: <CAGoCfiy2hnH0Xoz_+Q8JgcB-tzuTGbfv8QdK0kv+ttP7t+EZKg@mail.gmail.com> References: <1315322996-10576-1-git-send-email-mchehab@redhat.com> <CAGoCfiy2hnH0Xoz_+Q8JgcB-tzuTGbfv8QdK0kv+ttP7t+EZKg@mail.gmail.com> Date: Tue, 6 Sep 2011 23:20:58 -0400 Message-ID: <CAGoCfixa0pr048=-P3OUkZ2HMaY471eNO79BON0vjSVa1eRcTw@mail.gmail.com> Subject: Re: [PATCH 01/10] alsa_stream: port changes made on xawtv3 From: Devin Heitmueller <dheitmueller@kernellabs.com> To: Mauro Carvalho Chehab <mchehab@redhat.com> Cc: linux-media@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Devin Heitmueller
Sept. 7, 2011, 3:20 a.m. UTC
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: 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)? 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). 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. ALSA has never been my area of expertise, so I look to you and Hans to offer some suggestions. Devin
Comments
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
Hi Devin, I had some discussions with Mikael today at the #linuxtv channel about tvtime. Mikael has write access to the tvtime site at sourceforge and he is doing some maintainance on it for some time, and worked on some bugs from Gentoo, and also imported some stuff from Ubuntu. I've merged his patches on my repository: http://git.linuxtv.org/mchehab/tvtime.git Tvtime is compiling, at least on Fedora 15. I also added your patch there, and changed the latency delay to 50ms. I didn't test it yet. I'll do it later today or tomorrow. Btw, Mikael updated the Related Sites there to point to the LinuxTV site: http://tvtime.sourceforge.net/links.html He will try to contact Vektor again, in order to get his ack about adding a note at the main page pointing to us. I think we should move those patches to the main repository after testing the merges, and give write rights to the ones that are interested on maintaining tvtime. I'm interested on it, and also Mikael. IMHO, after testing it and applying a few other patches that Mikael might have, it is time for us to rename the version to 1.10 and do a tvtime release. Would that work for you? Thank you! Mauro -- 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 Thu, Oct 6, 2011 at 9:59 PM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > Hi Devin, > > I had some discussions with Mikael today at the #linuxtv channel about > tvtime. Mikael has write access to the tvtime site at sourceforge and he > is doing some maintainance on it for some time, and worked on some bugs > from Gentoo, and also imported some stuff from Ubuntu. > > I've merged his patches on my repository: > http://git.linuxtv.org/mchehab/tvtime.git > > Tvtime is compiling, at least on Fedora 15. I also added your patch there, > and changed the latency delay to 50ms. I didn't test it yet. I'll do it > later > today or tomorrow. > > Btw, Mikael updated the Related Sites there to point to the LinuxTV site: > http://tvtime.sourceforge.net/links.html > > He will try to contact Vektor again, in order to get his ack about adding > a note at the main page pointing to us. > > I think we should move those patches to the main repository after testing > the > merges, and give write rights to the ones that are interested on maintaining > tvtime. > > I'm interested on it, and also Mikael. > > IMHO, after testing it and applying a few other patches that Mikael might > have, > it is time for us to rename the version to 1.10 and do a tvtime release. > > Would that work for you? > > Thank you! > Mauro Hi Mauro, It's good to hear that patches are continuing to be merged, and of course contributors are always welcome. The more I think about this, the more I recognize that I'm not really adding any value to this process. While I would really like to put more time/energy into tvtime, I just don't have the time and it appears I'm actually slowing down a community of contributors who are trying to move things forward. At this point I would recommend the LinuxTV community just take over the project, give yourself write access to the main repo, and spin a release. I would indeed recommend calling it 1.10, to prevent confusion with the various vendor branches where I believe some of which may actually already be calling themselves 1.03. Regarding expanding the list of individuals with commit rights, I might suggest keeping the list of write privileges for the main repo to a minimum in the short term (starting with yourself), until developers have demonstrated their ability to author coherent patches which won't cause breakage as well as the ability to review the work of others. Cheers, Devin
Em 07-10-2011 10:38, Devin Heitmueller escreveu: > On Thu, Oct 6, 2011 at 9:59 PM, Mauro Carvalho Chehab > <mchehab@redhat.com> wrote: >> Hi Devin, >> >> I had some discussions with Mikael today at the #linuxtv channel about >> tvtime. Mikael has write access to the tvtime site at sourceforge and he >> is doing some maintainance on it for some time, and worked on some bugs >> from Gentoo, and also imported some stuff from Ubuntu. >> >> I've merged his patches on my repository: >> http://git.linuxtv.org/mchehab/tvtime.git >> >> Tvtime is compiling, at least on Fedora 15. I also added your patch there, >> and changed the latency delay to 50ms. I didn't test it yet. I'll do it >> later >> today or tomorrow. >> >> Btw, Mikael updated the Related Sites there to point to the LinuxTV site: >> http://tvtime.sourceforge.net/links.html >> >> He will try to contact Vektor again, in order to get his ack about adding >> a note at the main page pointing to us. >> >> I think we should move those patches to the main repository after testing >> the >> merges, and give write rights to the ones that are interested on maintaining >> tvtime. >> >> I'm interested on it, and also Mikael. >> >> IMHO, after testing it and applying a few other patches that Mikael might >> have, >> it is time for us to rename the version to 1.10 and do a tvtime release. >> >> Would that work for you? >> >> Thank you! >> Mauro > > Hi Mauro, > > It's good to hear that patches are continuing to be merged, and of > course contributors are always welcome. > > The more I think about this, the more I recognize that I'm not really > adding any value to this process. While I would really like to put > more time/energy into tvtime, I just don't have the time and it > appears I'm actually slowing down a community of contributors who are > trying to move things forward. > > At this point I would recommend the LinuxTV community just take over > the project, give yourself write access to the main repo, and spin a > release. I would indeed recommend calling it 1.10, to prevent > confusion with the various vendor branches where I believe some of > which may actually already be calling themselves 1.03. Ok, I've added myself into it. I've just pushed everything into: http://git.linuxtv.org/tvtime.git For now, it is showing as version 1.0.4. I'll rename it to 1.1.0 after getting some feedback and maybe some additional fixes, and add an announcement about that when we'll be there. I tested it yesterday, and it seems to be working properly. Mikael patches were putting it on a borderless mode, with looked weird on my eyes ;) So, I added a new parameter (-L) to allow selecting the borderless mode for those that prefer that way. The default is to have borders. > Regarding expanding the list of individuals with commit rights, I > might suggest keeping the list of write privileges for the main repo > to a minimum in the short term (starting with yourself), until > developers have demonstrated their ability to author coherent patches > which won't cause breakage as well as the ability to review the work > of others. Maybe Hans de Goede would also like to get his hands on it, as he wrote a few patches for it on Fedora. Thanks, Mauro -- 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;