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

[Chen Yufei]

On Tue, Jan 30, 2024 at 10:59 AM Chen Yufei <cyfdecyf at gmail.com> wrote:
>
> On Tue, Jan 30, 2024 at 1:07 AM Anton Khirnov <anton at khirnov.net> wrote:
> >
> > Quoting Chen Yufei (2024-01-29 04:01:51)
> > > On Sun, Jan 28, 2024 at 10:10 PM 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.
> > >
> > > "is not recommended, it is generally preferable"
> > > I take this as using `AV_OPT_TYPE_BINARY` is not a MUST.
> > >
> > > I'm not an English native speaker, correct me If I'm wrong.
> > >
> > > > > >>>
> > > > > >>> 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.
> > >
> > > Now I understand using `AV_OPT_TYPE_BINARY` is a must.
> >
> > Okay, sorry for being unclear. When I say 'recommended', it means it
> > must be done this way unless there is a strong technical reason to do it
> > otherwise. But then please provide a detailed explanation, not
> > something vague like "didn't seem appropriate".
>
> After taking more time looking into how to use `AV_OPT_TYPE_BINARY`,
> I'll give more details.
>
> 1. For LUT file parsing, I'm not writing new code, it's taken from
> existing vf_lut3d.c.
>     I avoid making unnecessary modifications to avoid breaking things.
>
> 2. The original code relies on file name to detect LUT file type, uses
> `fgets` and the like.
>     From my understanding, `AV_OPT_TYPE_BINARY` does not give the file
> name to the calling filter.
>     If we use `AV_OPT_TYPE_BINARY`, then how to do file type detection?
>     Adding other cmdline option would require changing vf_lut3d's
> cmdline option.

I've spent some time this weekend trying to use `AV_OPT_TYPE_BINARY`.
Got an idea from vf_libplacebo.c.

It's possible to still use `AV_OPT_TYPE_STRING` in vf_lut3d.c, use
`av_file_map` to load text into a buffer, so that all the LUT parsing
functions can work on buffers instead of `FILE*`. But this is still
reading file in filter code.

It's more work than I expected and I don't know if this change will
meet the standards to be accepted.

I'll stop here.

If someone's interested in this feature, feel free to use my patch
https://github.com/cyfdecyf/FFmpeg/tree/vpp_qsv_lut_sysmem

>
> 3. I tried to find basic text processing utils in FFmpeg to avoid
> writing ad-hoc text processing code in lut3d.c
>     I've found `FFTextReader` in libavformat/subtitles.h, but I don't
> think I should use it in filter code.
>     I'm not sure if it's OK to follow the code in `FFTextReader` and
> duplicate read line logic in lut3d.c.
>
> Besides, there's `BINARY` in the name of `AV_OPT_TYPE_BINARY`, but
> what I'm specifying here is text file.
> This is not a big problem, but seem like an option name like
> `AV_OPT_TYPE_TEXT` can express the usage of the option more clear in
> code.
>

-- 
Best regards,
Chen Yufei