public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed
* Some fixes for alsa_stream
@ 2011-06-14  2:01 Mauro Carvalho Chehab
  2011-06-14 12:48 ` Hans de Goede
  0 siblings, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-14  2:01 UTC (permalink / raw)
  To: Devin Heitmueller; +Cc: Linux Media Mailing List

Hi Devin,

I've made a few fixes for your alsa_stream.c, used on tvtime.
They are at:
	http://git.linuxtv.org/xawtv3.git


In particular, those are the more interesting ones:

commit a1bb5ade5c2b09d6d6d624d18025f9e2c4398495
    alsa_stream: negotiate the frame rate

Without this patch, one of my em28xx devices doesn't work. It uses
32 k rate, while the playback minimal rate is 44.1 k.
I've changed the entire frame rate logic, to be more reliable, and to
avoid needing to do frame rate conversion, if both capture and playback
devices support the same rate.

commit 8adb3d7442b22022b9ca897b0b914962adf41270
    alsa_stream: Reduce CPU usage by putting the thread into blocking mode

This is just an optimization. I can't see why are you using a non-block
mode, as it works fine blocking.

commit c67f7aeb86c1caceb7ab30439d169356ea5b1e72
    alsa_stream.c: use mmap mode instead of the normal mode

Instead of using the normal way, this patch implements mmap mode, and change
it to be the default mode. This should also help to reduce CPU usage.

Feel free to rebase those patches and apply into your tvtime tree, if you
want.

Cheers,
Mauro.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14  2:01 Some fixes for alsa_stream Mauro Carvalho Chehab
@ 2011-06-14 12:48 ` Hans de Goede
  2011-06-14 13:05   ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-06-14 12:48 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/14/2011 04:01 AM, Mauro Carvalho Chehab wrote:
> Hi Devin,
>
> I've made a few fixes for your alsa_stream.c, used on tvtime.
> They are at:
> 	http://git.linuxtv.org/xawtv3.git
>
>
> In particular, those are the more interesting ones:
>
> commit a1bb5ade5c2b09d6d6d624d18025f9e2c4398495
>      alsa_stream: negotiate the frame rate
>
> Without this patch, one of my em28xx devices doesn't work. It uses
> 32 k rate, while the playback minimal rate is 44.1 k.
> I've changed the entire frame rate logic, to be more reliable, and to
> avoid needing to do frame rate conversion, if both capture and playback
> devices support the same rate.
>
> commit 8adb3d7442b22022b9ca897b0b914962adf41270
>      alsa_stream: Reduce CPU usage by putting the thread into blocking mode
>
> This is just an optimization. I can't see why are you using a non-block
> mode, as it works fine blocking.
>
> commit c67f7aeb86c1caceb7ab30439d169356ea5b1e72
>      alsa_stream.c: use mmap mode instead of the normal mode
>
> Instead of using the normal way, this patch implements mmap mode, and change
> it to be the default mode. This should also help to reduce CPU usage.
>

hmm, does this include automatic fallback to read mode if mmap mode is not
available, mmap mode does not work with a number of devices (such as pulseaudio's
alsa plugin).

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 12:48 ` Hans de Goede
@ 2011-06-14 13:05   ` Mauro Carvalho Chehab
  2011-06-14 13:39     ` Mauro Carvalho Chehab
  2011-06-14 13:47     ` Hans de Goede
  0 siblings, 2 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-14 13:05 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 14-06-2011 09:48, Hans de Goede escreveu:
> Hi,
> 
> On 06/14/2011 04:01 AM, Mauro Carvalho Chehab wrote:
>> Hi Devin,
>>
>> I've made a few fixes for your alsa_stream.c, used on tvtime.
>> They are at:
>>     http://git.linuxtv.org/xawtv3.git
>>
>>
>> In particular, those are the more interesting ones:
>>
>> commit a1bb5ade5c2b09d6d6d624d18025f9e2c4398495
>>      alsa_stream: negotiate the frame rate
>>
>> Without this patch, one of my em28xx devices doesn't work. It uses
>> 32 k rate, while the playback minimal rate is 44.1 k.
>> I've changed the entire frame rate logic, to be more reliable, and to
>> avoid needing to do frame rate conversion, if both capture and playback
>> devices support the same rate.
>>
>> commit 8adb3d7442b22022b9ca897b0b914962adf41270
>>      alsa_stream: Reduce CPU usage by putting the thread into blocking mode
>>
>> This is just an optimization. I can't see why are you using a non-block
>> mode, as it works fine blocking.
>>
>> commit c67f7aeb86c1caceb7ab30439d169356ea5b1e72
>>      alsa_stream.c: use mmap mode instead of the normal mode
>>
>> Instead of using the normal way, this patch implements mmap mode, and change
>> it to be the default mode. This should also help to reduce CPU usage.
>>
> 
> hmm, does this include automatic fallback to read mode if mmap mode is not
> available, mmap mode does not work with a number of devices (such as pulseaudio's
> alsa plugin).

No, it doesn't. I'm about to add an option at xawtv3 to allow users to manually
select between mmap/normal, and to change the input/output devices.

Well, pulseaudio is a bad behavioured boy that has several broken things, like
preventing the removal of a V4L2 device for nothing. I won't be surprised if we
notice even more problems with pulseaudio and V4L devices. At least on my test
with fedora 15, audio is playing, even if pulseaudio is loaded.

It should be noticed that the driver tries first to access the alsa driver directly,
by using hw:0,0 output device. If it fails, it falls back to plughw:0,0. I'm not sure
what's the name of the pulseaudio output, but I suspect that both are just bypassing
pulseaudio, with is good ;)

