[MPlayer-dev-eng] [PATCH] New command support select sub by source and ID

[Alban Bedel]

On Sat, 3 Nov 2007 21:14:26 +0800
Ulion <ulion2002 at gmail.com> wrote:

> Thank you for the notify, here's the updated version, combined with a
> sub_source property range fix.

Sorry for the delay, but please don't abuse too much of remainder via
private mail.

> +    case M_PROPERTY_PRINT:
> +        if (!arg)
> +            return M_PROPERTY_ERROR;
> +        return mp_property_sub(prop, M_PROPERTY_PRINT, arg, mpctx);

Calling PRINT on the "sub_source" must print the current subtitle source
name, anything else is non-sense.

> +    case M_PROPERTY_SET:
> +        if (!arg || *(int *) arg < -1 || *(int *) arg >= SUB_SOURCES)
> +            return M_PROPERTY_ERROR;

It should just clamp the parameter into the valid range instead of
returning an error.

> +        if (*(int *) arg < 0)
> +            mpctx->global_sub_pos = -1;
> +        else if (*(int *) arg != sub_source(mpctx)) {
> +            if (*(int *) arg != sub_source_by_pos(mpctx, mpctx->global_sub_indices[*(int *) arg]))
> +                return M_PROPERTY_UNAVAILABLE;
> +            mpctx->global_sub_pos = mpctx->global_sub_indices[*(int
> *) arg];
> +        }
> +        break;

And it should return the value effectively set (like GET do).

> +    case M_PROPERTY_STEP_UP: {
> +        int cur_source = sub_source(mpctx);
> +        for (source = cur_source + 1; source != cur_source; ++source) {
> +            if (source >= SUB_SOURCES) {
> +                mpctx->global_sub_pos = -1;
> +                break;
> +            }
> +            if (source == sub_source_by_pos(mpctx, mpctx->global_sub_indices[source])) {
> +                mpctx->global_sub_pos = mpctx->global_sub_indices[source];
> +                break;
> +            }
> +        }
> +        break;

You should take the optional argument (and its sign) in account if possible.
This mean it's generaly better to join STEP_UP and STEP_DOWN and just compute
the step size with something like this:
(arg ? *(int*)arg : 1)*(action == M_PROPERTY_STEP_UP ? 1 : -1)

I know the sub property don't do it, but it should. This mean here:

> +    --mpctx->global_sub_pos;
> +    return mp_property_sub(prop, M_PROPERTY_STEP_UP, arg, mpctx);

You should pass NULL instead of arg, otherwise things will break if the
sub property get fixed one day.

> +    switch (strlen(prop->name)) {
> +        case 7: source = SUB_SOURCE_VOBSUB; break;
> +        case 8: source = SUB_SOURCE_SUBS; break;
> +        case 9: source = SUB_SOURCE_DEMUX; break;
> +        default: return M_PROPERTY_ERROR;
> +    }

This is really a gratious hack, just use strcmp().

That's all from the property API POV I think. For the subtile code
I can't really comment beside saying that it all (current code and
this patch) look way, way too messy. Is anybody thinking about
cleaning this awful mess?

	Albeu