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.
> > > + AVDictionaryEntry *t = NULL;
> > > + AVDictionary *tmp = NULL;
> > > + int ret = 0;
> > > +
> > > + av_dict_copy(&tmp, *options, 0);
> > > +
> > > + while ((t = av_dict_get(*options, "", t, AV_DICT_IGNORE_SUFFIX))) {
> > > + ret = av_set_string3(obj, t->key, t->value, 1, NULL);
> > > + if (ret >= 0)
> > > + av_dict_set(&tmp, t->key, NULL, 0);
> >
> > > + else if (ret != AVERROR_OPTION_NOT_FOUND) {
> > > + av_log(obj, AV_LOG_ERROR, "Error setting option %s to value
> > > %s\n", t->key, t->value);
> > > + break;
> > > + }
> >
> > Maybe this could be handled through a flag
> > AV_OPT_FLAG_FAIL_IF_NOT_FOUND, in case you want to fail in case an
> > option is not found (alternatively you can simply check on the
> > dictionary filled with the not found options, but this looks more
> > extensible).
> >
>
> I don't really see how this is better. I'd prefer not to bloat the API
> with more pointless parameters, when the exact same thing can be
> accomplished without it.
I see your point.
> > > + 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).
--
All possibility of understanding is rooted in the ability to say no.
-- Susan Sontag
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
