| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
> -----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.
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
> -----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 --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;
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(-)