[MPlayer-dev-eng] [PATCH] libbs2b audio filter

[Diego Biurrun]

On Thu, Mar 26, 2009 at 11:28:28PM +0300, Andrew Savchenko wrote:
> 
> On Wednesday 25 March 2009 23:19, Diego Biurrun wrote:
> > On Mon, Mar 23, 2009 at 06:27:42PM +0300, Andrew Savchenko wrote:
> > > --- libaf/af_bs2b.c	(revision 0)
> > > +++ libaf/af_bs2b.c	(revision 0)
> > > @@ -0,0 +1,210 @@
> > > +    int level;          ///< crossfeed level
> > > +    int profile;        ///< profile (easy or normal)
> > > +    t_bs2bdp filter;    ///< instance of a library filter
> > > +} af_bs2b_t;
> >
> > That's POSIX-reserved namespace, i.e. _t.  Do you need the
> > typedef? What's the problem with just writing struct sometimes?
> 
> Typedef makes code more terse and using typedef for struct is a 
> rather common practice, it is widely used in mplayer too, and in 
> almost all other audio filters in particular.

A lot of things in MPlayer are neither good practice nor good style and
should not be taken as a model.

> And what about af_info_t, af_data_t, etc...?

All of these non-POSIX names should be changed some day.  For FFmpeg we
already did it, it's time to do it in MPlayer.  In the meantime, we
should not add more.

> > > +/// Initialization and runtime control
> > > +static int control(struct af_instance_s *af, int cmd, void
> > > *arg) +{
> > > +    af_bs2b_t *s = af->setup;
> > > +
> > > +    switch(cmd){
> > > +    case AF_CONTROL_REINIT:{
> >
> > Put spaces around the () and before { for greater K&R
> > compatibility. same in other places
> 
> I hope this note affects only outer logical brackets (e.g. switch 
> and if statements), but not function calls or type conversions...

Yes.  K&R places a space after flow control keywords, i.e.
if/while/switch/for, but not after function names.

> > > +/// Description of this filter
> > > +af_info_t af_info_bs2b= {
> >
> > spaces around the = for extra good karma
> 
> Lol, I already feel this karma, done )).

:)

> > This patch reminded me, why do we still have this af_msg
> > special-casing?
> 
> I have absolutely no idea. It is defined at af.h, it is not marked 
> as deprecated, it is used widely in other peaces of code; so I 
> used it for consistency.
> 
> Now I replaced them by mp_msg, because newly introduced af_stats 
> filter do the same. Perhaps af_msg should be declared deprecated 
> and removed from other audio filters.

Any volunteers to remove all of them? :)
 
> --- configure	(revision 29067)
> +++ configure	(working copy)
> @@ -6589,6 +6593,45 @@
>  
> +  if $_pkg_config --exists libbs2b ; then

Gah, pkg-config..

> +    cc_check $_inc_tmp $_ld_tmp && extra_ldflags="$extra_ldflags $_ld_tmp" \
> +      extra_cflags="$extra_cflags $_inc_tmp" && _libbs2b=yes

Missing && ?

> +def_libbs2b="#undef CONFIG_LIBBS2B"
> +if test "$_libbs2b" = yes; then
> +  def_libbs2b="#define CONFIG_LIBBS2B"
> +fi

You can save two lines with && here.

> --- libaf/af_bs2b.c	(revision 0)
> +++ libaf/af_bs2b.c	(revision 0)
> @@ -0,0 +1,215 @@
> +/*
> + * This filter adds support for the Bauer stereophonic-to-binaural
> + * DSP using bs2b library: http://bs2b.sourceforge.net/

"This filter adds support for" is completely redundant.

Compare

  This sentence tells you that what you wrote is redundant.

vs.

  What you wrote is redundant.

Diego