[PATCH] Implement libavcodec/opt.[ch] generic options handling
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
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 [...] 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 -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I count him braver who overcomes his desires than him who conquers his enemies for the hardest victory is over self. -- Aristotle
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. Regards. -- FFmpeg = Friendly Funny Most Pacific Ecletic Guru
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates
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. 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. Regards. -- FFmpeg = Forgiving Funny Merciful Prodigious Esoteric Gem
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
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
On Tue, Oct 13, 2009 at 01:58:24AM +0200, Stefano Sabatini wrote: [...]
} AVOption2;
Would be that an acceptable move towards an acceptable solution?
honestly this all is very messy maybe we can just repace default_val by an union of double and char* and add a flag to indicate that its a char*, this with some luck should maintain compaibility. and i do not see why a overrideable set_string() would make sense add your code to the existing set_string()
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.
i dont understand what you want to do with set_string() here?
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".
i dont understand what this is supposed to be [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
On date Tuesday 2009-10-13 12:04:01 +0200, Michael Niedermayer encoded:
On Tue, Oct 13, 2009 at 01:58:24AM +0200, Stefano Sabatini wrote: [...]
} AVOption2;
Would be that an acceptable move towards an acceptable solution?
honestly this all is very messy maybe we can just repace default_val by an union of double and char* and add a flag to indicate that its a char*, this with some luck should maintain compaibility.
and i do not see why a overrideable set_string() would make sense add your code to the existing set_string()
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.
i dont understand what you want to do with set_string() here? [...]
Let's put it in another way: consider this example, I want to set an option which takes in input a string representing a color, and I want to put the corresponding color somewhere inside a context. Without a set_string callback, the only possible way to do it would be to extend opt.c, which is exactly what I want to avoid. Why? Because the function I'll use for setting that option may not be defined in libavcodec. Also I don't want to make opt.c grow and grow every time I need a specialized function for setting an option. Show me how to do it without to extend opt.c and I'll be happy, just telling that such a feature isn't worth the trouble is not an option (pun intended). Regards. -- FFmpeg = Free and Fantastic Multimedia Pitiless Ephemeral Gem
On Tue, Oct 13, 2009 at 11:38:37PM +0200, Stefano Sabatini wrote:
On date Tuesday 2009-10-13 12:04:01 +0200, Michael Niedermayer encoded:
On Tue, Oct 13, 2009 at 01:58:24AM +0200, Stefano Sabatini wrote: [...]
} AVOption2;
Would be that an acceptable move towards an acceptable solution?
honestly this all is very messy maybe we can just repace default_val by an union of double and char* and add a flag to indicate that its a char*, this with some luck should maintain compaibility.
and i do not see why a overrideable set_string() would make sense add your code to the existing set_string()
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.
i dont understand what you want to do with set_string() here? [...]
Let's put it in another way: consider this example, I want to set an option which takes in input a string representing a color, and I want to put the corresponding color somewhere inside a context.
Without a set_string callback, the only possible way to do it would be to extend opt.c, which is exactly what I want to avoid. Why? Because the function I'll use for setting that option may not be defined in libavcodec. Also I don't want to make opt.c grow and grow every time I need a specialized function for setting an option.
Ive already in the past rejected this hybrid system. You try again with the very same system ... Its poor design, first the whole need of having user supplied parsing functions is somewhat unclear to me. I mean the whole is for patches i rejected and IIRC that was not the only nor primary reason for their rejection but i might misremember ... AVOptions have a variable called type, it specifies which type the underlaying field is (float/string/color/flags/...) this variable selects which parser / set_string code is used Currently all these parsers are inside opt.c, they could be moved into opt_parser.c or we could even allow an user app or a libav* to register its own parsers for some types. But completely bypassing the existing framework, and to make that not blow up rewrite it all using a new AVOption2 filled with callbacks, is simply just a mess.
Show me how to do it without to extend opt.c and I'll be happy, just telling that such a feature isn't worth the trouble is not an option (pun intended).
see above but i still have my doubts about it being a good idea The only thing that becomes possible with it is putting the code in a different libraray or into the user app, i dont see the point of that currently [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
On date Thursday 2009-10-15 16:36:54 +0200, Michael Niedermayer encoded:
On Tue, Oct 13, 2009 at 11:38:37PM +0200, Stefano Sabatini wrote: [...] Ive already in the past rejected this hybrid system. You try again with the very same system ...
+hybryd-rewrite -> rejected -hybrid+rewrite -> rejected You rejected both approaches, now I don't know if there is another available -hybrid-rewrite approach, which allows all the features I would like.
Its poor design, first the whole need of having user supplied parsing functions is somewhat unclear to me. I mean the whole is for patches i rejected and IIRC that was not the only nor primary reason for their rejection but i might misremember ...
AVOptions have a variable called type, it specifies which type the underlaying field is (float/string/color/flags/...) this variable selects which parser / set_string code is used Currently all these parsers are inside opt.c, they could be moved into opt_parser.c or we could even allow an user app or a libav* to register its own parsers for some types.
One of the things I object to the current system is that the types are statically defined, so I wonder how would be possible to register new types at run-time.
But completely bypassing the existing framework, and to make that not blow up rewrite it all using a new AVOption2 filled with callbacks, is simply just a mess.
Show me how to do it without to extend opt.c and I'll be happy, just telling that such a feature isn't worth the trouble is not an option (pun intended).
see above but i still have my doubts about it being a good idea The only thing that becomes possible with it is putting the code in a different libraray or into the user app, i dont see the point of that currently
Again consider my example, the parsing code for a color is currently implemented in lavfi, in order to implement an OPT_TYPE_COLOR you would have to: 1) re-implement in lavc/lavu/wherever-the-opt-system-is the parsing code 2) move the color parsing code to wherever-the-opt-system-is. which are both rather poor choices. Also an application (e.g. ffmpeg) may want to use the option system already implemented for e.g. parsing the CLI options, rather than implement a new one, for example ffmpeg may have an FFmpegContext where to set all the parsed options. And in general you need for each library a different set of options, which are only useful in that library, I did the example of colors/notes which would be useful only in lavfi, for example a size/framerate option may be useful in lavf/lavc. Regards. -- FFmpeg = Fierce and Fundamentalist Mortal Plastic Elitist Gospel
On Fri, Oct 16, 2009 at 01:06:31AM +0200, Stefano Sabatini wrote:
On date Thursday 2009-10-15 16:36:54 +0200, Michael Niedermayer encoded:
On Tue, Oct 13, 2009 at 11:38:37PM +0200, Stefano Sabatini wrote: [...] Ive already in the past rejected this hybrid system. You try again with the very same system ...
+hybryd-rewrite -> rejected -hybrid+rewrite -> rejected
You rejected both approaches, now I don't know if there is another available -hybrid-rewrite approach, which allows all the features I would like.
Its poor design, first the whole need of having user supplied parsing functions is somewhat unclear to me. I mean the whole is for patches i rejected and IIRC that was not the only nor primary reason for their rejection but i might misremember ...
AVOptions have a variable called type, it specifies which type the underlaying field is (float/string/color/flags/...) this variable selects which parser / set_string code is used Currently all these parsers are inside opt.c, they could be moved into opt_parser.c or we could even allow an user app or a libav* to register its own parsers for some types.
One of the things I object to the current system is that the types are statically defined, so I wonder how would be possible to register new types at run-time.
I would prefer if all types where listed in opt.h
But completely bypassing the existing framework, and to make that not blow up rewrite it all using a new AVOption2 filled with callbacks, is simply just a mess.
Show me how to do it without to extend opt.c and I'll be happy, just telling that such a feature isn't worth the trouble is not an option (pun intended).
see above but i still have my doubts about it being a good idea The only thing that becomes possible with it is putting the code in a different libraray or into the user app, i dont see the point of that currently
Again consider my example, the parsing code for a color is currently implemented in lavfi, in order to implement an OPT_TYPE_COLOR you would have to:
1) re-implement in lavc/lavu/wherever-the-opt-system-is the parsing code
2) move the color parsing code to wherever-the-opt-system-is.
which are both rather poor choices.
please read what i wrote ill re-explain it just in case what you suggest is to rewrite the code (all your suggestions included a rewrite of the struct -> AVOption2 or whatever, there was no hybrid without rewrite) and the only thing you add is function pointers bloating the tables up and creating a huge mess bypassing the existing architecture (yeah thats why you cannot do it without a rewrite) and it also always breaks ABI&API What i suggest is that you register the set_string() for your type like a codec or demuxer is registered, its still not needed but it does everything you claim your system can and the existing can not you could have your code scattered in whichever lib and register it from there, that said, i dont see for what we would want to use this. We dont scatter codecs or pixel formats in 3 libs either. changing default_val to char* makes sense, what is with that now btw??? are you going to work on that or not? this has no relation to the rest
Also an application (e.g. ffmpeg) may want to use the option system already implemented for e.g. parsing the CLI options, rather than implement a new one, for example ffmpeg may have an FFmpegContext where to set all the parsed options.
ffmpeg could do that already if we wanted
And in general you need for each library a different set of options, which are only useful in that library, I did the example of colors/notes which would be useful only in lavfi, for example a size/framerate option may be useful in lavf/lavc.
colors are usefull for lets say subtitle fonts size/framerate is usefull for libavcodec and filters do you have another use case of this set_string() scattering? Iam not against it (with type in opt.h and clean registering) assuming there is a actual case where we want to use it [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates
On date Friday 2009-10-16 03:01:11 +0200, Michael Niedermayer encoded:
On Fri, Oct 16, 2009 at 01:06:31AM +0200, Stefano Sabatini wrote:
On date Thursday 2009-10-15 16:36:54 +0200, Michael Niedermayer encoded:
On Tue, Oct 13, 2009 at 11:38:37PM +0200, Stefano Sabatini wrote: [...] Ive already in the past rejected this hybrid system. You try again with the very same system ...
+hybryd-rewrite -> rejected -hybrid+rewrite -> rejected
You rejected both approaches, now I don't know if there is another available -hybrid-rewrite approach, which allows all the features I would like.
Its poor design, first the whole need of having user supplied parsing functions is somewhat unclear to me. I mean the whole is for patches i rejected and IIRC that was not the only nor primary reason for their rejection but i might misremember ...
AVOptions have a variable called type, it specifies which type the underlaying field is (float/string/color/flags/...) this variable selects which parser / set_string code is used Currently all these parsers are inside opt.c, they could be moved into opt_parser.c or we could even allow an user app or a libav* to register its own parsers for some types.
One of the things I object to the current system is that the types are statically defined, so I wonder how would be possible to register new types at run-time.
I would prefer if all types where listed in opt.h
But completely bypassing the existing framework, and to make that not blow up rewrite it all using a new AVOption2 filled with callbacks, is simply just a mess.
Show me how to do it without to extend opt.c and I'll be happy, just telling that such a feature isn't worth the trouble is not an option (pun intended).
see above but i still have my doubts about it being a good idea The only thing that becomes possible with it is putting the code in a different libraray or into the user app, i dont see the point of that currently
Again consider my example, the parsing code for a color is currently implemented in lavfi, in order to implement an OPT_TYPE_COLOR you would have to:
1) re-implement in lavc/lavu/wherever-the-opt-system-is the parsing code
2) move the color parsing code to wherever-the-opt-system-is.
which are both rather poor choices.
please read what i wrote ill re-explain it just in case what you suggest is to rewrite the code (all your suggestions included a rewrite of the struct -> AVOption2 or whatever, there was no hybrid without rewrite) and the only thing you add is function pointers bloating the tables up and creating a huge mess bypassing the existing architecture (yeah thats why you cannot do it without a rewrite) and it also always breaks ABI&API
What i suggest is that you register the set_string() for your type like a codec or demuxer is registered, its still not needed but it does everything you claim your system can and the existing can not you could have your code scattered in whichever lib and register it from there, that said, i dont see for what we would want to use this. We dont scatter codecs or pixel formats in 3 libs either. changing default_val to char* makes sense, what is with that now btw??? are you going to work on that or not? this has no relation to the rest
Also an application (e.g. ffmpeg) may want to use the option system already implemented for e.g. parsing the CLI options, rather than implement a new one, for example ffmpeg may have an FFmpegContext where to set all the parsed options.
ffmpeg could do that already if we wanted
And in general you need for each library a different set of options, which are only useful in that library, I did the example of colors/notes which would be useful only in lavfi, for example a size/framerate option may be useful in lavf/lavc.
colors are usefull for lets say subtitle fonts size/framerate is usefull for libavcodec and filters
do you have another use case of this set_string() scattering? Iam not against it (with type in opt.h and clean registering) assuming there is a actual case where we want to use it
Pinging this discussion as it was mentioned in another thread. Michael's solution with the current layout may sound something like this: * move libavcodec/opt.[hc] to lavcore * keep hardcoded option types, and eventually extend them (e.g. adding framesizes, framerates, colors, notes, whatever) without breaking API Note than I'm not volunteering for this, as I predict that I won't have much time to commit to FFmpeg in the next few months. Regards. -- FFmpeg = Faithless and Fundamental Most Portable Extreme Governor
participants (2)
-
michaelni＠gmx.at -
stefano.sabatini-lala＠poste.it