> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of > myp...@gmail.com > Sent: Monday, December 16, 2019 7:43 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Cc: Pedro Arthur <bygran...@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....@intel.com> wrote: > > > > > > > > > -----Original Message----- > > > From: Pedro Arthur [mailto:bygran...@gmail.com] > > > Sent: Friday, December 13, 2019 10:40 PM > > > To: Guo, Yejun <yejun....@intel.com> > > > Cc: ffmpeg-devel@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....@intel.com> > > > escreveu: > > > > > > > > > From: Pedro Arthur [mailto:bygran...@gmail.com] > > > > > Sent: Friday, December 13, 2019 12:45 AM > > > > > To: FFmpeg development discussions and patches > > > <ffmpeg-devel@ffmpeg.org> > > > > > Cc: Guo, Yejun <yejun....@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
