[FFmpeg-devel] [PATCH 3/4] avfilter/vf_dnn_processing: add format GRAY8 and GRAYF32 support
Guo, Yejun
yejun.guo at intel.com
Mon Dec 16 14:39:02 EET 2019
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> mypopy at gmail.com
> Sent: Monday, December 16, 2019 7:43 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: Pedro Arthur <bygrandao at gmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH 3/4] avfilter/vf_dnn_processing: add
> format GRAY8 and GRAYF32 support
>
> On Mon, Dec 16, 2019 at 7:18 PM Guo, Yejun <yejun.guo at intel.com> wrote:
> >
> >
> >
> > > -----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).
> >
> >
>
> Based on the flexibility, I prefer option 1
thanks for the discussion, I might not explain the background well.
For a given dnn model, the fmt is determined. 'fmt' has two meanings, the format of the Frame and also denotes the input format of dnn model.
I think the advantage of option 1 is simpler, but the disadvantage of option 1 is no flexible since
we need to change parameters of two filters when a new model with different input is used.
> _______________________________________________
> 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".
More information about the ffmpeg-devel
mailing list