[01/10] alsa_stream: port changes made on xawtv3

  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
  

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