> On Jan 29, 2024, at 11:01, Chen Yufei <cyfde...@gmail.com> wrote:
>
> On Sun, Jan 28, 2024 at 10:10 PM Anton Khirnov <an...@khirnov.net
> <mailto:an...@khirnov.net>> wrote:
>>
>> Quoting Zhao Zhili (2024-01-28 14:51:58)
>>>
>>>
>>>> On Jan 28, 2024, at 18:31, Anton Khirnov <an...@khirnov.net> wrote:
>>>>
>>>> Quoting Chen Yufei (2024-01-25 17:16:46)
>>>>> On Wed, Jan 24, 2024 at 7:39 PM Anton Khirnov <an...@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@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org <mailto:ffmpeg-devel-requ...@ffmpeg.org> with
> subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