> 
> 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 13:05   ` Mauro Carvalho Chehab
@ 2011-06-14 13:39     ` Mauro Carvalho Chehab
  2011-06-14 13:47     ` Hans de Goede
  1 sibling, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-14 13:39 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 14-06-2011 10:05, Mauro Carvalho Chehab escreveu:
> Em 14-06-2011 09:48, Hans de Goede escreveu:
>> Hi,
>>
>> On 06/14/2011 04:01 AM, Mauro Carvalho Chehab wrote:
>>> Hi Devin,
>>>
>>> I've made a few fixes for your alsa_stream.c, used on tvtime.
>>> They are at:
>>>     http://git.linuxtv.org/xawtv3.git
>>>
>>>
>>> In particular, those are the more interesting ones:
>>>
>>> commit a1bb5ade5c2b09d6d6d624d18025f9e2c4398495
>>>      alsa_stream: negotiate the frame rate
>>>
>>> Without this patch, one of my em28xx devices doesn't work. It uses
>>> 32 k rate, while the playback minimal rate is 44.1 k.
>>> I've changed the entire frame rate logic, to be more reliable, and to
>>> avoid needing to do frame rate conversion, if both capture and playback
>>> devices support the same rate.
>>>
>>> commit 8adb3d7442b22022b9ca897b0b914962adf41270
>>>      alsa_stream: Reduce CPU usage by putting the thread into blocking mode
>>>
>>> This is just an optimization. I can't see why are you using a non-block
>>> mode, as it works fine blocking.
>>>
>>> commit c67f7aeb86c1caceb7ab30439d169356ea5b1e72
>>>      alsa_stream.c: use mmap mode instead of the normal mode
>>>
>>> Instead of using the normal way, this patch implements mmap mode, and change
>>> it to be the default mode. This should also help to reduce CPU usage.
>>>
>>
>> hmm, does this include automatic fallback to read mode if mmap mode is not
>> available, mmap mode does not work with a number of devices (such as pulseaudio's
>> alsa plugin).
> 
> No, it doesn't. I'm about to add an option at xawtv3 to allow users to manually
> select between mmap/normal, and to change the input/output devices.

The options are there:
	http://git.linuxtv.org/xawtv3.git?a=commit;h=81fc25c5c551ab54fbd90fa6aacd563f03ff73d3

this adds support for -v option inside the alsa_stream, and adds 3 options to allow
manually enabling/disabling mmap and changing the detected alsa devices.

> 
> Well, pulseaudio is a bad behavioured boy that has several broken things, like
> preventing the removal of a V4L2 device for nothing. I won't be surprised if we
> notice even more problems with pulseaudio and V4L devices. At least on my test
> with fedora 15, audio is playing, even if pulseaudio is loaded.
> 
> It should be noticed that the driver tries first to access the alsa driver directly,
> by using hw:0,0 output device. If it fails, it falls back to plughw:0,0. I'm not sure
> what's the name of the pulseaudio output, but I suspect that both are just bypassing
> pulseaudio, with is good ;)
> 
>>
>> 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
> 
> --
> 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 13:05   ` Mauro Carvalho Chehab
  2011-06-14 13:39     ` Mauro Carvalho Chehab
@ 2011-06-14 13:47     ` Hans de Goede
  2011-06-14 13:52       ` Devin Heitmueller
  1 sibling, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-06-14 13:47 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/14/2011 03:05 PM, Mauro Carvalho Chehab wrote:
> Em 14-06-2011 09:48, Hans de Goede escreveu:
>> Hi,
>>
>> On 06/14/2011 04:01 AM, Mauro Carvalho Chehab wrote:
>>> Hi Devin,
>>>
>>> I've made a few fixes for your alsa_stream.c, used on tvtime.
>>> They are at:
>>>      http://git.linuxtv.org/xawtv3.git
>>>
>>>
>>> In particular, those are the more interesting ones:
>>>
>>> commit a1bb5ade5c2b09d6d6d624d18025f9e2c4398495
>>>       alsa_stream: negotiate the frame rate
>>>
>>> Without this patch, one of my em28xx devices doesn't work. It uses
>>> 32 k rate, while the playback minimal rate is 44.1 k.
>>> I've changed the entire frame rate logic, to be more reliable, and to
>>> avoid needing to do frame rate conversion, if both capture and playback
>>> devices support the same rate.
>>>
>>> commit 8adb3d7442b22022b9ca897b0b914962adf41270
>>>       alsa_stream: Reduce CPU usage by putting the thread into blocking mode
>>>
>>> This is just an optimization. I can't see why are you using a non-block
>>> mode, as it works fine blocking.
>>>
>>> commit c67f7aeb86c1caceb7ab30439d169356ea5b1e72
>>>       alsa_stream.c: use mmap mode instead of the normal mode
>>>
>>> Instead of using the normal way, this patch implements mmap mode, and change
>>> it to be the default mode. This should also help to reduce CPU usage.
>>>
>>
>> hmm, does this include automatic fallback to read mode if mmap mode is not
>> available, mmap mode does not work with a number of devices (such as pulseaudio's
>> alsa plugin).
>
> No, it doesn't. I'm about to add an option at xawtv3 to allow users to manually
> select between mmap/normal, and to change the input/output devices.
>


Hmm, we really don't need more cmdline options IMHO, it is quite easy to detect
if an alsa device supports mmap mode, and if not fall back to r/w mode, I know
several programs which do that (some if which I've written the patches to do
this for myself).

> Well, pulseaudio is a bad behavioured boy that has several broken things, like
> preventing the removal of a V4L2 device for nothing. I won't be surprised if we
> notice even more problems with pulseaudio and V4L devices. At least on my test
> with fedora 15, audio is playing, even if pulseaudio is loaded.
>
> It should be noticed that the driver tries first to access the alsa driver directly,
> by using hw:0,0 output device. If it fails, it falls back to plughw:0,0. I'm not sure
> what's the name of the pulseaudio output, but I suspect that both are just bypassing
> pulseaudio, with is good ;)

Right this means you're just bypassing pulse audio, which for a tvcard + tv-viewing
app is a reasonable thing to do. Defaulting to hw:0,0 makes no sense to me though, we
should default to either the audio devices belonging to the video device (as determined
through sysfs), or to alsa's default input (which will likely be pulseaudio).

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 13:47     ` Hans de Goede
@ 2011-06-14 13:52       ` Devin Heitmueller
  2011-06-14 14:17         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 23+ messages in thread
From: Devin Heitmueller @ 2011-06-14 13:52 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Mauro Carvalho Chehab, Linux Media Mailing List

On Tue, Jun 14, 2011 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hmm, we really don't need more cmdline options IMHO, it is quite easy to
> detect
> if an alsa device supports mmap mode, and if not fall back to r/w mode, I
> know
> several programs which do that (some if which I've written the patches to do
> this for myself).

Agreed.

>> It should be noticed that the driver tries first to access the alsa driver
>> directly,
>> by using hw:0,0 output device. If it fails, it falls back to plughw:0,0.
>> I'm not sure
>> what's the name of the pulseaudio output, but I suspect that both are just
>> bypassing
>> pulseaudio, with is good ;)
>
> Right this means you're just bypassing pulse audio, which for a tvcard +
> tv-viewing

Actually, the ALSA client libraries route through PulseAudio (as long
as Pulse is running).  Basically PulseAudio is providing emulation for
the ALSA interface even if you specify "hw:1,0" as the device.

> app is a reasonable thing to do. Defaulting to hw:0,0 makes no sense to me
> though, we
> should default to either the audio devices belonging to the video device (as
> determined
> through sysfs), or to alsa's default input (which will likely be
> pulseaudio).

Mauro was talking about the output device, not the input device.

Devin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 13:52       ` Devin Heitmueller
@ 2011-06-14 14:17         ` Mauro Carvalho Chehab
  2011-06-14 14:37           ` Hans de Goede
  0 siblings, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-14 14:17 UTC (permalink / raw)
  To: Devin Heitmueller; +Cc: Hans de Goede, Linux Media Mailing List

Em 14-06-2011 10:52, Devin Heitmueller escreveu:
> On Tue, Jun 14, 2011 at 9:47 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hmm, we really don't need more cmdline options IMHO, it is quite easy to
>> detect
>> if an alsa device supports mmap mode, and if not fall back to r/w mode, I
>> know
>> several programs which do that (some if which I've written the patches to do
>> this for myself).
> 
> Agreed.
> 
>>> It should be noticed that the driver tries first to access the alsa driver
>>> directly,
>>> by using hw:0,0 output device. If it fails, it falls back to plughw:0,0.
>>> I'm not sure
>>> what's the name of the pulseaudio output, but I suspect that both are just
>>> bypassing
>>> pulseaudio, with is good ;)
>>
>> Right this means you're just bypassing pulse audio, which for a tvcard +
>> tv-viewing
> 
> Actually, the ALSA client libraries route through PulseAudio (as long
> as Pulse is running).  Basically PulseAudio is providing emulation for
> the ALSA interface even if you specify "hw:1,0" as the device.

