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

[Chen Yufei <cyfdecyf@gmail.com>]

On Sun, Jan 28, 2024 at 10:10 PM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Zhao Zhili (2024-01-28 14:51:58)
> >
> >
> > > On Jan 28, 2024, at 18:31, Anton Khirnov <anton@khirnov.net> wrote:
> > >
> > > Quoting Chen Yufei (2024-01-25 17:16:46)
> > >> On Wed, Jan 24, 2024 at 7:39 PM Anton Khirnov <anton@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.

> >
> > 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.
>

The review is not ignored.

I've taken time to search for how to use `AV_OPT_TYPE_BINARY` and did
not think it's
appropriate to use in this case.

If someone can show me code in FFmpeg that parses loaded data as text,
I can follow
that and send a new patch. This is to avoid doing things the wrong
way, as I'm a newcomer
and don't know if there's existing code to do this.

> > 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
--
Best regards,
Chen Yufei
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".