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

[Alexander Strasser]

On 2020-08-20 01:10 +0000, Guo, Yejun wrote:
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Alexander Strasser
> > Sent: 2020年8月20日 4:06
> > To: FFmpeg development discussions and patches <ffmpeg-devel at 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 at 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