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

[Andrew Savchenko]

Hi,

On Sunday 15 March 2009 00:23, Reimar Döffinger wrote:
> 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.

Done.

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

I mimiced style from other filters. No practical reason. Fixed.

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

Agreed. I prefer the latter too, int* a, b; is a misleading 
declaration. Just missed some places. Fixed.

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

But this approach requires options to be named. Well, most of 
mplayer use it anyway... Done, but this bloats object file size a 
bit.

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

Fixed.

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

Fixed.

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

Done.

> > +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).

Yes, thanks for pointing out.

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

If AF_ERROR is returned, mplayer will exit anyway and all memory 
will be freed by OS. Does it make any sence to free that bytes a 
moment earlier? BTW, all other audio filters do the same. Well, 
several of them doesn't check calloc return value at all ;-).

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

The same note as above applies.

-- 
Best regards,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-filter-v5.patch
Type: text/x-diff
Size: 8065 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090315/6810aa79/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-docs-v2.patch
Type: text/x-diff
Size: 1192 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090315/6810aa79/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090315/6810aa79/attachment.pgp>