[MPlayer-dev-eng] [PATCH] libbs2b: new api

[Andrew Savchenko]

Hi,

On Monday 20 April 2009, Uoti Urpala wrote:
> On Mon, 2009-04-20 at 22:45 +0400, Andrew Savchenko wrote:
> > On Friday 03 April 2009, Andrew Savchenko wrote:
> > > Hello,
> > >
> > > recently api of libbs2b was changed, according to svn log it
> > > should be rather stable from r89. Audio filter update is
> > > attached.
> >
> > Ping. Proposed patch also applies for official libs2b-3.0.0
> > release and fully utilizes its functionality.
> >
> > Please, review. Will apply in a weak if there are no
> > objections.
>
> There are no configure changes. Shouldn't the test be updated to
> reject the old library version?

Configure already rejects older versions, it requires >=3.0.0. So 
all official releases prior to current will be rejected. The 
delicacy is that in libbs2b svn version number was changed to 
3.0.0 slightly before 3.0.0 official release. Anyway there will be 
no problem with releases and only svn head from svn trunk is 
supported.

> The documentation specifies certain parameter ranges, but the
> actual code uses range defines from the library. I don't know
> whether those are supposed to always stay constant; perhaps
> there should be some mention of that?

I don't know either. Anyway it is supposed to keep up-to-date sync 
with library, so if changes will occur they should be fixed 
relatively shortly. That's why I'm not sure this issue is worth to 
be mentioned in the manual page, of course this point of view is 
discussable.

> +                   "[bs2b] Requested sample rate is out of
> bounds [%i..%i] Hz.\n"
>
> I think this should say what the requested rate was.

Agreed. Fixed.

> Some code formatting issues that occur multiple times:
>
> +           "[bs2b] Feed level must be in range [%i..%i], but
> current value is %i.\n",
>
> You should use "%d" instead of "%i" like most of MPlayer code
> (and most C code I've seen in general).

Hm, I really can't see how it differs, standard allows both of 
them. Anyway there is zero difference for the bytecode, so let it 
be as you wish ).

> +            else
> +            if (!strcmp(s->profile,"cmoy"))
>
> "else if" should be on one line.

No, no, no. The idea is to keep all if statements at the same 
alignment level, otherwise it will be mess to read. Currently they 
have nice and easy-to-understand-the-code indentation. Statements 
are logically equal and there is no reason to accent one of them. 
You may consider this as swith statement for strings.

Yes, I know this is not strict K&R indentation but sometimes logics 
should override common rules.

> +                bs2b_set_level(s->filter,BS2B_JMEIER_CLEVEL);
>
> Space missing after ",".

Ok.

Thanks for review, new version is attached.

-- 
Best regards,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-newapi-v2.patch
Type: text/x-diff
Size: 7804 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090421/ed1ea1d6/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090421/ed1ea1d6/attachment.pgp>