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

[Reimar Döffinger]

On Sun, Mar 08, 2009 at 01:39:46AM +0300, Andrew Savchenko wrote:
> +// Internal specific data of the filter
> +typedef struct
> +{
> +    int level;          // crossfeed level
> +    int profile;        // profile (easy or normal)
> +    t_bs2bdp filter;    // instance of a library filter
> +} af_bs2b_t;

Where doxygen can process them, comments should be doxygenc-compatible.

> +static af_data_t* play_float(struct af_instance_s*, af_data_t*);
> +static af_data_t* play_s32(struct af_instance_s*, af_data_t*);
> +static af_data_t* play_s24(struct af_instance_s*, af_data_t*);
> +static af_data_t* play_s16(struct af_instance_s*, af_data_t*);
> +static af_data_t* play_s8(struct af_instance_s*, af_data_t*);
> +static af_data_t* play_u8(struct af_instance_s*, af_data_t*);

Why not just move the functions up?

> +    af_bs2b_t* s = af->setup; 
> +    af_bs2b_t *s = af->setup;   // Private data

A more consistent way to write the pointer types would be preferrable.
I always considered the former way quite stupid because of e.g.
int* a, b;
int* a, *b;
int *a, *b;

> +    case AF_CONTROL_COMMAND_LINE:{
> +        int opt[2]={3,1};
> +        if (!sscanf(arg,"%i:%i", &opt[0], &opt[1])) {
> +            af_msg(AF_MSG_ERROR,"[bs2b] Option should be an integer, but '%s' "
> +                "provided.\n",(char*)arg);
> +            return AF_ERROR;
> +        }
> +
> +        /* Sanity checks and assignment for given values */
> +        if (opt[0] < 1 || opt[0] > BS2B_CLEVELS) {
> +            af_msg(AF_MSG_ERROR,"[bs2b] Level must be in range 1..%i, but "
> +                "current value is %i.\nSee man page for details.\n",
> +                BS2B_CLEVELS, opt[0]);
> +            return AF_ERROR;
> +        }
> +        else
> +            s->level=opt[0];
> +
> +        if (opt[1] < 0 || opt[1] > 1) {
> +            af_msg(AF_MSG_ERROR,"[bs2b] Profile must be either 0 or 1, but "
> +                "current value is %i.\nSee man page for details.\n",
> +                opt[1]);
> +            return AF_ERROR;
> +        }
> +        else
> +            s->profile=opt[1];

subopt-helper.h probably provides a nicer way to do this, particularly
if you need more options in the future.

> +        bs2b_set_level(s->filter, s->level + (s->profile) ? BS2B_CLEVELS : 0);

useless () around s->profile.

> +// Deallocate memory and close library 
> +static void uninit(struct af_instance_s* af)
> +{
> +    af_bs2b_t* s;
> +    if(af->data)
> +        free(af->data);
> +    if(af->setup) {
> +        s = af->setup;
> +        if(s->filter)
> +            bs2b_close(s->filter);
> +        free(af->setup);

Assign s right above and use it everywhere, makes it less confusing.

> +static af_data_t* play_float(struct af_instance_s* af, af_data_t* data)
> +{
> +    af_bs2b_t *s = af->setup;   // Private data
> +    float     *a = data->audio; // Audio data
> +    t_bs2bdp   f = s->filter;   // Pointer to the library filter
> +    float   *lim = a + data->len/4 - 1; // end of current chunk
> +    register float *i;
> +
> +    // filter is called for each pair of samples
> +    for(i = a; i < lim; i += 2)
> +        bs2b_cross_feed_f32(f, i);
> +
> +    return data;
> +}

Except for the 24 bit case they could easily be created by a macro...

> +static af_data_t* play_s24(struct af_instance_s* af, af_data_t* data)
> +{
> +    af_bs2b_t *s = af->setup;   // Private data
> +    int8_t    *a = data->audio; // Audio data
> +    t_bs2bdp   f = s->filter;   // Pointer to the library filter
> +    int8_t  *lim = a + data->len - 3; // end of current chunk
> +    register int8_t *i;
> +
> +    // filter is called for each pair of samples
> +    for(i = a; i < lim; i += 6)
> +        bs2b_cross_feed_24(f, i);

lim is wrong (-3 vd. -5).

> +    af->data = calloc(1,sizeof(af_data_t));
> +    af->setup = s = calloc(1,sizeof(af_bs2b_t));
> +    if (af->data == NULL || af->setup == NULL)
> +        return AF_ERROR;

May leak memory

> +    // NULL means failed initialization
> +    if (!(s->filter = bs2b_open()))
> +        return AF_ERROR;

Leaks memory.