I'm not so sure about that. This probably depends on how the alsa library
is configured, and this is distribution-specific. I'm almost sure that
pulseaudio won't touch on hw: on Fedora.

>> app is a reasonable thing to do. Defaulting to hw:0,0 makes no sense to me
>> though, we
>> should default to either the audio devices belonging to the video device (as
>> determined
>> through sysfs), or to alsa's default input (which will likely be
>> pulseaudio).
> 
> Mauro was talking about the output device, not the input device.

Yes.

The default for capture is the one detected via sysfs.

The default for playback is not really hw:0,0. It defaults to the first hw: that it is not 
associated with a video device. 

I don't like the idea of defaulting to pulseaudio: on my own experiences, the addition
of pulseaudio didn't bring me any benefit, but it causes several troubles that I needed to
workaround, like disabling the access to the master volume control on a Sony Vaio notebook
while setting it to 0 (I had to manually add some scripting at rc.local to fix), 
limiting the max volume to half of the maximum (very bad effect on some notebooks), 
preventing rmmod of V4L devices, and not working when the development user is different
than the console owner, even when it is at the audio group. I can't think on even a single 
benefit of using it on my usecase.

Besides that, video playback generates too much IO, and, on slower machines, it demands
a lot of CPU time. Not having an extra software layer is a good thing to do for the
default.

If someone wants to use pulseaudio, all they need to do is to pass an extra parameter.
That's said, I was not able yet to discover what are the alsa names for pulseaudio
devices. Any ideas on how to get it?

Mauro

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 14:17         ` Mauro Carvalho Chehab
@ 2011-06-14 14:37           ` Hans de Goede
  2011-06-14 14:45             ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-06-14 14:37 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/14/2011 04:17 PM, Mauro Carvalho Chehab wrote:
> Em 14-06-2011 10:52, Devin Heitmueller escreveu:

<snip>

> Yes.
>
> The default for capture is the one detected via sysfs.
>
> The default for playback is not really hw:0,0. It defaults to the first hw: that it is not
> associated with a video device.
>

I have a really weird idea, why not make the default output device be "default", so that
xawtv will use whatever the distro (or user if overriden by the user) has configured as
default alsa output device?

This will do the right thing for pulseaudio and not pulseaudio users alike.

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 14:37           ` Hans de Goede
@ 2011-06-14 14:45             ` Mauro Carvalho Chehab
  2011-06-14 14:48               ` Devin Heitmueller
  2011-06-15 13:43               ` Hans de Goede
  0 siblings, 2 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-14 14:45 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 14-06-2011 11:37, Hans de Goede escreveu:
> Hi,
> 
> On 06/14/2011 04:17 PM, Mauro Carvalho Chehab wrote:
>> Em 14-06-2011 10:52, Devin Heitmueller escreveu:
> 
> <snip>
> 
>> Yes.
>>
>> The default for capture is the one detected via sysfs.
>>
>> The default for playback is not really hw:0,0. It defaults to the first hw: that it is not
>> associated with a video device.
>>
> 
> I have a really weird idea, why not make the default output device be "default", so that
> xawtv will use whatever the distro (or user if overriden by the user) has configured as
> default alsa output device?
> 
> This will do the right thing for pulseaudio and not pulseaudio users alike.

Pulseaudio sucks. See what happens when I pass "-alsa-pb default" argument to pulseaudio:

1) ssh section. User is the same as the console owner:

ALSA lib pulse.c:229:(pulse_connect) PulseAudio: Unable to connect: Connection refused
Cannot open ALSA Playback device default: Connection refused

2) console, with mmap enabled:

Alsa devices: cap: hw:1,0 (/dev/video0), out: default
Access type not available for playback: Invalid argument
Unable to set parameters for playback stream: Invalid argument
Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz with mmap enabled
write error: File descriptor in bad state
...
write error: File descriptor in bad state

3) console, with mmap disabled:

Alsa devices: cap: hw:1,0 (/dev/video0), out: default
Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz
write error: Input/output error
...
write error: Input/output error

Pulseaudio needs first to be fixed in order to work like an alsa device, before
having applications supporting it as default.

Mauro.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 14:45             ` Mauro Carvalho Chehab
@ 2011-06-14 14:48               ` Devin Heitmueller
  2011-06-14 15:51                 ` Mauro Carvalho Chehab
  2011-06-15 13:43               ` Hans de Goede
  1 sibling, 1 reply; 23+ messages in thread
From: Devin Heitmueller @ 2011-06-14 14:48 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Hans de Goede, Linux Media Mailing List

On Tue, Jun 14, 2011 at 10:45 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Pulseaudio sucks. See what happens when I pass "-alsa-pb default" argument to pulseaudio:
>
> 1) ssh section. User is the same as the console owner:
>
> ALSA lib pulse.c:229:(pulse_connect) PulseAudio: Unable to connect: Connection refused
> Cannot open ALSA Playback device default: Connection refused
>
> 2) console, with mmap enabled:
>
> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
> Access type not available for playback: Invalid argument
> Unable to set parameters for playback stream: Invalid argument
> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz with mmap enabled
> write error: File descriptor in bad state
> ...
> write error: File descriptor in bad state
>
> 3) console, with mmap disabled:
>
> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz
> write error: Input/output error
> ...
> write error: Input/output error
>
> Pulseaudio needs first to be fixed in order to work like an alsa device, before
> having applications supporting it as default.

People have been screaming about Pulseaudio for *years*, and those
concerns/complaints have largely fallen on deaf ears.  Lennart works
for Red Hat too - maybe you can convince him to take these issues
seriously.

Devin


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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 14:48               ` Devin Heitmueller
@ 2011-06-14 15:51                 ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-14 15:51 UTC (permalink / raw)
  To: Devin Heitmueller, Hans de Goede; +Cc: Linux Media Mailing List

Em 14-06-2011 11:48, Devin Heitmueller escreveu:
> On Tue, Jun 14, 2011 at 10:45 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Pulseaudio sucks. See what happens when I pass "-alsa-pb default" argument to pulseaudio:
>>
>> 1) ssh section. User is the same as the console owner:
>>
>> ALSA lib pulse.c:229:(pulse_connect) PulseAudio: Unable to connect: Connection refused
>> Cannot open ALSA Playback device default: Connection refused
>>
>> 2) console, with mmap enabled:
>>
>> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
>> Access type not available for playback: Invalid argument
>> Unable to set parameters for playback stream: Invalid argument
>> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz with mmap enabled
>> write error: File descriptor in bad state
>> ...
>> write error: File descriptor in bad state
>>
>> 3) console, with mmap disabled:
>>
>> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
>> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz
>> write error: Input/output error
>> ...
>> write error: Input/output error
>>
>> Pulseaudio needs first to be fixed in order to work like an alsa device, before
>> having applications supporting it as default.

Hans,

Feel free to add pulseaudio support for it, if you want. I don't have time (or
interest) on fixing support for it. Even after fixed, I think that it is safer
to keep the default behavior to directly use alsa, but it is a good idea to
allow users to override.

> People have been screaming about Pulseaudio for *years*, and those
> concerns/complaints have largely fallen on deaf ears. 

Hmm.. I forgot to add a disclaimer on my previous post that my comments express my
own opinion, doesn't reflect the opinion of my employer, and so on ;)

Seriously speaking, I'm not saying that pulseaudio is bad. I just said it didn't
bring anything that _I_ was needing, and that, on the other hand, it broke several
things, requiring some tricks to workaround about it. Well, it works most of the
time, and, on my development machines, I generally don't start X, as I prefer to
work via ssh.

> Lennart works
> for Red Hat too - maybe you can convince him to take these issues
> seriously.

I don't have much contact with Lennart. Red Hat is a big company. I'll talk with
him if I have an opportunity, but I don't think I'll be able to convince him. My
usecase is to use my development machines as if they were servers, working on
them remotely via ssh and tty consoles. This is not the typical usage for pulseaudio. 
Clearly, it were designed thinking on a normal desktop user, as pulseaudio is started 
via X session.

Thanks,
Mauro.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-14 14:45             ` Mauro Carvalho Chehab
  2011-06-14 14:48               ` Devin Heitmueller
@ 2011-06-15 13:43               ` Hans de Goede
  2011-06-15 14:25                 ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-06-15 13:43 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/14/2011 04:45 PM, Mauro Carvalho Chehab wrote:
