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

[Andrew Savchenko]

Hi,

On Wednesday 25 March 2009 23:19, Diego Biurrun wrote:
> On Mon, Mar 23, 2009 at 06:27:42PM +0300, Andrew Savchenko wrote:
> > --- configure	(revision 29040)
> > +++ configure	(working copy)
>
> OK, but you will have to update some variable names.

Done.

> > @@ -6590,6 +6594,46 @@
> >
> > +if test "$_libbs2b" = yes; then
> > +  def_libbs2b="#define CONFIG_LIBBS2B"
> > +else
> > +  def_libbs2b="#undef CONFIG_LIBBS2B"
>
> Initialize this to #undef and you can get rid of the else.

ok

> > --- Makefile	(revision 29040)
> > +++ Makefile	(working copy)
> > @@ -331,6 +331,8 @@
> >
> > +SRCS_COMMON-$(LIBBS2B)               += libaf/af_bs2b.c
> > +
> >  # These filters use private headers and do not work with
> > shared libavcodec. SRCS_COMMON-$(LIBAVCODEC_A)          +=
> > libaf/af_lavcac3enc.c \ libmpcodecs/vf_fspp.c \
>
> This is not in alphabetical order.

Fixed. Originally I was confused by the comment above, but it 
applies only to single directive below, now to all of them ).

> > --- libaf/af_bs2b.c	(revision 0)
> > +++ libaf/af_bs2b.c	(revision 0)
> > @@ -0,0 +1,210 @@
> > +
> > +/// Internal specific data of the filter
> > +typedef struct
> > +{
>
> K&R: typedef struct {

ok

> > +    int level;          ///< crossfeed level
> > +    int profile;        ///< profile (easy or normal)
> > +    t_bs2bdp filter;    ///< instance of a library filter
> > +} af_bs2b_t;
>
> That's POSIX-reserved namespace, i.e. _t.  Do you need the
> typedef? What's the problem with just writing struct sometimes?

Typedef makes code more terse and using typedef for struct is a 
rather common practice, it is widely used in mplayer too, and in 
almost all other audio filters in particular. And what about 
af_info_t, af_data_t, etc...?

Those repeating struct keywords look rather annoying to me, I 
dislike them and try to avoid. But you are not the first person 
objecting this kind of approach, so I'll remove typedef. Whilst I 
dislike this solution, such kind of things are not principal for 
me, because they doesn't affect bytecode at all.

> > +    af_msg(AF_MSG_ERROR,"[bs2b] Level must be in range 1..%i,
> > but " +        "current value is %i.\n", BS2B_CLEVELS, val);
>
> weird indentation, same below

Just normal 4 spaces 8-). I realligned code to the 1st character 
after opening bracket.

> > +/// Initialization and runtime control
> > +static int control(struct af_instance_s *af, int cmd, void
> > *arg) +{
> > +    af_bs2b_t *s = af->setup;
> > +
> > +    switch(cmd){
> > +    case AF_CONTROL_REINIT:{
>
> Put spaces around the () and before { for greater K&R
> compatibility. same in other places

I hope this note affects only outer logical brackets (e.g. switch 
and if statements), but not function calls or type conversions...

> > +        format = ((af_data_t*)arg)->format;
> > +        af->data->rate = ((af_data_t*)arg)->rate;
> > +        af->data->nch  = 2;     // bs2b is useful only for
> > 2ch audio +        af->data->bps  = ((af_data_t*)arg)->bps;
> > +        af->data->format = format;
>
> align

Done.

> > +    if(s && s->filter)
>
> K&R: if (
>
> > +    af->control = control;
> > +    af->uninit = uninit;
> > +    af->mul = 1;

Done.

> > +    af->data = calloc(1,sizeof(af_data_t));
> > +    af->setup = s = calloc(1,sizeof(af_bs2b_t));
>
> align

Hmm, it seems you reviewed libbs2b-filter-v7.patch instead of v8. 
These statements are included inside if conditions in the latter 
patch.

> > +/// Description of this filter
> > +af_info_t af_info_bs2b= {
>
> spaces around the = for extra good karma

Lol, I already feel this karma, done )).

[...]
> > +.B bs2b[=option1:option2]
> > +Bauer stereophonic to binaural transformation using libbs2b.
> > +It improves headphones listening making it similar to sound
> > from +loudspeakers, allowing each ear to hear both channels,
> > taking +into account distance difference and head shadowing
> > effect.
>
> Hmmm, that sounds a bit awkward, how about
>
>   Improves the headphone listening experience by making the
> sound similar to that from loudspeakers, allowing each ear to
> hear both channels and taking into account the distance
> difference and the head shadowing effect.

I'm gladly accept this variant. Hmm, it seems that my phrasing in 
English is awkward sometimes, maybe this is a price for leaving in 
non-english speaking society. It seems I should read more 
*non-technical* literature ).

> > +It is applicable only for 2 channel audio.
>
> s/for/to/

ok.

> This patch reminded me, why do we still have this af_msg
> special-casing?

I have absolutely no idea. It is defined at af.h, it is not marked 
as deprecated, it is used widely in other peaces of code; so I 
used it for consistency.

Now I replaced them by mp_msg, because newly introduced af_stats 
filter do the same. Perhaps af_msg should be declared deprecated 
and removed from other audio filters.

-- 
Best regards,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-configure-v4.patch
Type: text/x-diff
Size: 2750 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090326/2863115d/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-filter-v9.patch
Type: text/x-diff
Size: 7664 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090326/2863115d/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libbs2b-docs-v4.patch
Type: text/x-diff
Size: 952 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090326/2863115d/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/20090326/2863115d/attachment.pgp>