diff mbox series

[FFmpeg-devel,V4] dnn_backend_openvino.c: parse options in openvino backend

Message ID 1597763299-5823-1-git-send-email-yejun.guo@intel.com
State New
Headers show
Series [FFmpeg-devel,V4] dnn_backend_openvino.c: parse options in openvino backend
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Guo, Yejun Aug. 18, 2020, 3:08 p.m. UTC
Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
---
v3: use AVOption
v4: don't add new file dnn_common.h/c

 libavfilter/dnn/dnn_backend_openvino.c | 50 +++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

Comments

Alexander Strasser Aug. 19, 2020, 8:06 p.m. UTC | #1
Hi Yejun!

On 2020-08-18 23:08 +0800, Guo, Yejun wrote:
> Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> ---
> v3: use AVOption
> v4: don't add new file dnn_common.h/c
>
>  libavfilter/dnn/dnn_backend_openvino.c | 50 +++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/libavfilter/dnn/dnn_backend_openvino.c b/libavfilter/dnn/dnn_backend_openvino.c
> index d343bf2..277c313 100644
> --- a/libavfilter/dnn/dnn_backend_openvino.c
> +++ b/libavfilter/dnn/dnn_backend_openvino.c
> @@ -26,9 +26,37 @@
>  #include "dnn_backend_openvino.h"
>  #include "libavformat/avio.h"
>  #include "libavutil/avassert.h"
> +#include "libavutil/opt.h"
>  #include <c_api/ie_c_api.h>
>
> +typedef struct OVOptions{
> +    uint32_t batch_size;
> +    uint32_t req_num;
> +} OVOptions;
> +
> +typedef struct OvContext {
> +    const AVClass *class;
> +    OVOptions options;
> +} OvContext;
> +
> +#define OFFSET(x) offsetof(OvContext, x)
> +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM
> +static const AVOption dnn_ov_options[] = {
> +    { "batch",   "batch size",        OFFSET(options.batch_size), AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> +    { "nireq",   "number of request", OFFSET(options.req_num),    AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> +    { NULL },
> +};

If I'm not mistaken, you must use type int when using AV_OPT_TYPE_INT .

AFAIK we have these integer types

* AV_OPT_TYPE_INT -> int
* AV_OPT_TYPE_INT64 -> int64_t
* AV_OPT_TYPE_UINT64 -> uint64_t

and you can assume int to be at least 32 bits wide.


> +
> +static const AVClass dnn_ov_class = {
> +    .class_name = "dnn_ov",
> +    .item_name  = av_default_item_name,
> +    .option     = dnn_ov_options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +    .category   = AV_CLASS_CATEGORY_FILTER,
> +};
> +
>  typedef struct OVModel{
> +    OvContext ctx;
>      ie_core_t *core;
>      ie_network_t *network;
>      ie_executable_network_t *exe_network;
> @@ -155,6 +183,22 @@ err:
>      return DNN_ERROR;
>  }
>
> +static int dnn_parse_options(void *ctx, const char *options)
> +{
> +    AVDictionary *dict = NULL;
> +    int err = av_dict_parse_string(&dict, options, "=", "&", 0);
> +    if (err < 0) {
> +        av_dict_free(&dict);
> +        return err;
> +    }
> +
> +    av_opt_set_defaults(ctx);
> +    err = av_opt_set_dict(ctx, &dict);
> +
> +    av_dict_free(&dict);
> +    return err;
> +}
> +
>  DNNModel *ff_dnn_load_model_ov(const char *model_filename, const char *options)
>  {
>      DNNModel *model = NULL;
> @@ -171,6 +215,11 @@ DNNModel *ff_dnn_load_model_ov(const char *model_filename, const char *options)
>      if (!ov_model)
>          goto err;
>
> +    ov_model->ctx.class = &dnn_ov_class;
> +    model->options = options;
> +    if (dnn_parse_options(&ov_model->ctx, model->options) < 0)
> +        goto err;
> +
>      status = ie_core_create("", &ov_model->core);
>      if (status != OK)
>          goto err;
> @@ -186,7 +235,6 @@ DNNModel *ff_dnn_load_model_ov(const char *model_filename, const char *options)
>      model->model = (void *)ov_model;
>      model->set_input_output = &set_input_output_ov;
>      model->get_input = &get_input_ov;
> -    model->options = options;
>
>      return model;
>
> --

Sorry, if I missed it, are the values set from the options used anywhere?


  Alexander
Guo, Yejun Aug. 20, 2020, 1:10 a.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Alexander Strasser
> Sent: 2020年8月20日 4:06
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V4] dnn_backend_openvino.c: parse options
> in openvino backend
> 
> Hi Yejun!
> 
> On 2020-08-18 23:08 +0800, Guo, Yejun wrote:
> > Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> > ---
> > v3: use AVOption
> > v4: don't add new file dnn_common.h/c
> >
> >  libavfilter/dnn/dnn_backend_openvino.c | 50
> > +++++++++++++++++++++++++++++++++-
> >  1 file changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/dnn/dnn_backend_openvino.c
> > b/libavfilter/dnn/dnn_backend_openvino.c
> > index d343bf2..277c313 100644
> > --- a/libavfilter/dnn/dnn_backend_openvino.c
> > +++ b/libavfilter/dnn/dnn_backend_openvino.c
> > @@ -26,9 +26,37 @@
> >  #include "dnn_backend_openvino.h"
> >  #include "libavformat/avio.h"
> >  #include "libavutil/avassert.h"
> > +#include "libavutil/opt.h"
> >  #include <c_api/ie_c_api.h>
> >
> > +typedef struct OVOptions{
> > +    uint32_t batch_size;
> > +    uint32_t req_num;
> > +} OVOptions;
> > +
> > +typedef struct OvContext {
> > +    const AVClass *class;
> > +    OVOptions options;
> > +} OvContext;
> > +
> > +#define OFFSET(x) offsetof(OvContext, x) #define FLAGS
> > +AV_OPT_FLAG_FILTERING_PARAM static const AVOption dnn_ov_options[] =
> > +{
> > +    { "batch",   "batch size",        OFFSET(options.batch_size),
> AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> > +    { "nireq",   "number of request", OFFSET(options.req_num),
> AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> > +    { NULL },
> > +};
> 
> If I'm not mistaken, you must use type int when using AV_OPT_TYPE_INT .
> 
> AFAIK we have these integer types
> 
> * AV_OPT_TYPE_INT -> int
> * AV_OPT_TYPE_INT64 -> int64_t
> * AV_OPT_TYPE_UINT64 -> uint64_t
> 
> and you can assume int to be at least 32 bits wide.
> 

thanks, I'll change from uint32_t to int.

> > @@ -186,7 +235,6 @@ DNNModel *ff_dnn_load_model_ov(const char
> *model_filename, const char *options)
> >      model->model = (void *)ov_model;
> >      model->set_input_output = &set_input_output_ov;
> >      model->get_input = &get_input_ov;
> > -    model->options = options;
> >
> >      return model;
> >
> > --
> 
> Sorry, if I missed it, are the values set from the options used anywhere?

You are right, the options are not used. 

My teammates and I are working on the dnn backend performance
improvement, we have done locally many quick dirty code to verify our ideas and
found it requires some changes in the DNN module including these options. 
(In our quick code, we are using fixed magic number for these options)

So, as a collaboration, my idea is to separate the changes one patch by one patch, 
and we can keep rebase locally, the final purpose is to upstream all our local code with refinement.
Alexander Strasser Aug. 20, 2020, 4:57 p.m. UTC | #3
On 2020-08-20 01:10 +0000, Guo, Yejun wrote:
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Alexander Strasser
> > Sent: 2020年8月20日 4:06
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH V4] dnn_backend_openvino.c: parse options
> > in openvino backend
> >
> > On 2020-08-18 23:08 +0800, Guo, Yejun wrote:
> > > Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> > > ---
> > > v3: use AVOption
> > > v4: don't add new file dnn_common.h/c
> > >
> > >  libavfilter/dnn/dnn_backend_openvino.c | 50
> > > +++++++++++++++++++++++++++++++++-
> > >  1 file changed, 49 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavfilter/dnn/dnn_backend_openvino.c
> > > b/libavfilter/dnn/dnn_backend_openvino.c
> > > index d343bf2..277c313 100644
> > > --- a/libavfilter/dnn/dnn_backend_openvino.c
> > > +++ b/libavfilter/dnn/dnn_backend_openvino.c
> > > @@ -26,9 +26,37 @@
> > >  #include "dnn_backend_openvino.h"
> > >  #include "libavformat/avio.h"
> > >  #include "libavutil/avassert.h"
> > > +#include "libavutil/opt.h"
> > >  #include <c_api/ie_c_api.h>
> > >
> > > +typedef struct OVOptions{
> > > +    uint32_t batch_size;
> > > +    uint32_t req_num;
> > > +} OVOptions;
> > > +
> > > +typedef struct OvContext {
> > > +    const AVClass *class;
> > > +    OVOptions options;
> > > +} OvContext;
> > > +
> > > +#define OFFSET(x) offsetof(OvContext, x) #define FLAGS
> > > +AV_OPT_FLAG_FILTERING_PARAM static const AVOption dnn_ov_options[] =
> > > +{
> > > +    { "batch",   "batch size",        OFFSET(options.batch_size),
> > AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> > > +    { "nireq",   "number of request", OFFSET(options.req_num),
> > AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> > > +    { NULL },
> > > +};
> >
> > If I'm not mistaken, you must use type int when using AV_OPT_TYPE_INT .
> >
> > AFAIK we have these integer types
> >
> > * AV_OPT_TYPE_INT -> int
> > * AV_OPT_TYPE_INT64 -> int64_t
> > * AV_OPT_TYPE_UINT64 -> uint64_t
> >
> > and you can assume int to be at least 32 bits wide.
> >
>
> thanks, I'll change from uint32_t to int.

Sounds about right.

Though as I'm looking at the code again, you should correct the
allowed range as well. I assume full signed int range was never
intended, since the original type was uint32_t .


> > > @@ -186,7 +235,6 @@ DNNModel *ff_dnn_load_model_ov(const char
> > *model_filename, const char *options)
> > >      model->model = (void *)ov_model;
> > >      model->set_input_output = &set_input_output_ov;
> > >      model->get_input = &get_input_ov;
> > > -    model->options = options;
> > >
> > >      return model;
> > >
> > > --
> >
> > Sorry, if I missed it, are the values set from the options used anywhere?
>
> You are right, the options are not used.
>
> My teammates and I are working on the dnn backend performance
> improvement, we have done locally many quick dirty code to verify our ideas and
> found it requires some changes in the DNN module including these options.
> (In our quick code, we are using fixed magic number for these options)

I feel you. It can be a long path, including back tracking at some
points, to properly include some quick hacks.


> So, as a collaboration, my idea is to separate the changes one patch by one patch,
> and we can keep rebase locally, the final purpose is to upstream all our local code with refinement.

Sounds like a good idea.

Would be good if you could do it in a way that the individual commits
are mostly understandable on their own. Like here: parsing the options
but not using them somewhere looks strange.

If it's not feasibly possible, it would at least be required to mention
the planned follow-ups in the commit message. So we can make sense of
it when looking at the commit message years from now.


  Alexander
Guo, Yejun Aug. 21, 2020, 3 a.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Alexander Strasser
> Sent: 2020年8月21日 0:58
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V4] dnn_backend_openvino.c: parse options
> in openvino backend
> 
> On 2020-08-20 01:10 +0000, Guo, Yejun wrote:
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Alexander Strasser
> > > Sent: 2020年8月20日 4:06
> > > To: FFmpeg development discussions and patches
> > > <ffmpeg-devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH V4] dnn_backend_openvino.c: parse
> > > options in openvino backend
> > >
> > > On 2020-08-18 23:08 +0800, Guo, Yejun wrote:
> > > > Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> > > > ---
> > > > v3: use AVOption
> > > > v4: don't add new file dnn_common.h/c
> > > >
> > > >  libavfilter/dnn/dnn_backend_openvino.c | 50
> > > > +++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 49 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavfilter/dnn/dnn_backend_openvino.c
> > > > b/libavfilter/dnn/dnn_backend_openvino.c
> > > > index d343bf2..277c313 100644
> > > > --- a/libavfilter/dnn/dnn_backend_openvino.c
> > > > +++ b/libavfilter/dnn/dnn_backend_openvino.c
> > > > @@ -26,9 +26,37 @@
> > > >  #include "dnn_backend_openvino.h"
> > > >  #include "libavformat/avio.h"
> > > >  #include "libavutil/avassert.h"
> > > > +#include "libavutil/opt.h"
> > > >  #include <c_api/ie_c_api.h>
> > > >
> > > > +typedef struct OVOptions{
> > > > +    uint32_t batch_size;
> > > > +    uint32_t req_num;
> > > > +} OVOptions;
> > > > +
> > > > +typedef struct OvContext {
> > > > +    const AVClass *class;
> > > > +    OVOptions options;
> > > > +} OvContext;
> > > > +
> > > > +#define OFFSET(x) offsetof(OvContext, x) #define FLAGS
> > > > +AV_OPT_FLAG_FILTERING_PARAM static const AVOption
> > > > +dnn_ov_options[] = {
> > > > +    { "batch",   "batch size",        OFFSET(options.batch_size),
> > > AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> > > > +    { "nireq",   "number of request", OFFSET(options.req_num),
> > > AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
> > > > +    { NULL },
> > > > +};
> > >
> > > If I'm not mistaken, you must use type int when using AV_OPT_TYPE_INT .
> > >
> > > AFAIK we have these integer types
> > >
> > > * AV_OPT_TYPE_INT -> int
> > > * AV_OPT_TYPE_INT64 -> int64_t
> > > * AV_OPT_TYPE_UINT64 -> uint64_t
> > >
> > > and you can assume int to be at least 32 bits wide.
> > >
> >
> > thanks, I'll change from uint32_t to int.
> 
> Sounds about right.
> 
> Though as I'm looking at the code again, you should correct the allowed range as
> well. I assume full signed int range was never intended, since the original type
> was uint32_t .

thanks, the range will be [0, int_max]

> 
> 
> > > > @@ -186,7 +235,6 @@ DNNModel *ff_dnn_load_model_ov(const char
> > > *model_filename, const char *options)
> > > >      model->model = (void *)ov_model;
> > > >      model->set_input_output = &set_input_output_ov;
> > > >      model->get_input = &get_input_ov;
> > > > -    model->options = options;
> > > >
> > > >      return model;
> > > >
> > > > --
> > >
> > > Sorry, if I missed it, are the values set from the options used anywhere?
> >
> > You are right, the options are not used.
> >
> > My teammates and I are working on the dnn backend performance
> > improvement, we have done locally many quick dirty code to verify our
> > ideas and found it requires some changes in the DNN module including these
> options.
> > (In our quick code, we are using fixed magic number for these options)
> 
> I feel you. It can be a long path, including back tracking at some points, to
> properly include some quick hacks.
> 
> 
> > So, as a collaboration, my idea is to separate the changes one patch
> > by one patch, and we can keep rebase locally, the final purpose is to upstream
> all our local code with refinement.
> 
> Sounds like a good idea.
> 
> Would be good if you could do it in a way that the individual commits are mostly
> understandable on their own. Like here: parsing the options but not using them
> somewhere looks strange.

yes, good point. I'll just pending it and will send with other patch which uses the options.

> 
> If it's not feasibly possible, it would at least be required to mention the planned
> follow-ups in the commit message. So we can make sense of it when looking at
> the commit message years from now.
> 
> 
>   Alexander
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with
> subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavfilter/dnn/dnn_backend_openvino.c b/libavfilter/dnn/dnn_backend_openvino.c
index d343bf2..277c313 100644
--- a/libavfilter/dnn/dnn_backend_openvino.c
+++ b/libavfilter/dnn/dnn_backend_openvino.c
@@ -26,9 +26,37 @@ 
 #include "dnn_backend_openvino.h"
 #include "libavformat/avio.h"
 #include "libavutil/avassert.h"
+#include "libavutil/opt.h"
 #include <c_api/ie_c_api.h>
 
+typedef struct OVOptions{
+    uint32_t batch_size;
+    uint32_t req_num;
+} OVOptions;
+
+typedef struct OvContext {
+    const AVClass *class;
+    OVOptions options;
+} OvContext;
+
+#define OFFSET(x) offsetof(OvContext, x)
+#define FLAGS AV_OPT_FLAG_FILTERING_PARAM
+static const AVOption dnn_ov_options[] = {
+    { "batch",   "batch size",        OFFSET(options.batch_size), AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
+    { "nireq",   "number of request", OFFSET(options.req_num),    AV_OPT_TYPE_INT,  { .i64 = 1 }, INT_MIN, INT_MAX, FLAGS },
+    { NULL },
+};
+
+static const AVClass dnn_ov_class = {
+    .class_name = "dnn_ov",
+    .item_name  = av_default_item_name,
+    .option     = dnn_ov_options,
+    .version    = LIBAVUTIL_VERSION_INT,
+    .category   = AV_CLASS_CATEGORY_FILTER,
+};
+
 typedef struct OVModel{
+    OvContext ctx;
     ie_core_t *core;
     ie_network_t *network;
     ie_executable_network_t *exe_network;
@@ -155,6 +183,22 @@  err:
     return DNN_ERROR;
 }
 
+static int dnn_parse_options(void *ctx, const char *options)
+{
+    AVDictionary *dict = NULL;
+    int err = av_dict_parse_string(&dict, options, "=", "&", 0);
+    if (err < 0) {
+        av_dict_free(&dict);
+        return err;
+    }
+
+    av_opt_set_defaults(ctx);
+    err = av_opt_set_dict(ctx, &dict);
+
+    av_dict_free(&dict);
+    return err;
+}
+
 DNNModel *ff_dnn_load_model_ov(const char *model_filename, const char *options)
 {
     DNNModel *model = NULL;
@@ -171,6 +215,11 @@  DNNModel *ff_dnn_load_model_ov(const char *model_filename, const char *options)
     if (!ov_model)
         goto err;
 
+    ov_model->ctx.class = &dnn_ov_class;
+    model->options = options;
+    if (dnn_parse_options(&ov_model->ctx, model->options) < 0)
+        goto err;
+
     status = ie_core_create("", &ov_model->core);
     if (status != OK)
         goto err;
@@ -186,7 +235,6 @@  DNNModel *ff_dnn_load_model_ov(const char *model_filename, const char *options)
     model->model = (void *)ov_model;
     model->set_input_output = &set_input_output_ov;
     model->get_input = &get_input_ov;
-    model->options = options;
 
     return model;