> Em 14-06-2011 11:37, Hans de Goede escreveu:
>> Hi,
>>
>> On 06/14/2011 04:17 PM, Mauro Carvalho Chehab wrote:
>>> Em 14-06-2011 10:52, Devin Heitmueller escreveu:
>>
>> <snip>
>>
>>> Yes.
>>>
>>> The default for capture is the one detected via sysfs.
>>>
>>> The default for playback is not really hw:0,0. It defaults to the first hw: that it is not
>>> associated with a video device.
>>>
>>
>> I have a really weird idea, why not make the default output device be "default", so that
>> xawtv will use whatever the distro (or user if overriden by the user) has configured as
>> default alsa output device?
>>
>> This will do the right thing for pulseaudio and not pulseaudio users alike.
>
> Pulseaudio sucks.

<sigh> Can we stop the pulseaudio bashing please, it is not really constructive. Pulseaudio
is happily used by many people and is the default on all major distros. So we will need
to support whether you like it or not.

Also usually when people complain about pulseaudio, they are actually being bitten by
bugs elsewhere, but blame pulseaudio, because that seems to be the popular thing
to do. And in this case as usual the problem is with the alsa code in xawtv, not in
pulseaudio. The alsa code in xawtv is buggy in several places, and makes assumptions
it should not (like capture and playback device having a shared period size).

See what happens when I pass "-alsa-pb default" argument to pulseaudio:
>
> 1) ssh section. User is the same as the console owner:
>
> ALSA lib pulse.c:229:(pulse_connect) PulseAudio: Unable to connect: Connection refused
> Cannot open ALSA Playback device default: Connection refused
>

Right, because ConsoleKit ensures that devices like floppydrives, cdroms, audio cards,
webcams, etc. are only available to users sitting behind the console, usually this works
by setting acls on the /dev/foo nodes, so if you're logged in to the console, you can
also access those same devices over ssh, as an unintended side effect. The pulseaudio
daemon actually asks ConsoleKit if the pid on the other end of the unix domain socket
belongs to a session marked as being behind the locale console. Which an ssh session is
not. So this is fully expected.

> 2) console, with mmap enabled:
>
> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
> Access type not available for playback: Invalid argument
> Unable to set parameters for playback stream: Invalid argument
> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz with mmap enabled
> write error: File descriptor in bad state
> ...
> write error: File descriptor in bad state

As already said pulseaudio does not support mmap mode, the reason we are getting
weird errors here is due to a bug in xawtv's alsa params setting code error handling,
which leads to the I don't do mmap mode error not getting caught.

>
> 3) console, with mmap disabled:
>
> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz
> write error: Input/output error
> ...
> write error: Input/output error
>

This is a combination of the assumption there is a shared period size between
the input device and the output device + the broken error handling.
Actually you're lucky, in my case the non proper error handling leads to a
segfault.

I've just pushed an initial set of fixes to the xawtv repo. I'm working on
another set. First thing of that set will be removing the mmap support you
added since it is worthless, quoting from the alsa API documentation:

"If you like to use the compatibility functions in mmap mode, there are
read / write routines equaling to standard read / write transfers. Using
these functions discards the benefits of direct access to memory region.
See the snd_pcm_mmap_readi(), snd_pcm_writei(), snd_pcm_readn() and
snd_pcm_writen() functions."

Note the "Using these functions discards the benefits of direct access to
memory" bit. Properly implementing mmap support is quite hard actually,
and given that we're talking audio here, and thus not large amounts of
we can live with the small memcpy overhead just fine.

On the subject of the current mmap code, it is not only not useful it
is also buggy, asking for:

         snd_pcm_access_mask_set(mask, SND_PCM_ACCESS_MMAP_INTERLEAVED);
         snd_pcm_access_mask_set(mask, SND_PCM_ACCESS_MMAP_NONINTERLEAVED);
         snd_pcm_access_mask_set(mask, SND_PCM_ACCESS_MMAP_COMPLEX);
         err = snd_pcm_hw_params_set_access_mask(handle, params, mask);

