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

[Alban Bedel]

On Thu, 15 Nov 2007 18:11:46 +0800
Ulion <ulion2002 at gmail.com> wrote:


> > > +    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.
> 
> I already did that, at:
>     { "sub_source", mp_property_sub_source, CONF_TYPE_INT,
>      M_OPT_RANGE, -1, SUB_SOURCES - 1, NULL },

No, this just fill in the info, you still have to use it. With the
M_PROPERTY_CLAMP() macro for ex.

> >
> > > +        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).
> 
> Indeed the code did return the value effectively set, when arg < 0, it
> must be -1, we set -1, and the effectively set value also -1, no need
> to change the value. when arg >= 0, I have a check call
> sub_source_by_pos to see whether the effectively set value is same
> with the input arg, when they not match, set fail, else set it, the
> input arg is just the effectively set value we make it sure here.

It might not if the input was out of range and needed to be clamped to
the valid range.
 
> >
> > 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
> 
> Any direction for the 'cleaning'? What's the 'messy' detail? I think I
> didn't get your meaning about the 'messy' and 'cleaning', would you
> make it more clear?

Just take a look at the size of the subtitle property, how it have
specific code for each and every type of subtitle, and even code
specific to some particular demuxer!

What is needed first is a single API for all demuxer, having a slightly
different one for each demuxer is pure non sense. Then a clean
abstraction with a consistante API should be made on top of the various
subtitle type.

One way could be to simply have a list of source and add them as needed.
A source could be defined with something like:

struct sub_source {
    // demux, vob, etc
    char* name;
    // Return the available subtitles
    sub_desc_t*  (*get_sub_list)(sub_src_t* src);
    // Select a sub, disable if sub==NULL
    int (*select_sub)(sub_src_t* src, sub_desc_t* sub);
    // Destroy the source
    void (*destroy)(*get_sub_list);
};

Add some simple but generic enouth description of the subs:

struct sub_desc {
    int    id;         // id from the demuxer, etc
    char*  filename;   // for vob, txt files, etc
    char*  lang;       // en,     fr,     de
    char*  region;     // us, gb, be, ca, de, ch
    char** meta;       // key/value pair
};

And suddenly we could:
1/ Cut down the current sub property code to 20% of its current size
2/ Have a clean and independant implementation for each source
   instead of a tangled mess.

Perhaps I miss something, but I fail to see why it would be so hard to
do something like this. Comments from those how know more about the
subtile code would be welcome.

	Albeu