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

[Zuxy Meng]

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.

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

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

> > +     // 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!
-- 
Zuxy
Beauty is truth,
While truth is beauty.
PGP KeyID: E8555ED6