Is wrong when using snd_pcm_mmap_readi(), snd_pcm_writei(), when using those
the access mode must be SND_PCM_ACCESS_MMAP_INTERLEAVED, and not one of
the other 2.

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-15 13:43               ` Hans de Goede
@ 2011-06-15 14:25                 ` Mauro Carvalho Chehab
  2011-06-15 14:35                   ` Hans de Goede
  2011-06-15 14:36                   ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-15 14:25 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 15-06-2011 10:43, Hans de Goede escreveu:
> Hi,
> 
> On 06/14/2011 04:45 PM, Mauro Carvalho Chehab wrote:
>> Em 14-06-2011 11:37, Hans de Goede escreveu:
>>> Hi,
>>>
>>> On 06/14/2011 04:17 PM, Mauro Carvalho Chehab wrote:
>>>> Em 14-06-2011 10:52, Devin Heitmueller escreveu:
>>>
>>> <snip>
>>>
>>>> Yes.
>>>>
>>>> The default for capture is the one detected via sysfs.
>>>>
>>>> The default for playback is not really hw:0,0. It defaults to the first hw: that it is not
>>>> associated with a video device.
>>>>
>>>
>>> I have a really weird idea, why not make the default output device be "default", so that
>>> xawtv will use whatever the distro (or user if overriden by the user) has configured as
>>> default alsa output device?
>>>
>>> This will do the right thing for pulseaudio and not pulseaudio users alike.
>>
>> Pulseaudio sucks.
> 
> <sigh> Can we stop the pulseaudio bashing please, it is not really constructive. Pulseaudio
> is happily used by many people and is the default on all major distros.

It is used because distros packaged it, not because people are happy. Most of the time, it
doesn't hurt much, but the lack of pulseaudio is one of the good things of RHEL5. Audio is
more stable there.

> So we will need
> to support whether you like it or not.

Agreed.

> Also usually when people complain about pulseaudio, they are actually being bitten by
> bugs elsewhere, but blame pulseaudio, because that seems to be the popular thing
> to do. And in this case as usual the problem is with the alsa code in xawtv, not in
> pulseaudio. The alsa code in xawtv is buggy in several places, and makes assumptions
> it should not (like capture and playback device having a shared period size).
> 
> See what happens when I pass "-alsa-pb default" argument to pulseaudio:
>>
>> 1) ssh section. User is the same as the console owner:
>>
>> ALSA lib pulse.c:229:(pulse_connect) PulseAudio: Unable to connect: Connection refused
>> Cannot open ALSA Playback device default: Connection refused
>>
> 
> Right, because ConsoleKit ensures that devices like floppydrives, cdroms, audio cards,
> webcams, etc. are only available to users sitting behind the console,

This is a wrong assumption. There's no good reason why other users can't access those
devices. 

> usually this works by setting acls on the /dev/foo nodes,

(Fedora 15)
$ ls -la /dev/video0 /dev/snd/pcmC1D0c 
crw-rw----+ 1 root audio 116, 6 Jun 15 11:14 /dev/snd/pcmC1D0c
crw-rw---- 1 root video 81, 0 Jun 15 11:12 /dev/video0

That seems OK to me. Adding an user at the video and audio group should be enough to allow
somebody else to use it. In my case:

audio:x:63:vdr,mchehab,v4l,mythtv
video:x:39:vdr,mchehab,v4l,mythtv

> so if you're logged in to the console, you can
> also access those same devices over ssh, as an unintended side effect. 

Or if you are at the audio and video groups.

> The pulseaudio
> daemon actually asks ConsoleKit if the pid on the other end of the unix domain socket
> belongs to a session marked as being behind the locale console. Which an ssh session is
> not. So this is fully expected.

This is wrong, as pulseaudio (or ConsoleKit) is not checking if the user has permission
for using the device at the Unix group.

>> 2) console, with mmap enabled:
>>
>> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
>> Access type not available for playback: Invalid argument
>> Unable to set parameters for playback stream: Invalid argument
>> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz with mmap enabled
>> write error: File descriptor in bad state
>> ...
>> write error: File descriptor in bad state
> 
> As already said pulseaudio does not support mmap mode, the reason we are getting
> weird errors here is due to a bug in xawtv's alsa params setting code error handling,
> which leads to the I don't do mmap mode error not getting caught.
> 
>>
>> 3) console, with mmap disabled:
>>
>> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
>> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz
>> write error: Input/output error
>> ...
>> write error: Input/output error
>>
> 
> This is a combination of the assumption there is a shared period size between
> the input device and the output device + the broken error handling.

The code is doing a negotiation, in order to find a period that are acceptable
by both. Ok, there are other ways of doing it, but sharing the same period 
probably means less overhead.

> Actually you're lucky, in my case the non proper error handling leads to a
> segfault.
> 
> I've just pushed an initial set of fixes to the xawtv repo. I'm working on
> another set. First thing of that set will be removing the mmap support you
> added since it is worthless, quoting from the alsa API documentation:
> 
> "If you like to use the compatibility functions in mmap mode, there are
> read / write routines equaling to standard read / write transfers. Using
> these functions discards the benefits of direct access to memory region.
> See the snd_pcm_mmap_readi(), snd_pcm_writei(), snd_pcm_readn() and
> snd_pcm_writen() functions."

The driver is using snd_pcm_mmap_readi() and snd_pcm_writei() at the mmap mode.
The code there came from aplay source code and works properly. Please don't
remove it.

> Note the "Using these functions discards the benefits of direct access to
> memory" bit. Properly implementing mmap support is quite hard actually,
> and given that we're talking audio here, and thus not large amounts of
> we can live with the small memcpy overhead just fine.

If you do some tests with mplayer and a few audio devices, you'll find that
the audio performance may degrade the video streaming up to some point where
you can't see the video stream. It is wise to offer a few options to the
user, in order to allow workaround on that.

> On the subject of the current mmap code, it is not only not useful it
> is also buggy, asking for:
> 
>         snd_pcm_access_mask_set(mask, SND_PCM_ACCESS_MMAP_INTERLEAVED);
>         snd_pcm_access_mask_set(mask, SND_PCM_ACCESS_MMAP_NONINTERLEAVED);
>         snd_pcm_access_mask_set(mask, SND_PCM_ACCESS_MMAP_COMPLEX);
>         err = snd_pcm_hw_params_set_access_mask(handle, params, mask);
> 
> Is wrong when using snd_pcm_mmap_readi(), snd_pcm_writei(), when using those
> the access mode must be SND_PCM_ACCESS_MMAP_INTERLEAVED, and not one of
> the other 2.

This is just what aplay is doing. So, or alsa-utils-1.0.24.1-3.fc15.i686
is broken, or the documentation is outdated.

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-15 14:25                 ` Mauro Carvalho Chehab
@ 2011-06-15 14:35                   ` Hans de Goede
  2011-06-15 14:51                     ` Mauro Carvalho Chehab
  2011-06-15 15:45                     ` Mauro Carvalho Chehab
  2011-06-15 14:36                   ` Mauro Carvalho Chehab
  1 sibling, 2 replies; 23+ messages in thread
From: Hans de Goede @ 2011-06-15 14:35 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/15/2011 04:25 PM, Mauro Carvalho Chehab wrote:
> Em 15-06-2011 10:43, Hans de Goede escreveu:

<snip>

 >> Right, because ConsoleKit ensures that devices like floppydrives, cdroms, audio cards,
 >> webcams, etc. are only available to users sitting behind the console,
 >
 > This is a wrong assumption. There's no good reason why other users can't access those
 > devices.

This is not an assumption, this is a policy decision. The policy is that devices like
audiocards and webcams should be only available to local console users / processes. To
avoid for example someone from spying upon someone else sitting behind the computer.

<snip>

>>> 3) console, with mmap disabled:
>>>
>>> Alsa devices: cap: hw:1,0 (/dev/video0), out: default
>>> Alsa stream started, capturing from hw:1,0, playing back on default at 1 Hz
>>> write error: Input/output error
>>> ...
>>> write error: Input/output error
>>>
>>
>> This is a combination of the assumption there is a shared period size between
>> the input device and the output device + the broken error handling.
>
> The code is doing a negotiation, in order to find a period that are acceptable
> by both. Ok, there are other ways of doing it, but sharing the same period
> probably means less overhead.
>

This is what we call a premature optimization, there is not all that much
overhead here, and demanding that both sizes will support a share period size
may not always fly, and may likely lead to unnecessary large period sizes
and thus too much latency in some cases.

<snip>

> If you do some tests with mplayer and a few audio devices, you'll find that
> the audio performance may degrade the video streaming up to some point where
> you can't see the video stream. It is wise to offer a few options to the
> user, in order to allow workaround on that.

Since we're doing the audio from a separate thread here, and do no syncing
that cannot happen here. Also the right thing to do is to fix the code
to work under all circumstances not offer a gazillion cmdline options
and let the user figure out which ones happen to work for him.

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-15 14:25                 ` Mauro Carvalho Chehab
  2011-06-15 14:35                   ` Hans de Goede
