[FFmpeg-devel] [PATCH 2/9] lavu/opt: introduce av_opt_serialize()

Stefano Sabatini stefasab at gmail.com
Tue Nov 18 17:06:26 CET 2014


On date Wednesday 2014-11-12 22:32:04 +0100, Lukasz Marek encoded:
> On 12.11.2014 17:45, Stefano Sabatini wrote:
> >On date Tuesday 2014-11-11 21:24:45 +0100, Lukasz Marek encoded:
> >>On 11.11.2014 17:19, Stefano Sabatini wrote:
> >>>We have already av_get_opt() which serializes the value. Also we should
> >>>probably escape the values.
> >>
> >>I saw that function, but don't remember why I didn't use. I was
> >>wrong obviously.
> >>BTW, I found few bugs in it, sent separate patch for it.
> >
> >>Updated patch is attached. It is without escaping. I left it for
> >>separate patch.
> >
> >Not sure this is a good idea. Indeed the API allows to specify several
> >key/value and option separators, so this may be a limitation.
> 
> I don't want to say it is not needed or so, but it can be done in
> separate patch, thats all.

Yes but it would break public API.
> 
> 
> >>+int av_opt_serialize(void *obj, int opt_flags, int skip_defaults, char **buffer,
> >>+                     const char key_val_sep, const char pairs_sep)
> >
> >skip_defaults could be a flag, in case we want to extend it further
> >(for example suppose we only want to print long or short option
> >names).
> 
> Yes, this is good and I changed it.
> 
> I don't know if we should also add a parameter for choosing
> >the escaping algorithm, probably not.
> 
> I don't see any reason for it. If any in future, it can still be
> forced by flag.
> 
> >>+{
> >>+    const AVOption *o = NULL;
> >>+    uint8_t *buf;
> >>+    AVBPrint bprint;
> >>+    int ret, cnt = 0;
> >>+
> >>+    if (!obj || !buffer)
> >>+        return AVERROR(EINVAL);
> >>+
> >>+    *buffer = NULL;
> >>+    av_bprint_init(&bprint, 64, AV_BPRINT_SIZE_UNLIMITED);
> >>+
> >>+    while (o = av_opt_next(obj, o)) {
> >>+        if (o->flags & opt_flags != opt_flags || o->type == AV_OPT_TYPE_CONST)
> >>+            continue;
> >>+        if (skip_defaults && av_opt_is_set_to_default(obj, o) > 0)
> >>+            continue;
> >
> >>+        if ((ret = av_opt_get(obj, o->name, 0, &buf)) < 0) {
> >
> >Here there is a potential problem. At the moment there is no way to
> >distinguish between a string set to NULL and a string set to "", since
> >av_opt_get() will return "" for a string set to NULL. For some
> >configurable elements this will make a difference. The only solution
> >in this case is to use "skip defaults". Also in general the user won't
> >be able to set an option to NULL if not using the binary interface.
> 
> I know it cannot be distinguish now. It is possible to serialize
> null as \0 for example (where \ is escaping char), but such string
> have to be unescaped before passing to set_from_string function, or
> such function support escaping itself.
> 
> 
> >>+            av_bprint_finalize(&bprint, NULL);
> >>+            return ret;
> >>+        }
> >
> >This will print alias options as well. This was my solution:
> 

> I'm not sure it is always safe. Options with the same offset may
> have different opt_flags and different defaults.

I think this would be a bug, since for example set_defaults with set
only the value of the last specified option. Do you have examples for
this?

> user may want to serialize object if opt_flags = 0, but apply to
> object with specific mask.


> This can be controlled by a flag I think.
> 
> Updated doxy, I hope it is better.

> From 715f2ef32c85112a4d7dced62afcb6d89274a927 Mon Sep 17 00:00:00 2001
> From: Lukasz Marek <lukasz.m.luki2 at gmail.com>
> Date: Mon, 10 Nov 2014 22:28:44 +0100
> Subject: [PATCH] lavu/opt: introduce av_opt_serialize()
> 
> TODO: bump minor version, update doc/APIchanges
> 
> Function allows to create string containing object's serialized options.
> Such string may be passed back to av_set_options_string() in order to restore options.
> 
> Signed-off-by: Lukasz Marek <lukasz.m.luki2 at gmail.com>
> ---
>  libavutil/opt.c    | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/opt.h    | 20 +++++++++++++++++
>  tests/ref/fate/opt |  8 +++++++
>  3 files changed, 92 insertions(+)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 1381cc9..f0bb3cf 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -37,6 +37,7 @@
>  #include "pixdesc.h"
>  #include "mathematics.h"
>  #include "samplefmt.h"
> +#include "bprint.h"
>  
>  #include <float.h>
>  
> @@ -1835,6 +1836,40 @@ int av_opt_is_set_to_default_by_name(void *obj, const char *name, int search_fla
>      return av_opt_is_set_to_default(target, o);
>  }
>  
> +int av_opt_serialize(void *obj, int opt_flags, int flags, char **buffer,
> +                     const char key_val_sep, const char pairs_sep)
> +{
> +    const AVOption *o = NULL;
> +    uint8_t *buf;
> +    AVBPrint bprint;
> +    int ret, cnt = 0;
> +
> +    if (!obj || !buffer)
> +        return AVERROR(EINVAL);
> +
> +    *buffer = NULL;
> +    av_bprint_init(&bprint, 64, AV_BPRINT_SIZE_UNLIMITED);
> +
> +    while (o = av_opt_next(obj, o)) {
> +        if (o->flags & opt_flags != opt_flags || o->type == AV_OPT_TYPE_CONST)
> +            continue;
> +        if (flags & AV_OPT_SERIALIZE_SKIP_DEFAULTS && av_opt_is_set_to_default(obj, o) > 0)
> +            continue;
> +        if ((ret = av_opt_get(obj, o->name, 0, &buf)) < 0) {
> +            av_bprint_finalize(&bprint, NULL);
> +            return ret;
> +        }
> +        if (buf) {
> +            if (cnt++)
> +                av_bprint_append_data(&bprint, &pairs_sep, 1);
> +            av_bprintf(&bprint, "%s%c%s", o->name, key_val_sep, buf);
> +            av_freep(&buf);
> +        }
> +    }
> +    av_bprint_finalize(&bprint, buffer);
> +    return 0;
> +}
> +

Not sure we should skip escaping here...

>  #ifdef TEST
>  
>  typedef struct TestContext
> @@ -1854,6 +1889,10 @@ typedef struct TestContext
>      int64_t channel_layout;
>      void *binary;
>      int binary_size;
> +    void *binary1;
> +    int binary_size1;
> +    void *binary2;
> +    int binary_size2;
>      int64_t num64;
>      float flt;
>      double dbl;
> @@ -1882,6 +1921,8 @@ static const AVOption test_options[]= {
>  {"color", "set color",   OFFSET(color), AV_OPT_TYPE_COLOR, {.str = "pink"}, 0, 0},
>  {"cl", "set channel layout", OFFSET(channel_layout), AV_OPT_TYPE_CHANNEL_LAYOUT, {.i64 = AV_CH_LAYOUT_HEXAGONAL}, 0, INT64_MAX},
>  {"bin", "set binary value",    OFFSET(binary),   AV_OPT_TYPE_BINARY,   {.str="62696e00"}, 0,        0 },
> +{"bin1", "set binary value",   OFFSET(binary1),  AV_OPT_TYPE_BINARY,   {.str=NULL},       0,        0 },
> +{"bin2", "set binary value",   OFFSET(binary2),  AV_OPT_TYPE_BINARY,   {.str=""},         0,        0 },
>  {"num64",    "set num 64bit",  OFFSET(num64),    AV_OPT_TYPE_INT64,    {.i64 = 1},        0,        100 },
>  {"flt",      "set float",      OFFSET(flt),      AV_OPT_TYPE_FLOAT,    {.dbl = 1.0/3},    0,        100 },
>  {"dbl",      "set double",     OFFSET(dbl),      AV_OPT_TYPE_DOUBLE,   {.dbl = 1.0/3},    0,        100 },
> @@ -1949,6 +1990,29 @@ int main(void)
>          }
>      }
>  
> +    printf("\nTest av_opt_serialize()\n");
> +    {
> +        TestContext test_ctx = { 0 };
> +        char *buf;
> +        test_ctx.class = &test_class;
> +
> +        av_log_set_level(AV_LOG_QUIET);
> +
> +        av_opt_set_defaults(&test_ctx);
> +        if (av_opt_serialize(&test_ctx, 0, 0, &buf, '=', ',') >= 0) {
> +            printf("%s\n", buf);
> +            av_opt_free(&test_ctx);
> +            memset(&test_ctx, 0, sizeof(test_ctx));
> +            test_ctx.class = &test_class;
> +            av_set_options_string(&test_ctx, buf, "=", ",");
> +            av_free(buf);
> +            if (av_opt_serialize(&test_ctx, 0, 0, &buf, '=', ',') >= 0) {
> +                printf("%s\n", buf);
> +                av_free(buf);
> +            }
> +        }
> +    }
> +
>      printf("\nTesting av_set_options_string()\n");
>      {
>          TestContext test_ctx = { 0 };
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 5158067..5fa7706 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -869,6 +869,26 @@ int av_opt_is_set_to_default(void *obj, const AVOption *o);
>   */
>  int av_opt_is_set_to_default_by_name(void *obj, const char *name, int search_flags);
>  
> +
> +#define AV_OPT_SERIALIZE_SKIP_DEFAULTS 0x00000001  ///< Serialize options that are not set to default values only.
> +
> +/**
> + * Serialize object's options.
> + *
> + * Create a string containing object's serialized options.
> + * Such string may be passed back to av_opt_set_from_string() in order to restore option values.
> + *
> + * @param[in]  obj           AVClass object to serialize
> + * @param[in]  opt_flags     serialize options with all the specified flags set (AV_OPT_FLAG)
> + * @param[in]  flags         combination of AV_OPT_SERIALIZE_* flags

> + * @param[out] buffer        Pointer to buffer that will be allocated with string containg serialized options.
> + *                           Buffer must be freed by the caller when is no longer needed.

nit: pointer

> + * @param[in]  key_val_sep   character used to separate key from value
> + * @param[in]  pairs_sep     character used to separate two pairs from each other
> + * @return                   >= 0 on success, negative on error
> + */
> +int av_opt_serialize(void *obj, int opt_flags, int flags, char **buffer,
> +                     const char key_val_sep, const char pairs_sep);
>  /**
>   * @}
>   */
> diff --git a/tests/ref/fate/opt b/tests/ref/fate/opt
> index 7953ce8..16f3387 100644
> --- a/tests/ref/fate/opt
> +++ b/tests/ref/fate/opt
> @@ -34,6 +34,8 @@ name:  duration default:0 error:
>  name:     color default:0 error:
>  name:        cl default:0 error:
>  name:       bin default:0 error:
> +name:      bin1 default:1 error:
> +name:      bin2 default:1 error:
>  name:     num64 default:0 error:
>  name:       flt default:0 error:
>  name:       dbl default:0 error:
> @@ -53,10 +55,16 @@ name:  duration default:1 error:
>  name:     color default:1 error:
>  name:        cl default:1 error:
>  name:       bin default:1 error:
> +name:      bin1 default:1 error:
> +name:      bin2 default:1 error:
>  name:     num64 default:1 error:
>  name:       flt default:1 error:
>  name:       dbl default:1 error:
>  
> +Test av_opt_serialize()
> +num=0,toggle=1,rational=1/1,string=default,flags=0x00000001,size=200x300,pix_fmt=0bgr,sample_fmt=s16,video_rate=25/1,duration=0:00:00.001000,color=0xffc0cbff,cl=0x137,bin=62696E00,bin1=,bin2=,num64=1,flt=0.333333,dbl=0.333333
> +num=0,toggle=1,rational=1/1,string=default,flags=0x00000001,size=200x300,pix_fmt=0bgr,sample_fmt=s16,video_rate=25/1,duration=0:00:00.001000,color=0xffc0cbff,cl=0x137,bin=62696E00,bin1=,bin2=,num64=1,flt=0.333333,dbl=0.333333

LGTM otherwise.
-- 
FFmpeg = Faithless and Formidable Multimedia Power Elected Game


More information about the ffmpeg-devel mailing list