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

[Reimar Döffinger]

On Sun, Mar 15, 2009 at 03:44:40PM +0300, Andrew Savchenko wrote:
> > 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.

Hm, did you test? I thought it was designed so you could leave the names
out if you specified all options. It's been too long though, I don't
remember for sure.

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

Not really, half of it still uses af->setup and the other half s.
Anyway it can be simplified to:
> af_bs2b_t* s = af->setup;
> if (s && s->filter) bs2b_close(s->filter);
> free(s);
> free(af->data);


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

More like "mplayer will crash anyway a moment later. Does it make sense
to check the return value a moment earlier?"
Well, I don't care, I just don't see the point of half-assed error
checking, IMO either do it properly so it actually works correctly or
just don't do it at all.
Above is what I consider the "hide the bug (we wouldn't want it to be
easy to debug after all!) instead of fixing it" approach.

> +        /* check for formats supporteb by libbs2b

supporteD

> + * Curretly (long*) libbs2b filter may be used only on 32-bit 

CurreNtly

> + * architecture, because it uses architecture dependand data

dependEnT

> + * type long for audio data.

Hm, I just notice you did not take into account non-x86 or Windows
(where long is 32 bit on x86_64).

> +#ifdef ARCH_X86_32
> +            case AF_FORMAT_S32_NE:
> +                af->play = play_s32;
> +                break;
> +#endif // ARCH_X86_32
> +            case AF_FORMAT_S24_NE:
> +                af->play = play_s24;
> +                break;
> +            case AF_FORMAT_S16_NE:
> +                af->play = play_s16;
> +                break;
> +            case AF_FORMAT_S8:
> +                af->play = play_s8;
> +                break;
> +            case AF_FORMAT_U8:
> +                af->play = play_u8;
> +                break;
> +            case AF_FORMAT_FLOAT_NE:
> +            default:
> +                af->play = play_float;
> +                af->data->format = AF_FORMAT_FLOAT_NE;
> +                af->data->bps=4;
> +                break;

So I think this should be
> case AF_FORMAT_S32_NE:
> af->play = play_s32;
> // libbs2b incorrectly assumes long 
> if (sizeof(long) == 4) break;
> case AF_FORMAT_FLOAT_NE:
> default:

Apart from that, current SVN has that fixed already and adds functions
to process multiple samples with one function call.

> +        opt_t subopts[] = {

const

> +    AF_FLAGS_NOT_REENTRANT,

This one is not acceptable in new filters, is there any reason this
filter should need it?
AFAICT something like -af bs2b=1,bs2b=0
should work fine, even if it makes little to no sense.

> +.B bs2b[=option1:option2]
> +.IPs level=<1\-3>
> +Cross feed level, higher level means stronger effect (default: 3).
> +.IPs profile=<0\-1>
> +Profile selection, 1 provides softer sound than 0 (default: 1).

Doesn't quite fit together.