@ 2011-06-15 14:36                   ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-15 14:36 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 15-06-2011 11:25, Mauro Carvalho Chehab escreveu:
> Em 15-06-2011 10:43, Hans de Goede escreveu:
>> Hi,
>>
>> On 06/14/2011 04:45 PM, Mauro Carvalho Chehab wrote:
>>> Em 14-06-2011 11:37, Hans de Goede escreveu:
>>>> Hi,
>>>>
>>>> On 06/14/2011 04:17 PM, Mauro Carvalho Chehab wrote:
>>>>> Em 14-06-2011 10:52, Devin Heitmueller escreveu:
>>>>
>>>> <snip>
>>>>
>>>>> Yes.
>>>>>
>>>>> The default for capture is the one detected via sysfs.
>>>>>
>>>>> The default for playback is not really hw:0,0. It defaults to the first hw: that it is not
>>>>> associated with a video device.
>>>>>
>>>>
>>>> I have a really weird idea, why not make the default output device be "default", so that
>>>> xawtv will use whatever the distro (or user if overriden by the user) has configured as
>>>> default alsa output device?
>>>>
>>>> This will do the right thing for pulseaudio and not pulseaudio users alike.
>>>
>>> Pulseaudio sucks.
>>
>> <sigh> Can we stop the pulseaudio bashing please, it is not really constructive. Pulseaudio
>> is happily used by many people and is the default on all major distros.
> 
> It is used because distros packaged it, not because people are happy. Most of the time, it
> doesn't hurt much, but the lack of pulseaudio is one of the good things of RHEL5. Audio is
> more stable there.
> 
>> So we will need
>> to support whether you like it or not.
> 
> Agreed.
> 
>> Also usually when people complain about pulseaudio, they are actually being bitten by
>> bugs elsewhere, but blame pulseaudio, because that seems to be the popular thing
>> to do. And in this case as usual the problem is with the alsa code in xawtv, not in
>> pulseaudio. The alsa code in xawtv is buggy in several places, and makes assumptions
>> it should not (like capture and playback device having a shared period size).
>>
>> See what happens when I pass "-alsa-pb default" argument to pulseaudio:
>>>
>>> 1) ssh section. User is the same as the console owner:
>>>
>>> ALSA lib pulse.c:229:(pulse_connect) PulseAudio: Unable to connect: Connection refused
>>> Cannot open ALSA Playback device default: Connection refused
>>>
>>
>> Right, because ConsoleKit ensures that devices like floppydrives, cdroms, audio cards,
>> webcams, etc. are only available to users sitting behind the console,
> 
> This is a wrong assumption. There's no good reason why other users can't access those
> devices. 
> 
>> usually this works by setting acls on the /dev/foo nodes,
> 
> (Fedora 15)
> $ ls -la /dev/video0 /dev/snd/pcmC1D0c 
> crw-rw----+ 1 root audio 116, 6 Jun 15 11:14 /dev/snd/pcmC1D0c
> crw-rw---- 1 root video 81, 0 Jun 15 11:12 /dev/video0

One small correction here:

Before loging into the console, the permissions are:

$  ls -la /dev/video0 /dev/snd/pcmC1D0c 
crw-rw---- 1 root audio 116, 6 Jun 15 11:14 /dev/snd/pcmC1D0c
crw-rw---- 1 root video  81, 0 Jun 15 11:12 /dev/video0

After that, acl's were added:

$  ls -la /dev/video0 /dev/snd/pcmC1D0c 
crw-rw----+ 1 root audio 116, 6 Jun 15 11:14 /dev/snd/pcmC1D0c
crw-rw----+ 1 root video  81, 0 Jun 15 11:12 /dev/video0

$ getfacl /dev/video0 /dev/snd/pcmC1D0c
getfacl: Removing leading '/' from absolute path names
# file: dev/video0
# owner: root
# group: video
user::rw-
user:mchehab:rw-
group::rw-
mask::rw-
other::---

# file: dev/snd/pcmC1D0c
# owner: root
# group: audio
user::rw-
user:mchehab:rw-
group::rw-
mask::rw-
other::---

I've logged as mchehab, so, ConsoleKit added a new permission. That's OK,
since it didn't remove the permissions for the group.

in this specific case, what's wrong with pulseaudio is that it is not
honouring the ACL permissions for the group.

Cheers,
Mauro.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-15 14:35                   ` Hans de Goede
@ 2011-06-15 14:51                     ` Mauro Carvalho Chehab
  2011-06-15 15:45                     ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-15 14:51 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 15-06-2011 11:35, Hans de Goede escreveu:
> Hi,
> 
> On 06/15/2011 04:25 PM, Mauro Carvalho Chehab wrote:
>> Em 15-06-2011 10:43, Hans de Goede escreveu:
> 
> <snip>
> 
>>> Right, because ConsoleKit ensures that devices like floppydrives, cdroms, audio cards,
>>> webcams, etc. are only available to users sitting behind the console,
>>
>> This is a wrong assumption. There's no good reason why other users can't access those
>> devices.
> 
> This is not an assumption, this is a policy decision. The policy is that devices like
> audiocards and webcams should be only available to local console users / processes. To
> avoid for example someone from spying upon someone else sitting behind the computer.
> 
> <snip>

The concept behind the policy decision is right, but the policy itself and its 
implementation are wrong.

It makes sense to protect the access to the notebook/desktop microphone and webcam to
the console, but it doesn't make sense to assume that all audio cards and video devices
are microphones and webcams. There are several cases where they aren't, like mythtv/vdr
servers, Video Surveillance devices, TV boards, etc.

Also, I know at least one usecase where a Radio broadcast station is using an USB 
webcam to allow their audience to see what's happening there. So, even the assumption
that all webcams need protection is wrong (In this specific case, they really want
to allow someone else to spy what's happening there ;) ).

That's said, it is OK that the default will be to not allow accessing to them, but
adding someone else to the audio/video group should be enough to allow users to
override this protection.

Mauro.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-15 14:35                   ` Hans de Goede
  2011-06-15 14:51                     ` Mauro Carvalho Chehab
@ 2011-06-15 15:45                     ` Mauro Carvalho Chehab
  2011-06-16 12:29                       ` Hans de Goede
  1 sibling, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-15 15:45 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 15-06-2011 11:35, Hans de Goede escreveu:

>>> This is a combination of the assumption there is a shared period size between
>>> the input device and the output device + the broken error handling.
>>
>> The code is doing a negotiation, in order to find a period that are acceptable
>> by both. Ok, there are other ways of doing it, but sharing the same period
>> probably means less overhead.
>>
> 
> This is what we call a premature optimization, there is not all that much
> overhead here, and demanding that both sizes will support a share period size
> may not always fly, and may likely lead to unnecessary large period sizes
> and thus too much latency in some cases.

I didn't write that part of the code, although I fixed it to work with a device
that provides audio at 32kHZ to play back on my audio card at 44.1kHz, using
software interpolation for the frame rate. It came from Devin's tree, that, in
turn, came from an alsa example (alsa-driver test tool latency.c, according with
the source code) but, IMHO, the code should do:

1) try to find a common buffer size that are acceptable by both drivers,
   as using the same buffer size helps to avoid memcpy's, especially if
   mmap mode is enabled;

2) If the buffer size means that the latency will be more than a reasonable
   time interval [1], then fall back to use different periods;

3) inform the latency introduced by the audio driver, in order to allow
   xawtv to sync audio and video.

(3) is not simple, because alsa doesn't provide a reliable timestamp, but
it shouldn't be that hard to implement some logic that will at least 
compensate for the additional delay introduced by the buffer size.

[1] Not sure what would be a reasonable delay. The original code were using a
buffer of 600 bytes, meaning a delay of 600/4800 (12,5 ms). Maybe twice this
value would still be reasonable. If we don't want to create a complicated
sync processing for (3), the maximum upper limit is the video streaming DQBUF
rate (about 166 ms, for an interlaced video at 30 fps).

