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

[Diego Biurrun]

On Mon, Mar 23, 2009 at 06:27:42PM +0300, Andrew Savchenko wrote:
> 
> --- configure	(revision 29040)
> +++ configure	(working copy)

OK, but you will have to update some variable names.

> @@ -6590,6 +6594,46 @@
>  
> +if test "$_libbs2b" = yes; then
> +  def_libbs2b="#define CONFIG_LIBBS2B"
> +else
> +  def_libbs2b="#undef CONFIG_LIBBS2B"

Initialize this to #undef and you can get rid of the else.

> --- Makefile	(revision 29040)
> +++ Makefile	(working copy)
> @@ -331,6 +331,8 @@
>  
> +SRCS_COMMON-$(LIBBS2B)               += libaf/af_bs2b.c
> +
>  # These filters use private headers and do not work with shared libavcodec.
>  SRCS_COMMON-$(LIBAVCODEC_A)          += libaf/af_lavcac3enc.c \
>                                          libmpcodecs/vf_fspp.c \

This is not in alphabetical order.

> --- libaf/af_bs2b.c	(revision 0)
> +++ libaf/af_bs2b.c	(revision 0)
> @@ -0,0 +1,210 @@
> +
> +/// Internal specific data of the filter
> +typedef struct
> +{

K&R: typedef struct {

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

> +    af_msg(AF_MSG_ERROR,"[bs2b] Level must be in range 1..%i, but "
> +        "current value is %i.\n", BS2B_CLEVELS, val);

weird indentation, same below

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

> +        format = ((af_data_t*)arg)->format;
> +        af->data->rate = ((af_data_t*)arg)->rate;
> +        af->data->nch  = 2;     // bs2b is useful only for 2ch audio
> +        af->data->bps  = ((af_data_t*)arg)->bps;
> +        af->data->format = format;

align

> +    if(s && s->filter)

K&R: if (

> +    af->control = control;
> +    af->uninit = uninit;
> +    af->mul = 1;
> +    af->data = calloc(1,sizeof(af_data_t));
> +    af->setup = s = calloc(1,sizeof(af_bs2b_t));

align

> +/// Description of this filter
> +af_info_t af_info_bs2b= {

spaces around the = for extra good karma

And yes, I promise to shut up about nits now :)

> +    "Bauer stereophonic-to-binaural audio filter",
> +    "bs2b",
> +    "Andrew Savchenko",
> +    "",
> +    AF_FLAGS_REENTRANT,
> +    af_open
> +};
> --- DOCS/man/en/mplayer.1	(revision 29040)
> +++ DOCS/man/en/mplayer.1	(working copy)
> @@ -5058,6 +5058,22 @@
>  .PD 1
>  .
>  .TP
> +.B bs2b[=option1:option2]
> +Bauer stereophonic to binaural transformation using libbs2b.
> +It improves headphones listening making it similar to sound from
> +loudspeakers, allowing each ear to hear both channels, taking 
> +into account distance difference and head shadowing effect.

Hmmm, that sounds a bit awkward, how about

  Improves the headphone listening experience by making the sound
  similar to that from loudspeakers, allowing each ear to hear both
  channels and taking into account the distance difference and the
  head shadowing effect.

> +It is applicable only for 2 channel audio.

s/for/to/


This patch reminded me, why do we still have this af_msg special-casing?

Diego