[FFmpeg-devel] [PATCH 3/4] avfilter/vf_dnn_processing: add format GRAY8 and GRAYF32 support
Pedro Arthur
bygrandao at gmail.com
Mon Dec 16 16:14:21 EET 2019
Em seg., 16 de dez. de 2019 às 09:39, Guo, Yejun <yejun.guo at intel.com> escreveu:
>
>
>
> > -----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'm not 100% sure about what you mean, but if the model determines the
expected format, it should be present in the model file and require no
manual setting by the user, at least for our own model file format.
>
> 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.
I agree we have one more filter with option 1 but it seems a more
standard (and direct) way of setting formats. If it can achieve the
same features as option 2 I would still prefer option 1.
If you have a specific use case which would be penalized by using
option 1 let us know.
>
> > _______________________________________________
> > 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