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

[Alban Bedel]

On Thu, 31 May 2007 09:52:24 +0800
"Zuxy Meng" <zuxy.meng at gmail.com> wrote:

> Hi,
> 
> 2007/5/31, Alban Bedel <albeu at free.fr>:
> > On Wed, 30 May 2007 22:53:19 +0800
> > "Zuxy Meng" <zuxy.meng at gmail.com> wrote:
> >
> > > +/// Balance (RW)
> > > +static int mp_property_balance(m_option_t * prop, int action, void *arg,
> > > +                           MPContext * mpctx)
> > > +{
> > > +    float bal;
> > > +
> > > +    if (!arg)
> > > +     return M_PROPERTY_ERROR;
> >
> > This is bad STEP_UP and STEP_DOWN can get called without argument.
> 
> OK. And should STEP_UP and STEP_DOWN ignore the value of *arg
> altogether? mp_property_volume() checkes only the sign of *arg when
> handling STEP_UP and STEP_DOWN.

No, if an argument is present it should be used. IIRC the volume API
doesn't allow to use a step size, that's why only the sign is used.
BTW it would probably be nicer if the API for the balance didn't
suffered from the same pb.
 
> >
> > > +    if (!mpctx->sh_audio || mpctx->sh_audio->channels < 2)
> > > +     return M_PROPERTY_UNAVAILABLE;
> > > +
> > > +    switch (action) {
> > > +    case M_PROPERTY_GET:
> > > +     mixer_getbalance(&mpctx->mixer, arg);
> > > +     return M_PROPERTY_OK;
> > > +    case M_PROPERTY_PRINT:{
> > > +         mixer_getbalance(&mpctx->mixer, &bal);
> > > +         return m_property_float_range(prop, action, arg, &bal);
> > > +     }
> >
> > Perhaps something like "X% left", "center" and "X% right" would be nicer.
> 
> Will in next revision.
> > Otherwise you should better call m_property_float_ro(), that's where
> > PRINT is implemented.
> 
> But mp_property_volume() used m_property_float_range for PRINT....

It could be fixed that's one useless indirection, no big deal however.

> >
> > > +    case M_PROPERTY_STEP_UP:
> > > +        *(float*)arg = -*(float*)arg;
> >
> > You shouldn't modifiy the caller arguments.
> 
> That will be done. But IMHO it isn't guaranteed in many functions
> where M_PROPERTY_CLAMP are used.

You are thinking of SET where the value effectively set can be returned.
But for the other controls you shouldn't.

> > > +     // falling thru
> > > +    case M_PROPERTY_STEP_DOWN:
> > > +     mixer_getbalance(&mpctx->mixer, &bal);
> > > +     bal -= *(float*)arg;
> >
> > STEP_UP and STEP_DOWN should use some default step size if no argument
> > is provided.
> 
> OK. Will be in next revision.
> 
> Again thanks for your review!

No pb :)

	Albeu