[FFmpeg-devel] [PATCH v2 1/1] avfilter/vf_vpp_qsv: apply 3D LUT from file.

[Zhao Zhili]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton Khirnov
> Sent: 2024年1月29日 0:52
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: Haihao Xiang <haihao.xiang at intel.com>; Chen Yufei <cyfdecyf at gmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/1] avfilter/vf_vpp_qsv: apply 3D LUT from file.
> 
> Quoting Zhao Zhili (2024-01-28 16:46:55)
> >
> >
> > > On Jan 28, 2024, at 22:10, Anton Khirnov <anton at khirnov.net> wrote:
> > >
> > > Quoting Zhao Zhili (2024-01-28 14:51:58)
> > >>
> > >>
> > >>> On Jan 28, 2024, at 18:31, Anton Khirnov <anton at khirnov.net> wrote:
> > >>>
> > >>> Quoting Chen Yufei (2024-01-25 17:16:46)
> > >>>> On Wed, Jan 24, 2024 at 7:39 PM Anton Khirnov <anton at khirnov.net> wrote:
> > >>>>>
> > >>>>> Quoting Chen Yufei (2024-01-20 16:14:29)
> > >>>>>> Usage: "vpp_qsv=lut3d_file=<path to file>"
> > >>>>>
> > >>>>> Passing file paths to a filter and having the filter load the file is
> > >>>>> not recommended, it is generally preferable to have an
> > >>>>> AV_OPT_TYPE_BINARY option, with IO performed by the caller.
> > >>>>>
> > >>>>> E.g. in ffmpeg CLI you can do
> > >>>>> -filter vpp_qsv=/lut3d=<file>
> > >>>>> to load the option value from a file.
> > >>>>
> > >>>> I searched for code using `AV_OPT_TYPE_BINARY`.
> > >>>> `vf_libplacebo.c` gives me a good example.
> > >>>>
> > >>>> The LUT parsing code is took from `libavfilter/vf_lut3d.c`.
> > >>>> It's mainly text processing which calls functions on `FILE*`.
> > >>>> Using `AV_OPT_TYPE_BINARY` would require many changes in LUT paring
> > >>>> code, and also need to change the command line option of `vf_lut3d`.
> > >>>> So I'd keep the lut file option as is.
> > >>>
> > >>> Your patch is rejected then.
> > >>
> > >> Could you be more elaborate?
> > >
> > > I said in my previous email what I believe should be done instead. I'm
> > > open to reasonable arguments about that of course, but "I am going to
> > > ignore your review" is not a good way to get patches in.
> > >
> > >> I agree pass LUT as file path isn’t flexible and clean, but it’s more questionable to
> > >> use AV_OPT_TYPE_BINARY for data which could be a large chunk.
> > >
> > > The LUT is loaded into memory in any case, is it not?
> >
> > The binary to string in hex format to binary is like transport image in base64. It doesn’t
> > sound right to use base64 for video streams.
> 
> I don't quite see the connection. Where is anything converted to a hex
> string?

When the option value is set via av_opt_set instead of av_opt_set_bin. av_opt_set_bin can
avoid such inefficiency, but it still do memcpy.

> 
> --
> Anton Khirnov
> _______________________________________________
> 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".