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

[Anton Khirnov]

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?

> Pass as file path works for me for now since it’s not the first case. We can replace
> it if anyone has a better idea.

Every new patch like this is just more work somebody has to undo in the
future. Given past experience, I'd MUCH prefer to do it properly in the
first place.

-- 
Anton Khirnov