Cheers,
Mauro.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-15 15:45                     ` Mauro Carvalho Chehab
@ 2011-06-16 12:29                       ` Hans de Goede
  2011-06-16 14:38                         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-06-16 12:29 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/15/2011 05:45 PM, Mauro Carvalho Chehab wrote:

<snip>

> 1) try to find a common buffer size that are acceptable by both drivers,
>     as using the same buffer size helps to avoid memcpy's, especially if
>     mmap mode is enabled;
>

This is not needed, using the same buffer size does nothing to avoid memcpy's
there are 2 possible scenarios:
1) Use read() + write() like we do now, this means 2 memcpy's in the form
    of copy_to_user to our buffer followed by a copy_from_user, and we don't
    need to care about buffer sizes, we just write the amount of samples we
    managed to read.

2) Properly implemented mmap, in this case we need to do a regular
    memcpy in userspace from the mmap-ed capture buffers to the mmapped
    playback buffers. In this case having indentical buffersizes would
    simplify the code, as it avoids the need to split the memcpy into
    multiple memcpy's when crossing a buffer boundary. But we
    need to handle this case anyways in case we cannot find a shared
    period size. More over mmap mode is a pain and just not worth it IMHO.

> 2) If the buffer size means that the latency will be more than a reasonable
>     time interval [1], then fall back to use different periods;

It is better to just aim for the optimal period size right away, this
greatly simplifies the code, as said before trying to get identical buffer
sizes is premature optimization IMHO. xawtv barely registers in top on
my machine and this includes copying over the actual video data from
/dev/video# to shared memory xv pixmaps. If that part does not even
register imagine how little CPU the audio part is using. There is no
need to make the code more complicated for some theoretical performance
gain here, instead we should KISS.

Note that I've just pushed a patch set which includes rewritten period
/ buf size negotiation and a bunch of cleanups in general. This removes
over 150 lines of code, while at the same time making the code more
flexible. It should now work with pretty much any combination of
input / output device (tested with a bt878 input and intel hda,
usb-audio or pulseaudio output).

I've also changed the default -alsa-pb value to "default" as we should
not be picking something else then the user / distro configured defaults
for output IMHO. The user can set a generic default in alsarc, and override
that on the cmdline if he/she wants, but unless overridden on the cmdline
we should respect the users generic default as specified in his
alsarc.

We could consider making the desired latency configurable, currently
I've hardcoded it to 30 ms (was 38 with the old code on my system) note
that I've chosen to specify the latency in ms rather then in a number
of samples, since it should be samplerate independent IMO.

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-16 12:29                       ` Hans de Goede
@ 2011-06-16 14:38                         ` Mauro Carvalho Chehab
  2011-06-16 15:11                           ` Hans de Goede
  0 siblings, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-16 14:38 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 16-06-2011 09:29, Hans de Goede escreveu:
> Hi,
> 
> On 06/15/2011 05:45 PM, Mauro Carvalho Chehab wrote:
> 
> <snip>
> 
>> 1) try to find a common buffer size that are acceptable by both drivers,
>>     as using the same buffer size helps to avoid memcpy's, especially if
>>     mmap mode is enabled;
>>
> 
> This is not needed, using the same buffer size does nothing to avoid memcpy's
> there are 2 possible scenarios:
> 1) Use read() + write() like we do now, this means 2 memcpy's in the form
>    of copy_to_user to our buffer followed by a copy_from_user, and we don't
>    need to care about buffer sizes, we just write the amount of samples we
>    managed to read.
> 
> 2) Properly implemented mmap, in this case we need to do a regular
>    memcpy in userspace from the mmap-ed capture buffers to the mmapped
>    playback buffers. In this case having indentical buffersizes would
>    simplify the code, as it avoids the need to split the memcpy into
>    multiple memcpy's when crossing a buffer boundary. But we
>    need to handle this case anyways in case we cannot find a shared
>    period size. More over mmap mode is a pain and just not worth it IMHO.
> 
>> 2) If the buffer size means that the latency will be more than a reasonable
>>     time interval [1], then fall back to use different periods;
> 
> It is better to just aim for the optimal period size right away, this
> greatly simplifies the code, as said before trying to get identical buffer
> sizes is premature optimization IMHO. xawtv barely registers in top on
> my machine and this includes copying over the actual video data from
> /dev/video# to shared memory xv pixmaps. If that part does not even
> register imagine how little CPU the audio part is using. There is no
> need to make the code more complicated for some theoretical performance
> gain here, instead we should KISS.

xawtv has an option to use zerocopy, if the X11 v4l driver is loaded and
if the display adapter supports the old overlay mode. It works fine with
a bttv or saa7134 board with an older Nvidia hardware, like FX-5200. This
works also with older ATI hardware.

I intend to make v4l Xorg driver to work with newer display adapters using
texture, but I  didn't have time for it yet. It would be good if we could
do the same for the mmap mode for audio as well, but I don't think this has
a top priority.

> Note that I've just pushed a patch set which includes rewritten period
> / buf size negotiation and a bunch of cleanups in general. This removes
> over 150 lines of code, while at the same time making the code more
> flexible. 

You removed mmap support, but you didn't removed the alsa-mmap option at xawtv.

> It should now work with pretty much any combination of
> input / output device (tested with a bt878 input and intel hda,
> usb-audio or pulseaudio output).

I'll run some tests later with the boards I have here.

> I've also changed the default -alsa-pb value to "default" as we should
> not be picking something else then the user / distro configured defaults
> for output IMHO. The user can set a generic default in alsarc, and override
> that on the cmdline if he/she wants, but unless overridden on the cmdline
> we should respect the users generic default as specified in his
> alsarc.

While pulseaudio refuses to work via ssh, this is actually a very bad idea.
Xawtv is used by developers to test their stuff, and they generally do it
on a remote machine, with the console captured via tty port, in order to
be able to catch panic messages.

For now, please revert this patch. After having pulseaudio fixed to properly
handle the audio group, I'm ok to re-add it.

> We could consider making the desired latency configurable, currently
> I've hardcoded it to 30 ms (was 38 with the old code on my system) note
> that I've chosen to specify the latency in ms rather then in a number
> of samples, since it should be samplerate independent IMO.

Yeah, having latency configurable sounds a good idea to me.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-16 14:38                         ` Mauro Carvalho Chehab
@ 2011-06-16 15:11                           ` Hans de Goede
  2011-06-16 15:35                             ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-06-16 15:11 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/16/2011 04:38 PM, Mauro Carvalho Chehab wrote:
> Em 16-06-2011 09:29, Hans de Goede escreveu:

<snip>

>> Note that I've just pushed a patch set which includes rewritten period
>> / buf size negotiation and a bunch of cleanups in general. This removes
>> over 150 lines of code, while at the same time making the code more
>> flexible.
>
> You removed mmap support, but you didn't removed the alsa-mmap option at xawtv.
>

Ah, fixed.

>> It should now work with pretty much any combination of
>> input / output device (tested with a bt878 input and intel hda,
>> usb-audio or pulseaudio output).
>
> I'll run some tests later with the boards I have here.
>

Thanks.

>> I've also changed the default -alsa-pb value to "default" as we should
>> not be picking something else then the user / distro configured defaults
>> for output IMHO. The user can set a generic default in alsarc, and override
>> that on the cmdline if he/she wants, but unless overridden on the cmdline
>> we should respect the users generic default as specified in his
>> alsarc.
>
> While pulseaudio refuses to work via ssh, this is actually a very bad idea.
> Xawtv is used by developers to test their stuff, and they generally do it
> on a remote machine, with the console captured via tty port, in order to
> be able to catch panic messages.
>

I'm sure developers are quite capable of creating either an .alsarc, pass
the cmdline option, or change pulseaudio's config to accept non local console
connections.

We should try to have sane user oriented defaults, not developer oriented
defaults.

Also not all developers work the same way you do, so having a certain default
just so it matches your work flow also is a bad idea IMHO.

> For now, please revert this patch. After having pulseaudio fixed to properly
> handle the audio group, I'm ok to re-add it.
>
>> We could consider making the desired latency configurable, currently
>> I've hardcoded it to 30 ms (was 38 with the old code on my system) note
>> that I've chosen to specify the latency in ms rather then in a number
>> of samples, since it should be samplerate independent IMO.
>
> Yeah, having latency configurable sounds a good idea to me.

Done.

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-16 15:11                           ` Hans de Goede
@ 2011-06-16 15:35                             ` Mauro Carvalho Chehab
  2011-06-16 18:19                               ` Hans de Goede
  0 siblings, 1 reply; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-16 15:35 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 16-06-2011 12:11, Hans de Goede escreveu:

