Pull request for alsa-plugins

Pull request for alsa-plugins

[Lennart Poettering]

Takashi, Jaroslav,

please pull and merge a series of 26 patches for alsa-plugins'
PulseAudio driver which I prepared in this repository of mine:

    git://git.0pointer.de/alsa-plugins.git

gitweb is here:

    http://git.0pointer.de/?p=alsa-plugins.git

There are quite a few bug fixes and feature enhancements among those
patches. It also includes some clean-ups in the coding
style. i.e. previously the indenting of the PA driver was pure chaos,
every single patch seemed to introduce a new indentation width. The
first patch in this series simply reindents the driver to Linux
style. All other patches are on top of that one.

Here's the shortlog:

Lennart Poettering (26):
      Reindent to Linux kernel style
      Add Emacs-style /*-*- linux-c -*-*/ header comment
      Make pulse_new() a proper C function
      Don't modify the SIGPIPE handler
      Call pa_context_disconnect() explicitly
      use SNDERR instead of fprintf to print error messages
      Support S32 sample types
      Add trailing NUL character to snprintf output
      Get rid of pulse_poll_revents()
      Add more error checking
      Remove fix for bug 0003470
      Rework hardware parameter selection
      A bag of minor clean ups for ctl_pulse.c
      Make pulse_ext_callback const
      Drop our own implementation of the poll() callbacks
      A bag of minor clean-ups for pulse.c
      Split out O_NONBLOCK setting into seperate function
      Save a byte of memory
      Adjust buffering metrics to match what PA internally uses
      Make sure we always have a sensible channel mapping
      Use PA_STREAM_EARLY_REQUESTS if available
      Use S32/FLOAT32 only where available in the PA libs
      Add const to our snd_pcm_ioplug_callback_t instances
      Don't implement our own poll handlers, we can use the default
      ones
      Remove our own poll handler implementation entirely
      A bag of clean-ups for pcm_ctl.c

Those patches only touch the pulse/ subdir. 

The tree is freshly rebased against current alsa-plugins master.

May I ask you to make me the "semi-official" maintainer of this
driver? I.e. I'd like to be consulted (as in 'Signed-Off-by') before
any patches for it are merged?

If requested I can also mail this as series of 26 seperate patches.

Thank you,

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Pull request for alsa-plugins

[Takashi Iwai]

At Wed, 3 Sep 2008 21:17:41 +0200,
Lennart Poettering wrote:
> 
> Takashi, Jaroslav,
> 
> please pull and merge a series of 26 patches for alsa-plugins'
> PulseAudio driver which I prepared in this repository of mine:
> 
>     git://git.0pointer.de/alsa-plugins.git

Thanks, pulled in, pushed out now.

At the next time, could you add our sign-off to each commit?
I don't care much about it for non-kernel codes, but certainly better
with it.

Also, please add the branch name in the same line of git://... above.
The recent git requires the branch name explicitly to pull.

> gitweb is here:
> 
>     http://git.0pointer.de/?p=alsa-plugins.git
> 
> There are quite a few bug fixes and feature enhancements among those
> patches. It also includes some clean-ups in the coding
> style. i.e. previously the indenting of the PA driver was pure chaos,
> every single patch seemed to introduce a new indentation width. The
> first patch in this series simply reindents the driver to Linux
> style. All other patches are on top of that one.

Oh yes, that was ugly.


> Here's the shortlog:
> 
> Lennart Poettering (26):
>       Reindent to Linux kernel style
>       Add Emacs-style /*-*- linux-c -*-*/ header comment
>       Make pulse_new() a proper C function
>       Don't modify the SIGPIPE handler
>       Call pa_context_disconnect() explicitly
>       use SNDERR instead of fprintf to print error messages
>       Support S32 sample types
>       Add trailing NUL character to snprintf output
>       Get rid of pulse_poll_revents()
>       Add more error checking
>       Remove fix for bug 0003470
>       Rework hardware parameter selection
>       A bag of minor clean ups for ctl_pulse.c
>       Make pulse_ext_callback const
>       Drop our own implementation of the poll() callbacks
>       A bag of minor clean-ups for pulse.c
>       Split out O_NONBLOCK setting into seperate function
>       Save a byte of memory
>       Adjust buffering metrics to match what PA internally uses
>       Make sure we always have a sensible channel mapping
>       Use PA_STREAM_EARLY_REQUESTS if available
>       Use S32/FLOAT32 only where available in the PA libs
>       Add const to our snd_pcm_ioplug_callback_t instances
>       Don't implement our own poll handlers, we can use the default
>       ones
>       Remove our own poll handler implementation entirely
>       A bag of clean-ups for pcm_ctl.c

