On Thu, 9 Jun 2011 16:13:07 +0200, Stefano Sabatini
<[email protected]> wrote:
> On date Thursday 2011-06-09 14:24:11 +0200, Anton Khirnov encoded:
> >
> > On Thu, 9 Jun 2011 13:30:54 +0200, Stefano Sabatini
> > <[email protected]> wrote:
> > > On date Thursday 2011-06-09 12:27:52 +0200, Anton Khirnov encoded:
> > > > This way the caller can pass all the options in one nice package.
> > > > ---
> > > > libavutil/opt.c | 24 ++++++++++++++++++++++++
> > > > libavutil/opt.h | 13 +++++++++++++
> > > > 2 files changed, 37 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/libavutil/opt.c b/libavutil/opt.c
> > > > index 172fcec..429c9ba 100644
> > > > --- a/libavutil/opt.c
> > > > +++ b/libavutil/opt.c
> > > > @@ -29,6 +29,7 @@
> > > > #include "avstring.h"
> > > > #include "opt.h"
> > > > #include "eval.h"
> > > > +#include "dict.h"
> > > >
> > > > //FIXME order them and do a bin search
> > > > const AVOption *av_find_opt(void *v, const char *name, const char
> > > > *unit, int mask, int flags)
> > > > @@ -528,6 +529,29 @@ void av_opt_free(void *obj)
> > > > av_freep((uint8_t *)obj + o->offset);
> > > > }
> > > >
> > > > +int av_opt_apply_dict(void *obj, AVDictionary **options)
> > >
> > > I suggest: av_opt_set_options_dict() (which will be consistent with:
> > > av_opt_set_options_string())
> > >
> > > longer but auto-explicative
> >
> > IMO the _options_ part is redundant. It's already prefixed with av_opt,
> > what else would it set if not options.
> > With that gone we're left with av_opt_set_dict, which might or might not
> > be a better choice, I don't have a strong opinion about that.
> >
> > Ad consistency, there is no av_opt_set_options_string. There is
> > av_set_options_string, which should be renamed.
>
> av_opt_set_dict() looks fine as well, but then I can't find a
> consistent replacement for av_set_options_string() ->
> av_opt_set_string() is already taken, maybe av_opt_set_strings() will
> do.
av_opt_parse_string/process_string/...?
> > > > + ret = 0;
> > > > + }
> > > > + av_dict_free(options);
> > > > + *options = tmp;
> > > > + return ret;
> > > > +}
> > > > +
> > > > #ifdef TEST
> > > >
> > > > #undef printf
> > > > diff --git a/libavutil/opt.h b/libavutil/opt.h
> > > > index 8c3b6c1..f16d989 100644
> > > > --- a/libavutil/opt.h
> > > > +++ b/libavutil/opt.h
> > > > @@ -29,6 +29,7 @@
> > > >
> > > > #include "rational.h"
> > > > #include "avutil.h"
> > > > +#include "dict.h"
> > > >
> > > > enum AVOptionType{
> > > > FF_OPT_TYPE_FLAGS,
> > > > @@ -181,4 +182,16 @@ int av_set_options_string(void *ctx, const char
> > > > *opts,
> > > > */
> > > > void av_opt_free(void *obj);
> > > >
> > > > +/*
> > > > + * A convenience function that applies all the options from options to
> > > > obj.
> > > > + *
> > > > + * @param obj A struct whose first element is a pointer to AVClass.
> > > > + * @param options Options to process. Will be replaced with an
> > > > AVDictionary
> > > > + * containing all options not found in obj.
> > >
> > > Would it make sense to keep not found options as a separate param? I
> > > mean in the case you want to keep the passed dictionary (seems safer
> > > to me, less side-effects). That would be:
> > >
> > > av_opt_...(void *obj, AVDictionary *options, AVDictionary
> > > **not_found_options, int flags?);
> > >
> > > In case not_found_options is NULL then the dictionary is not filled.
> >
> >
> > Again, i don't see the advantage in this. If the caller wants to
> > preserve the dict, he can do it with a single av_dict_copy. Adding a new
> > parameter seems messy to me.
>
> Re-using options is prone to leaks if you don't remember to explicitely
> free it, having:
>
> int av_opt_set_dict(void *obj, const AVDictionary *options, AVDictionary
> **not_found_options);
>
> is more explicit and you don't have to worry about the resulting
> not-found options dictionary if you don't care about it (just set
> not_found_options to NULL). The only problem is that you need to
> explicitely free options, but this looks like a feature to me, the
> default assumption is that a function won't destroy a param which is
> passed to it (safer, avoids side-effects).
I don't think it's more explicit. With my API, the caller always manages
the dicts he allocated himself. With what you're suggesting, lavu allocates
a dict for him and he has to remember to free it.
+it's yet another parameter creep.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
