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

[Andrew Savchenko]

On Sunday 15 March 2009 15:44, Andrew Savchenko wrote:
> 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.

Ping.

-- 
Best regards,
Andrew
-------------- 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/20090321/fc01e935/attachment.pgp>