[FFmpeg-devel] [PATCH 1/2] opt: Add support to query ranges

[Stefano Sabatini]

On date Monday 2012-11-26 02:07:51 +0100, Michael Niedermayer encoded:
> On Mon, Nov 26, 2012 at 12:32:32AM +0100, Stefano Sabatini wrote:
> > On date Sunday 2012-11-25 17:39:36 +0100, Michael Niedermayer encoded:
> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> [...]
> > > +AVOptionRanges *av_opt_query_ranges(void *obj, const char *key, int flags) {
> > > +    const AVClass *c = *(AVClass**)obj;
> > > +    AVOptionRanges *(*callback)(void *obj, const char *key, int flags) = NULL;
> > > +
> > > +    if(c->version > (52 << 16 | 8 << 9))
> > > +        callback = c->query_ranges;
> > > +
> > > +    if(!callback)
> > > +        callback = av_opt_query_ranges_default;
> > > +
> > > +    return callback(obj, key, flags);
> > > +}
> > 
> > Note that this can't fix the list_devices problem, since that requires
> > an already inited *instance* of a device (what we usually do, we
> > initialize the device given the params, query the device, log the
> > result).  This interface allows to query the class, but not the single
> > instance (which usually requires to provide at least a few params -
> > e.g. the device name).
> 
> obj is a instance, maybe i misunderstand you ?

When the code prints the options in -h full, it is passing a class (so
there is no real object "attached" to the class). I can't imagine how
this could be called on a real instance, assuming that the callback is
called on the class.


> 
> 
> > 
> > > +
> > > +AVOptionRanges *av_opt_query_ranges_default(void *obj, const char *key, int flags) {
> > > +    AVOptionRanges *ranges = av_mallocz(sizeof(*ranges));
> > > +    AVOptionRange **range_array = av_mallocz(sizeof(void*));
> > > +    AVOptionRange *range = av_mallocz(sizeof(*range));
> > > +    const AVOption *field = av_opt_find(obj, key, NULL, 0, 0);
> > > +
> > > +    if(!ranges || !range || !range_array || !field)
> > > +        goto fail;
> > > +
> > > +    ranges->range = range_array;
> > > +    ranges->range[0] = range;
> > > +    ranges->nb_ranges = 1;
> > > +    range->is_range = 1;
> > > +    range->value[0].dbl = field->min;
> > > +    range->value[1].dbl = field->max;
> > > +
> > > +    switch(field->type){
> > > +    case AV_OPT_TYPE_INT:
> > > +    case AV_OPT_TYPE_INT64:
> > > +    case AV_OPT_TYPE_PIXEL_FMT:
> > > +    case AV_OPT_TYPE_SAMPLE_FMT:
> > > +    case AV_OPT_TYPE_FLOAT:
> > > +    case AV_OPT_TYPE_DOUBLE:
> > > +        break;
> > > +    case AV_OPT_TYPE_STRING:
> > > +        range->component[0].dbl = 0;
> > > +        range->component[1].dbl = 0x10FFFF;
> > > +        range->value[0].dbl = -1;
> > > +        range->value[1].dbl = INT_MAX;
> > > +        break;
> > > +    case AV_OPT_TYPE_RATIONAL:
> > > +        range->component[0].dbl = INT_MIN;
> > > +        range->component[1].dbl = INT_MAX;
> > > +        break;
> >
> 
> > > +    case AV_OPT_TYPE_IMAGE_SIZE:
> > > +        range->component[0].dbl = 0;
> > > +        range->component[1].dbl = INT_MAX/128/8;
> > > +        range->value[0].dbl = 0;
> > > +        range->value[1].dbl = INT_MAX/8;
> > > +        break;
> > 
> > Uh? What's the meaning of "range" in case of an image?
> 

> the component range is the range of the component of the vector

which vector?

> the value range is the range in count of pixels

Please document this.

> [...]
> 
> > > +/**
> > > + * A single allowed range of values, or a single allowed value.
> > > + */
> > > +typedef struct AVOptionRange {
> > > +    union {
> > > +        double dbl;     ///< For string ranges this represents the min/max length
> > > +        const char *str;
> > > +    } value[2];
> > > +    union {
> > > +        double dbl;     ///< For string this represents the unicode range for chars, 0-127 limits to ASCII
> > > +    } component[2];
> > > +    int is_range;       ///< 1->range, 0->single value
> > > +} AVOptionRange;
> > 
> > Honestly I find this struct definition a bit confusing (certainly more
> > doxy may help. Also mix/max looks much more readable than
> > component[0]/component[1], and I'm unsure that the range of the single
> > char is really so much important (and even if it is, I'd rather have a
> > dedicated field min/max_char_value rather than abusing the dbl field).
> 
> that would make it quite inconsistent. A string is a vector of symbols
> why should the component range be stored elsewhere for this case ?
> 
> ill look into fixing the other issues you mentioned ASAP
[...]
-- 
FFmpeg = Friendly & Fanciful Magical Pitiful Elegant Gangster