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.
>
> > +{
> > + 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.
> > + 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.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
