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

[Andrew Savchenko]

Hi,

On Saturday 21 March 2009 17:47, Reimar Döffinger wrote:
> 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.

Yes, I did. This helper requires the names to be specified in both 
opt_t structure and at the command line.

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

Ok, I got the point. Also I looked in the C standard (paragraph 
7.20.3.2), it allows free(NULL) calls (they must do nothing), so I 
eliminated checks like if (!x) free(x);.

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

Imho, this is not half-check. The reason of this check is to detect 
memory allocation problem and initialize shutdown sequence. I 
agree, this is not the best solution. If memory allocation or 
reallocation error occurs this means something is going very 
wrong: system is either running into OOM condition or some 
hardware error/severe security violation happens. Under such 
conditions normal operation is no longer possible. 

IMO the best solution is to replace all occurences of 
malloc/calloc/realloc to their safe wrappers, e.g. 
xmalloc/xcalloc/xrealloc with return value check and fatal error 
message and exit() call if NULL is returned; if sane exit attempt 
is really necessary, signal may be issued and later proceed by 
sighandler. I do not know why this approach isn't used throughout 
all mplayer's code. Performance shouldn't be an issue because 
frequent memory allocations/reallocation will harm it by degree 
harder. This solution will also diminish code duplication by a 
bit.

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

Ok, I'll run spell checker next time ;-).

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

Current SVN fixes this. So I'll stick to that version.

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

Very well. I'll use current SVN from now. Version check is added to 
configure. Currently I do not resize buffer, maybe some minimal 
length should be set.

> > +        opt_t subopts[] = {
>
> const

I can't do this because initializers are not constant.

> > +    AF_FLAGS_NOT_REENTRANT,
>
> This one is not acceptable in new filters, is there any reason
> this filter should need it?

The only reason is that I see this meaningless. Technically this is 
possible, so there is no harm if I'll add 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.

Sorry, I can't understand you. Do you mean description is bad or 
inconsistent? I tried to improve it. Comments are welcome.

-- 
Best regards,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-configure-v2.patch
Type: text/x-diff
Size: 2734 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090323/df94662d/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-filter-v6.patch
Type: text/x-diff
Size: 7496 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090323/df94662d/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-docs-v3.patch
Type: text/x-diff
Size: 922 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090323/df94662d/attachment-0002.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/20090323/df94662d/attachment.pgp>