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

[Zhao Zhili]

> On Jan 29, 2024, at 11:01, Chen Yufei <cyfdecyf at gmail.com> wrote:
> 
> On Sun, Jan 28, 2024 at 10:10 PM Anton Khirnov <anton at khirnov.net <mailto: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.
> 
>>> 
>>> 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.

Check read_binary() in ffmpeg_filter.c for loading data from file and setting AV_OPT_TYPE_BINARY
option value.

Check pix_fmts option in buffersink.c for the consumer side of AV_OPT_TYPE_BINARY.

static const AVOption buffersink_options[] = {
    { "pix_fmts", "set the supported pixel formats", OFFSET(pixel_fmts), AV_OPT_TYPE_BINARY, .flags = FLAGS },

> 
>>> 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 at ffmpeg.org <mailto: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 <mailto:ffmpeg-devel-request at ffmpeg.org> with subject "unsubscribe".