[MPlayer-dev-eng] [PATCH] Audio balance feature

[Zuxy Meng]

Hi,

2007/6/17, Reimar Doeffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> Hello,
> On Sun, Jun 17, 2007 at 01:00:21PM +0800, Zuxy Meng wrote:
> > +/// Balance (RW)
> > +static int mp_property_balance(m_option_t * prop, int action, void *arg,
> > +                           MPContext * mpctx)
> > +{
> > +    float bal;
> > +
> > +    if (!mpctx->sh_audio || mpctx->sh_audio->channels < 2)
> > +     return M_PROPERTY_UNAVAILABLE;
> > +
> > +    switch (action) {
> > +    case M_PROPERTY_GET:
> > +     if (!arg)
> > +         return M_PROPERTY_ERROR;
> > +     mixer_getbalance(&mpctx->mixer, arg);
> > +     return M_PROPERTY_OK;
> > +    case M_PROPERTY_PRINT: {
> > +         char** str = arg;
> > +         if (!arg)
> > +             return M_PROPERTY_ERROR;
>
> IMO do not add an extra indentation level just because of the { after
> the case.

This is consistent with rest part of code:-) See mp_property_volume()

>
> [...]
> > +void mixer_getbalance(mixer_t *mixer, float *val)
> > +{
> > +  *val = 0.f;
> > +  if(!mixer->audio_out || !mixer->afilter)
> > +    return;
>
> What's the point in checking for audio_out?

Does mixer->afilter != 0 imply mixer->audio_out != 0? If so then this
would be redundant. I simply did the same check as mixer_getvolume.

> > +void mixer_setbalance(mixer_t *mixer, float val)
> > +{
> > +  float level[AF_NCH];
> > +  int i, nout;
> > +  af_control_ext_t arg_ext = { .arg = level };
> > +  af_instance_t* af_pan_balance;
> > +
> > +  if(!mixer->audio_out || !mixer->afilter)
> > +    return;
> > +
> > +  if (af_control_any_rev(mixer->afilter,
> > +     AF_CONTROL_PAN_BALANCE | AF_CONTROL_SET, &val))
> > +    return;
> > +
> > +  if (!(af_pan_balance = af_add(mixer->afilter, "pan"))) {
> > +    mp_msg(MSGT_GLOBAL, MSGL_ERR, MSGTR_NoBalance);
> > +    return;
> > +  }
> > +
> > +  af_init(mixer->afilter);
> > +  /* make all other channels pass thru since by default pan blocks all */
> > +  memset(level, 0, sizeof(level));
> > +  for (i = 2; i < AF_NCH; i++) {
> > +    arg_ext.ch = i;
> > +    level[i] = 1.f;
> > +    af_pan_balance->control(af_pan_balance,
> > +     AF_CONTROL_PAN_LEVEL | AF_CONTROL_SET, &arg_ext);
> > +    level[i] = 0.f;
> > +  }
>
> I think this should be done in af_pan as well.

You mean to change to default setting of af_pan?

> I'd strongly suggest
> using the same code as for volume control in mixer.c, possibly even
> factoring it out into a new function (e.g. af_control_rev_autoadd).

Maybe later:-)

> Printing a message when a filter is inserted (as for volume control)
> might be nice, too, since it sometimes has weird side effects.

OK.

-- 
Zuxy
Beauty is truth,
While truth is beauty.
PGP KeyID: E8555ED6