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

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Alexander Strasser
> Sent: 2020年8月21日 0:58
> 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-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 .

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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request at ffmpeg.org with
> subject "unsubscribe".