[FFmpeg-devel] [PATCH 3/4] avfilter/vf_dnn_processing: add format GRAY8 and GRAYF32 support

[Guo, Yejun]

> -----Original Message-----
> From: Pedro Arthur [mailto:bygrandao at gmail.com]
> Sent: Friday, December 13, 2019 10:40 PM
> To: Guo, Yejun <yejun.guo at intel.com>
> Cc: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 3/4] avfilter/vf_dnn_processing: add
> format GRAY8 and GRAYF32 support
> 
> Em sex., 13 de dez. de 2019 às 08:23, Guo, Yejun <yejun.guo at intel.com>
> escreveu:
> >
> > > From: Pedro Arthur [mailto:bygrandao at gmail.com]
> > > Sent: Friday, December 13, 2019 12:45 AM
> > > To: FFmpeg development discussions and patches
> <ffmpeg-devel at ffmpeg.org>
> > > Cc: Guo, Yejun <yejun.guo at intel.com>
> > > Subject: Re: [FFmpeg-devel] [PATCH 3/4] avfilter/vf_dnn_processing: add
> > > format GRAY8 and GRAYF32 support
> > > Hi,
> > >
> > > how should I test this patch?
> >
> > the fourth patch of this patch set is the fate test for this feature, so I ignored
> comments here.
> > I'll add the test descriptions back in v2.
> >
> > >
> > > Em sex., 22 de nov. de 2019 às 04:57, Guo, Yejun <yejun.guo at intel.com>
> > > escreveu:
> > >
> > > > Signed-off-by: Guo, Yejun <yejun.guo at intel.com>
> > > > ---
> > > >  doc/filters.texi                |   8 ++-
> > > >  libavfilter/vf_dnn_processing.c | 147
> > > > ++++++++++++++++++++++++++++++----------
> > > >  2 files changed, 118 insertions(+), 37 deletions(-)
> > > >
> > > > diff --git a/doc/filters.texi b/doc/filters.texi
> > > > index 1f86ae1..c3f7997 100644
> > > > --- a/doc/filters.texi
> > > > +++ b/doc/filters.texi
> > > > @@ -8992,7 +8992,13 @@ Set the input name of the dnn network.
> > > >  Set the output name of the dnn network.
> > > >
> > > >  @item fmt
> > > > -Set the pixel format for the Frame. Allowed values are
> > > > @code{AV_PIX_FMT_RGB24}, and @code{AV_PIX_FMT_BGR24}.
> > > > +Set the pixel format for the Frame, the value is determined by the input
> > > > of the dnn network model.
> > > >
> > > This sentence is a bit confusing, also I think this property should be
> > > removed. (I will explain bellow).
> >
> > sure, no problem.
> >
> > >
> > > +
> > > > +If the model handles RGB (or BGR) image and the data type of model
> input
> > > > is uint8, fmt must be @code{AV_PIX_FMT_RGB24} (or
> > > @code{AV_PIX_FMT_BGR24}.
> > > > +If the model handles RGB (or BGR) image and the data type of model
> input
> > > > is float, fmt must be @code{AV_PIX_FMT_RGB24} (or
> > > @code{AV_PIX_FMT_BGR24},
> > > > and this filter will do data type conversion internally.
> > > > +If the model handles GRAY image and the data type of model input is
> > > > uint8, fmt must be @code{AV_PIX_FMT_GRAY8}.
> > > > +If the model handles GRAY image and the data type of model input is
> > > > float, fmt must be @code{AV_PIX_FMT_GRAYF32}.
> > > > +
> > > >  Default value is @code{AV_PIX_FMT_RGB24}.
> > > >
> > > >  @end table
> > > > diff --git a/libavfilter/vf_dnn_processing.c
> > > > b/libavfilter/vf_dnn_processing.c
> > > > index ce976ec..963dd5e 100644
> > > > --- a/libavfilter/vf_dnn_processing.c
> > > > +++ b/libavfilter/vf_dnn_processing.c
> > > > @@ -70,10 +70,12 @@ static av_cold int init(AVFilterContext *context)
> > > >  {
> > > >      DnnProcessingContext *ctx = context->priv;
> > > >      int supported = 0;
> > > > -    // as the first step, only rgb24 and bgr24 are supported
> > > > +    // to support more formats
> > > >      const enum AVPixelFormat supported_pixel_fmts[] = {
> > > >          AV_PIX_FMT_RGB24,
> > > >          AV_PIX_FMT_BGR24,
> > > > +        AV_PIX_FMT_GRAY8,
> > > > +        AV_PIX_FMT_GRAYF32,
> > > >      };
> > > >      for (int i = 0; i < sizeof(supported_pixel_fmts) / sizeof(enum
> > > > AVPixelFormat); ++i) {
> > > >          if (supported_pixel_fmts[i] == ctx->fmt) {
> > > > @@ -156,14 +158,38 @@ static int config_input(AVFilterLink *inlink)
> > > >          return AVERROR(EIO);
> > > >      }
> > > >
> > > I think the filter should not check formats manually in the init function
> > > (unless I'm missing something), it would be best if you query for all the
> > > above supported formats in query_formats and later in config_input you
> make
> > > sure the expected model format matches the frame format.
> >
> > I'm afraid it is too late if we find the format mismatch in function
> config_input.
> >
> > For example, the dnn module is designed to accept BGR24 data, and the
> actual
> > format comes into config_input is RGB24 or YUV420P (we'll add yuv formats
> later in
> > supported pixel fmts) or something else such as GRAY8. We have two choices:
> >
> > - return error, and the application ends.
> >   This is not what we want.
> > - return no_error, and do format conversion at the beginning of function
> filter_frame.
> >   It makes this filter complex, and our implementation for the conversion
> might not be the best optimized.
> >   My idea is to keep this filter simple. And the users can choose what they
> want to do conversion in the filters graph.
> >
> Indeed if the filter receives the format already converted is much
> better, but if you already have to specify the format the model
> expects as a parameter one could simply use the -pix_fmt to set the
> format instead of having one more filter parameter.
> Is there any downside if it uses "-pix_fmt" that "fmt" avoids?

I see, your meaning is to add a format conversion filter explicitly before dnn_processing.

so, there are two options.
1). no parameter 'fmt' for dnn_processing
We must specify the format filter (or other filters converting format) explicitly, the command line looks like:
-vf "filter0, filter1, ..., format=pix_fmts=rgb24, dnn_processing=model=my_model_file, ..."

The advantage is dnn_processing is simpler without 'fmt'.
The disadvantage is we need to change parameters of two filters (format and dnn_processing) when a new model with different input is used.


2). dnn_processing has parameter 'fmt'.
This option supports two styles of command line:
a) -vf "filter0, filter1, ..., dnn_processing=model=my_model_file:fmt=rgb24, ..."
The filter 'format=pix_fmts=rgb24' is automatically inserted by ffmpeg, so we just need to change the parameters
of one filter (dnn_processing) when a new model with different input is used.

b) -vf "filter0, filter1, ..., format=pix_fmts=rgb24, dnn_processing=model=my_model_file:fmt=rgb24, ..."
It is redundant for the rgb24.


Another point is that logically the format is tied closely with the input of the model, I think
it's more nature to put them together in one filter, so I prefer option 2). Anyway, each option has
advantages and disadvantages, I do not find an ideal one and not insist on option 2).


> 
> > Regarding the below description in doc/filters.texi, the reason is that we do
> not have format such as rgbf32 and bgrf32, we have to do conversion within
> this filter.
> > "If the model handles RGB (or BGR) image and the data type of model input is
> float, fmt must be @code{AV_PIX_FMT_RGB24} (or @code{AV_PIX
> > _FMT_BGR24}, and this filter will do data type conversion internally."
> >
> >
> > Consider the filter vf_dnn_analytic in my plan, I think the conversion is
> necessary since mostly a scale down is needed.
> >