Thanks for your work.

One of my concerns in the current pulse code is the many use of
assert().  In many cases, assert() is a wrong choice.  For example,
checking the return value of malloc in assert() is definitely wrong.
It should be always checked, and should return an error instead of
aborting the program.

Yes, alsa-lib has also many assert() although I removed already many.
Some of them may be still wrong ones.  Hopefully this can be sorted
out later.

> Those patches only touch the pulse/ subdir. 
> 
> The tree is freshly rebased against current alsa-plugins master.
> 
> May I ask you to make me the "semi-official" maintainer of this
> driver? I.e. I'd like to be consulted (as in 'Signed-Off-by') before
> any patches for it are merged?

Well, the patches can go in by any ALSA developers, so it can't be
forced *always* through you.  But, I'll mail you pulse-related patches
before reviewing and merging.


Takashi

Re: Pull request for alsa-plugins

[Colin Guthrie]

Takashi Iwai wrote:
> One of my concerns in the current pulse code is the many use of
> assert().  In many cases, assert() is a wrong choice.  For example,
> checking the return value of malloc in assert() is definitely wrong.
> It should be always checked, and should return an error instead of
> aborting the program.
> 
> Yes, alsa-lib has also many assert() although I removed already many.
> Some of them may be still wrong ones.  Hopefully this can be sorted
> out later.

There has been a rather large thread on this on PA mailing list over the 
last couple days :)

http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/2014

I personally got some stuff wrong, but learned from Lennart's responses.

Certainly I think a lot of the cases where this plugin could have 
previously hit an assert have now been removed. Not sure about the 
malloc asserts() tho'...

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
   Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
   Mandriva Linux Contributor [http://www.mandriva.com/]
   PulseAudio Hacker [http://www.pulseaudio.org/]
   Trac Hacker [http://trac.edgewall.org/]

Re: Pull request for alsa-plugins

[Lennart Poettering]

On Thu, 04.09.08 09:48, Takashi Iwai (tiwai@suse.de) wrote:

> > please pull and merge a series of 26 patches for alsa-plugins'
> > PulseAudio driver which I prepared in this repository of mine:
> > 
> >     git://git.0pointer.de/alsa-plugins.git
> 
> Thanks, pulled in, pushed out now.
> 
> At the next time, could you add our sign-off to each commit?
> I don't care much about it for non-kernel codes, but certainly better
> with it.

Sure, will do.

> Also, please add the branch name in the same line of git://... above.
> The recent git requires the branch name explicitly to pull.

Will do.

> One of my concerns in the current pulse code is the many use of
> assert().  In many cases, assert() is a wrong choice.  For example,
> checking the return value of malloc in assert() is definitely wrong.
> It should be always checked, and should return an error instead of
> aborting the program.

I think I fixed every single assert() on malloc, OOM should now be
handled in similar way to most other code in in
alsa-lib/-plugins. 

Still, there are quite a few asserts left in the pulse driver which
check validity of function parameters. But they should only be hit on
internal programming errors and as such I think it makes a lot of
sense to leave them in.

> > Those patches only touch the pulse/ subdir. 
> > 
> > The tree is freshly rebased against current alsa-plugins master.
> > 
> > May I ask you to make me the "semi-official" maintainer of this
> > driver? I.e. I'd like to be consulted (as in 'Signed-Off-by') before
> > any patches for it are merged?
> 
> Well, the patches can go in by any ALSA developers, so it can't be
> forced *always* through you.  But, I'll mail you pulse-related patches
> before reviewing and merging.

Thanks a lot!

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4