[FFmpeg-devel] [PATCH] Implement libavcodec/opt.[ch] generic options handling

[Stefano Sabatini]

Hi.

On date Thursday 2009-02-05 21:05:22 +0100, Michael Niedermayer encoded:
> On Thu, Feb 05, 2009 at 08:39:30PM +0100, Stefano Sabatini wrote:
> > On date Tuesday 2009-02-03 01:58:49 +0100, Michael Niedermayer encoded:
> > > On Thu, Jan 08, 2009 at 01:51:11AM +0100, Stefano Sabatini wrote:
> > > > On date Monday 2009-01-05 19:52:58 +0100, Michael Niedermayer encoded:
> > > > > On Mon, Jan 05, 2009 at 02:04:11PM +0100, Stefano Sabatini wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > this is my first attempt to move the option handling code from the
> > > > > > applications to the library, it also should make possible to remove
> > > > > > the deprecated lavf AVFormatParameters.
> > > > > > 
> > > > > > Please comment.
> > > > > > 
> > > > > > Regards.
> > > > > > -- 
> > > > > > FFmpeg = Furious Faboulous Majestic Picky Encoding/decoding Gangster
> > > > > 
> > > > > > Index: ffmpeg/libavcodec/opt.c
> > > > > > ===================================================================
> > > > > > --- ffmpeg.orig/libavcodec/opt.c	2009-01-05 13:49:31.000000000 +0100
> > > > > > +++ ffmpeg/libavcodec/opt.c	2009-01-05 13:51:55.000000000 +0100
> > > > > > @@ -114,7 +114,7 @@
> > > > > >          *o_out = o;
> > > > > >      if(!o)
> > > > > >          return AVERROR(ENOENT);
> > > > > > -    if(!val || o->offset<=0)
> > > > > > +    if(!val || (o->type != FF_OPT_TYPE_HANDLER && o->offset<=0))
> > > > > >          return AVERROR(EINVAL);
> > > > > >  
> > > > > >      if(o->type == FF_OPT_TYPE_BINARY){
> > > > > 
> > > > > this patch needs a seperate thread and some justification why it would be
> > > > > a good idea
> > > > > [...]
> > > > 
> > > > Well, why would this be useful?
> > > > 
> > > > This would make possible for example to use the frame size/rate
> > > > abbreviation using av_set_string3(), when it is currently possible
> > > > only in the ff* tools while, having this facility in the library will
> > > > avoid to duplicate the code.
> > > > 
> > > > It also may be used for moving other code (e.g.: opt_preset() and
> > > > opt_target()) from applications to the library, reducing overall code
> > > > duplication and providing an higher level API.
> > > > 
> > > > > and moving the AVOption related code from libav*/utils to libav*/options is
> > > > > fine if that is really just moving and done with svn cp
> > > > 
> > > > Done in a separate thread.
> > > 
> > > what is the advantage of OPT_TYPE_HANDLER over adding 
> > > OPT_TYPE_BLAH FOO and BAR ?
> > > 
> > > similarly what is the advantage of not handling all by OPT_TYPE_HANDLER
> > > and not have any other types ?
> > > 
> > > supporting such a 2 layer system means more complexity than a single
> > > layer whichever of the 2 that would be
> > 
> > The idea was to provide predefined "handlers" for the most used
> > options, while providing to the applications the possibility to extend
> > the option system when they need some very ad-hoc handler, that is an
> > handler for some option which needs to be managed in a very peculiar
> > way.
> > 
> > Pixel formats, frame rate/size abbreviations seem to belong to the
> > latter category.
> > 
> 
> > The alternative is to define a new handler every time we need to deal
> > with a new kind of option, but I don't think this is a good idea since
> > we would end-up with a very huge/messy opt.c.
> 
> why?
> its the same code, just in functions instead of if/switch
> if its more messy than what we have now then you are doing something
> wrong. And in the same way your handlers likely are then sub optimal
> 
> 
> > 
> > The other alternative (all options handled by a
> > already-defined/user-defined handler), looks more appealable but I
> > still didn't considered how much hard it would be not to break
> > backward compatibility implementing it, 
> 
> > so I ended up with this hybrid
> > system.
> 
> anyway, iam against your hybrid hack

My attempt to avoid the hybrid hack ended up in a complete rewrite of
the option system:
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/93469

Now, since it's clear that this system needs somehow to be extended,
my question is how can we extend the new system withouth a complete
rewrite, keeping whenever possible the already existing code?

You suggested a svn copy (for example: libavcodec/opt.h ->
libavutil/opt2.h), followed by the minimal changes, which could break
anyway backward compatibility with the old system.

You seem to suggest the introduction of a set_string function in the
option struct, and to change:

double default_val

to:
const char *default_val

This way the default value would be simply a string, which is set by
the option handler function.

So the first step would be to redefine AVOption like this:

/**
 * AVOption2
 */
typedef struct AVOption2 {
    const char *name;

    /**
     * short English help text
     * @todo What about other languages?
     */
    const char *help;

    /**
     * The offset relative to the context structure where the option
     * value is stored. It should be 0 for named constants.
     */
    int offset;

    /**
     * the default value for scalar options
     */
    const char *default_val;
    double min;                 ///< minimum valid value for the option
    double max;                 ///< maximum valid value for the option

    int flags;
#define AV_OPT_FLAG_ENCODING_PARAM  1   ///< a generic parameter which can be set by the user for muxing or encoding
#define AV_OPT_FLAG_DECODING_PARAM  2   ///< a generic parameter which can be set by the user for demuxing or decoding
#define AV_OPT_FLAG_METADATA        4   ///< some data extracted or inserted into the file like title, comment, ...
#define AV_OPT_FLAG_AUDIO_PARAM     8
#define AV_OPT_FLAG_VIDEO_PARAM     16
#define AV_OPT_FLAG_SUBTITLE_PARAM  32
//FIXME think about enc-audio, ... style flags

    /**
     * The logical unit to which the option belongs. Non-constant
     * options and corresponding named constants share the same
     * unit. May be NULL.
     */
    const char *unit;

    /**
     * the function used to set the option
     */
    int (*set_string)(void *ctx, const char *val);
} AVOption2;

Would be that an acceptable move towards an acceptable solution?

Having just a set_string handler and a string with the default value
(as opposed to my idea of using also a set_default_value() handler)
looks like a good idea.

Still I miss how would be possible to pass some parameters (e.g. min,
max, offset1, offset2) to a specific option, you somehow disliked my
idea of using an opaque field for that.

An alternative solution may be to have something like this:

   int (*set_string)(void *ctx, const char *params, const char *val);

where params is a string with a list of parameters which are defined
like this: "min=32:max=64".

But this still doesn't solve the problem if we need to pass e.g. an
offset, since this is know only during compilation, so you can't do
for example:

"offset=0xaabbccdd"

and can't address the case when an array of values (for example
a list of valid named constants values depending on the option) needs
to be passed when setting the option.

Regards.
-- 
FFmpeg = Freak & Faboulous Mega Patchable Extroverse God