>>> I've also changed the default -alsa-pb value to "default" as we should
>>> not be picking something else then the user / distro configured defaults
>>> for output IMHO. The user can set a generic default in alsarc, and override
>>> that on the cmdline if he/she wants, but unless overridden on the cmdline
>>> we should respect the users generic default as specified in his
>>> alsarc.
>>
>> While pulseaudio refuses to work via ssh, this is actually a very bad idea.
>> Xawtv is used by developers to test their stuff, and they generally do it
>> on a remote machine, with the console captured via tty port, in order to
>> be able to catch panic messages.
>>
> 
> I'm sure developers are quite capable of creating either an .alsarc, pass
> the cmdline option, or change pulseaudio's config to accept non local console
> connections.

As far as I noticed, most media developers seems to not be comfortable with 
pulseaudio. Using pulseaudio by default would require more experience from
developers, otherwise nobody will be able to properly support it.

For example, the only way I know that works to remove an audio 
module used by pulseaudio is by doing dirty tricks like: 

while : ; do kill pulseaudio; rmmod <audio_module> && break; done

I was told that there's a pactl syntax, but I was never able to find how to
make it work, not even on a local console (and I need it supported via ssh).
>From other posts, other developers at this ML also didn't discover how to do 
it yet.

A pulseaudio or .alsarc config to enable ssh would be fine, but, again,
that's not obvious.

While developers are not comfortable with pulseaudio, turning it into a default
is a bad idea.

> 
> We should try to have sane user oriented defaults, not developer oriented
> defaults.
> 
> Also not all developers work the same way you do, so having a certain default
> just so it matches your work flow also is a bad idea IMHO.
> 
>> For now, please revert this patch. After having pulseaudio fixed to properly
>> handle the audio group, I'm ok to re-add it.
>>
>>> We could consider making the desired latency configurable, currently
>>> I've hardcoded it to 30 ms (was 38 with the old code on my system) note
>>> that I've chosen to specify the latency in ms rather then in a number
>>> of samples, since it should be samplerate independent IMO.
>>
>> Yeah, having latency configurable sounds a good idea to me.
> 
> Done.

Thanks!

Mauro

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-16 15:35                             ` Mauro Carvalho Chehab
@ 2011-06-16 18:19                               ` Hans de Goede
  2011-06-16 18:28                                 ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-06-16 18:19 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, Linux Media Mailing List

Hi,

On 06/16/2011 05:35 PM, Mauro Carvalho Chehab wrote:
> Em 16-06-2011 12:11, Hans de Goede escreveu:

<snip>

> While developers are not comfortable with pulseaudio, turning it into a default
> is a bad idea.
>

1) Some developers are fine with pulseaudio, so please speak on behalf of
    yourself rather then of "developers". Disclaimer I've written patches
    to the alsa code for many a program to work properly with pulseaudio's
    alsa glue layer as well as (re)written the SDL pulseaudio backend.

2) Developers who are not comfortable with pulseaudio now, should better
    learn pulseaudio since that is what the majority of our users are using

3) I'm not making pulseaudio default *at all*. I'm making the "default"
    alsa device the default. Which seems like a very sane default to me.

Does your "default" alsa device happen to point to pulseaudio, and you
don't want that, plenty of options:

1) pass -alsa-pb hw.... to xawtv (requires no pulseaudio knowledge)
2) Set a different default alsa device in .alsarc
    (requires no pulseaudio knowledge, trivial alsa knowledge)
3) Do rpm -e alsa-plugins-pulseaudio

Noe that 3 will permanently make any alsa using app use alsa directly
instead of PA, which seems to be just what you want.

Regards,

Hans

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Some fixes for alsa_stream
  2011-06-16 18:19                               ` Hans de Goede
@ 2011-06-16 18:28                                 ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 23+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-16 18:28 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Devin Heitmueller, Linux Media Mailing List

Em 16-06-2011 15:19, Hans de Goede escreveu:
> Hi,

> 1) pass -alsa-pb hw.... to xawtv (requires no pulseaudio knowledge)
> 2) Set a different default alsa device in .alsarc
>    (requires no pulseaudio knowledge, trivial alsa knowledge)
> 3) Do rpm -e alsa-plugins-pulseaudio
> 
> Noe that 3 will permanently make any alsa using app use alsa directly
> instead of PA, which seems to be just what you want.

Even 3 still doesn't solve rmmod <alsa-module>

On desktops, an yum remove -y pulseaudio work, but bluetooth depends on it,
so this also doesn't provide a solution for notebooks.

Anyway, for now I'll do it on all my instances with pulseaudio, while
nobody provides a solution for rmmod.

Mauro.

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2011-06-16 18:28 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-14  2:01 Some fixes for alsa_stream Mauro Carvalho Chehab
2011-06-14 12:48 ` Hans de Goede
2011-06-14 13:05   ` Mauro Carvalho Chehab
2011-06-14 13:39     ` Mauro Carvalho Chehab
2011-06-14 13:47     ` Hans de Goede
2011-06-14 13:52       ` Devin Heitmueller
2011-06-14 14:17         ` Mauro Carvalho Chehab
2011-06-14 14:37           ` Hans de Goede
2011-06-14 14:45             ` Mauro Carvalho Chehab
2011-06-14 14:48               ` Devin Heitmueller
2011-06-14 15:51                 ` Mauro Carvalho Chehab
2011-06-15 13:43               ` Hans de Goede
2011-06-15 14:25                 ` Mauro Carvalho Chehab
2011-06-15 14:35                   ` Hans de Goede
2011-06-15 14:51                     ` Mauro Carvalho Chehab
2011-06-15 15:45                     ` Mauro Carvalho Chehab
2011-06-16 12:29                       ` Hans de Goede
2011-06-16 14:38                         ` Mauro Carvalho Chehab
2011-06-16 15:11                           ` Hans de Goede
2011-06-16 15:35                             ` Mauro Carvalho Chehab
2011-06-16 18:19                               ` Hans de Goede
2011-06-16 18:28                                 ` Mauro Carvalho Chehab
2011-06-15 14:36                   ` Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox