[FFmpeg-devel] Extending AVOption system

[Stefano Sabatini]

On date Monday 2008-06-09 19:23:49 +0200, Michael Niedermayer encoded:
> On Mon, Jun 09, 2008 at 06:16:25PM +0200, Stefano Sabatini wrote:
> > On date Monday 2008-06-09 17:06:55 +0200, Michael Niedermayer encoded:
> > > On Mon, Jun 09, 2008 at 04:45:05PM +0200, Stefano Sabatini wrote:
> > > > On date Sunday 2008-06-08 21:17:51 +0200, Michael Niedermayer encoded:
> > > > > On Sun, Jun 08, 2008 at 07:53:07PM +0200, Stefano Sabatini wrote:
> > > > > > Michael, eventual applications which work with non sorted arrays
> > > > > > won't work anymore with bsearch(), so I have to provide all the required
> > > > > > ifdeffery to keep backward compatibility, right?
> > > > > 
> > > > > hmm, who is using AVOptions arrays outside lav* ?
> > > > 
> > > > Who can say? Anyway since we're indeed breaking backward compatibility
> > > > a major bump seems anyway the right thing to do (again: I can also
> > > > live without such a major bump if you prefer like this, just making my
> > > > point clear).
> > > > 
> > > > Rethinking at it the major bump would be required in libavutil, since
> > > > we're requesting at some point these two things:
> > > > 1) AVClass.option array has to be sorted
> > > > 2) AVClass.option_count has to be correctly specified
> > > > 
> > > > If this is not true then the behaviour of the function which deal with
> > > > AVClass is undefined.
> > > 
> > > Just store the length in the first entry of the AVOption array. No change
> > > to AVClass no version bumps. And the length is where the corresponding array
> > > is.
> > > {" len", NULL, <array size>, FF_OPT_TYPE_CONST,
> > 
> > And still so we're requesting that:
> > 1) AVClass.option array has to be sorted
> > 2) AVClass.option_count has to contain a "len" option

mmh, it was:
2) AVClass.option has to contain a "len" option

> > With this option we save a minor bump for the introduction of
> > option_count, not the major bump, also we're complicating the
> > interface.
> 
> 1. AVClass.option[0].name == " len" -> array is sorted and option[0].offset
>    contains the length
> 2. AVClass.option[0].name != " len" -> array is not sorted and is NULL
>    terminated
> 
> 2 is under #ifdef version < blah

Sorry to bother again, I'm not really happy with this solution too...

This still breaks compatibility in a very subtle way, suppose that a
funky user from Mars already uses AVOption and has an AVClass with the
first item set to

{ " len", 42, ...}

when 42 is not equal to sizeof(options)/sizeof(AVOptions) -1 and
furthermore the array isn't sorted, yes you may wonder why someone
should do such thing but you know people from mars do very weird
things...

Even if possibly there aren't users affected by this problem, we're
still complicating the API, what's even worst we can't define the size of
the array in the options array itself like this:

static AVOption options[]={
 {" len", NULL, sizeof(options)/sizoef(AVOption),
 ...
}

since it issues an incomplete type error.

My solution is to set in the item the offset to -1 meaning
"unspecified", then perform a sort of lazy initialization when
performing the first av_opt_find():

static int av_opt_cmp(const void* key, const void *opt)
{
    return strcmp((char *)key, ((AVOption *)opt)->name);
}

const AVOption *av_find_opt(void *v, const char *name, const char *unit, int mask, int flags){
    AVClass *c= *(AVClass**)v; //FIXME silly way of storing AVClass
    AVOption *o= c->option;

    if (!o)
        return NULL;
    if (!strcmp(o->name, " len") && o->offset < 0) {
        int len = 0;
        /* options array lenght unspecified, compute and set it */
        AVOption *o2 = o+1;
        for (; o2->name; o2++)
            len++;
        o->offset = len;
    }

    if (!strcmp(o->name, " len") && o->offset > 0) { /* array is sorted and contains at least one element */
        o = (AVOption *)bsearch(name, o+1, o->offset, sizeof(AVOption), (int(*)(const void*, const void*))av_opt_cmp);
        for(;o && o->name; o++){
            if(!strcmp(o->name, name) && (!unit || (o->unit && !strcmp(o->unit, unit))) && (o->flags & mask) == flags )
                return o; 
        }
    }
#if LIBAVUTIL_VERSION_MAJOR <= 49
    for(;o && o->name; o++){
        if(!strcmp(o->name, name) && (!unit || (o->unit && !strcmp(o->unit, unit))) && (o->flags & mask) == flags )
            return o;
    }
#endif
    return NULL;
}

which requires also to eliminate the constness of both:

    const struct AVOption *option;

in log.h:AVClass and of:

static const AVOption options[]={

in the various utils.c.

For this I continue to prefer the solution contemplating a major bump.
 
> > Ignore this message if I'm misunderstanding something very trivial,
> 
> > I'll notice it later ;-).
> 
> Its also written in /dev/random ;)

Uh?? :-/

Regards.
-- 
FFmpeg = Free Foolish MultiPurpose EnGine