| Message ID | 20220304150307.61769-1-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] lavu: add syntax for loading AV_OPT_TYPE_BINARY from files | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
Quoting Niklas Haas (2022-03-04 16:03:06) > From: Niklas Haas <git@haasn.dev> > > I arbitrarily decided to use the syntax 'opt=@filename' to match e.g. > `curl -Ffield=@filename`, and also because @ is not a valid hex > character, nor does it conflict with any other common shell or ffmpeg > syntax. > > This is arguably somewhat clunky because it does not round-trip with > av_opt_get - you will get back a hex interpretation of the loaded file, > rather than the filename it was loaded from. It also implies a (perhaps > unnecessary) memcpy from mapped file memory into a allocated memory. > This is unfortunately necessary because there's no way for us to know > whether av_free or av_file_unmap is needed to clean up previous option > values. > > The motivating use case was the introduction of several new binary > options for vf_libplacebo, but other filters that currently rely on > manual file-path loading could benefit from it as well. Sorry, I think having an arbitrary file loader in the options parser will be an endless security nightmare. The alternative I had in mind was having ffmpeg.c itself do the file loading. This will be require some modification of the options parsing code in cmdutils.c and also extending the mechanisms we use to pass options to filters. I can try to make a POC in a few days.
On Tue, 08 Mar 2022 12:48:35 +0100 Anton Khirnov <anton@khirnov.net> wrote: > Quoting Niklas Haas (2022-03-04 16:03:06) > > From: Niklas Haas <git@haasn.dev> > > > > I arbitrarily decided to use the syntax 'opt=@filename' to match e.g. > > `curl -Ffield=@filename`, and also because @ is not a valid hex > > character, nor does it conflict with any other common shell or ffmpeg > > syntax. > > > > This is arguably somewhat clunky because it does not round-trip with > > av_opt_get - you will get back a hex interpretation of the loaded file, > > rather than the filename it was loaded from. It also implies a (perhaps > > unnecessary) memcpy from mapped file memory into a allocated memory. > > This is unfortunately necessary because there's no way for us to know > > whether av_free or av_file_unmap is needed to clean up previous option > > values. > > > > The motivating use case was the introduction of several new binary > > options for vf_libplacebo, but other filters that currently rely on > > manual file-path loading could benefit from it as well. > > Sorry, I think having an arbitrary file loader in the options parser > will be an endless security nightmare. > > The alternative I had in mind was having ffmpeg.c itself do the file > loading. This will be require some modification of the options parsing > code in cmdutils.c and also extending the mechanisms we use to pass > options to filters. > > I can try to make a POC in a few days. I think that is the better approach, too. I went for this approach because it was the easiest to implement, not because it makes the most sense. I do think that it's fine for ffmpeg.c to do this, but springing it upon unsuspecting API users *in general* is, well, in retrospect, not something I want to have my name attached to. I think that I would drop the second half of this commit (keeping only the introduction of `av_set_string_from_filepath`), and then make ffmpeg.c use it for cmdlist arguments starting with '@' as a separate commit.
diff --git a/doc/APIchanges b/doc/APIchanges index ea402f6118..0400b10721 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -14,6 +14,10 @@ libavutil: 2021-04-27 API changes, most recent first: +2022-03-04 - xxxxxxxxxx - lavu 57.23.100 - opt.h + Add av_opt_set_filepath() to set a binary option from a file. + Support '@filename' syntax in av_opt_set() for binary options. + 2022-02-07 - xxxxxxxxxx - lavu 57.21.100 - fifo.h Deprecate AVFifoBuffer and the API around it, namely av_fifo_alloc(), av_fifo_alloc_array(), av_fifo_free(), av_fifo_freep(), av_fifo_reset(), diff --git a/libavutil/opt.c b/libavutil/opt.c index d951edca9d..151536d229 100644 --- a/libavutil/opt.c +++ b/libavutil/opt.c @@ -32,6 +32,7 @@ #include "common.h" #include "dict.h" #include "eval.h" +#include "file.h" #include "log.h" #include "parseutils.h" #include "pixdesc.h" @@ -465,6 +466,33 @@ static int set_string_dict(void *obj, const AVOption *o, const char *val, uint8_ return 0; } +static int set_string_filepath(void *obj, const AVOption *o, const char *path, uint8_t **dst) +{ + int *lendst = (int *)(dst + 1); + uint8_t *bin; + size_t bin_len; + int err; + + av_freep(dst); + *lendst = 0; + + if (!path || !path[0]) + return 0; + + err = av_file_map(path, &bin, &bin_len, 0, obj); + if (err) + return err; + + *dst = av_memdup(bin, bin_len); + av_file_unmap(bin, bin_len); + + if (!*dst) + return AVERROR(ENOMEM); + + *lendst = bin_len; + return 0; +} + int av_opt_set(void *obj, const char *name, const char *val, int search_flags) { int ret = 0; @@ -492,7 +520,11 @@ int av_opt_set(void *obj, const char *name, const char *val, int search_flags) case AV_OPT_TYPE_STRING: return set_string(obj, o, val, dst); case AV_OPT_TYPE_BINARY: - return set_string_binary(obj, o, val, dst); + if (val && val[0] == '@') { + return set_string_filepath(obj, o, &val[1], dst); + } else { + return set_string_binary(obj, o, val, dst); + } case AV_OPT_TYPE_FLAGS: case AV_OPT_TYPE_INT: case AV_OPT_TYPE_INT64: @@ -631,6 +663,25 @@ int av_opt_set_bin(void *obj, const char *name, const uint8_t *val, int len, int return 0; } +int av_opt_set_filepath(void *obj, const char *name, const char *path, int search_flags) +{ + uint8_t *bin; + size_t bin_len; + int err; + + if (!path || !path[0]) + return av_opt_set_bin(obj, name, NULL, 0, search_flags); + + err = av_file_map(path, &bin, &bin_len, 0, obj); + if (err) + return err; + + err = av_opt_set_bin(obj, name, bin, bin_len, search_flags); + av_file_unmap(bin, bin_len); + + return err; +} + int av_opt_set_image_size(void *obj, const char *name, int w, int h, int search_flags) { void *target_obj; diff --git a/libavutil/opt.h b/libavutil/opt.h index 2820435eec..97d7c69482 100644 --- a/libavutil/opt.h +++ b/libavutil/opt.h @@ -699,6 +699,11 @@ int av_opt_set_channel_layout(void *obj, const char *name, int64_t ch_layout, in * caller still owns val is and responsible for freeing it. */ int av_opt_set_dict_val(void *obj, const char *name, const AVDictionary *val, int search_flags); +/** + * @note This behaves like av_opt_set_bin() but loads the binary data from a + * file. May also return errors related to file I/O. + */ +int av_opt_set_filepath(void *obj, const char *name, const char *val, int search_flags); /** * Set a binary option to an integer list. diff --git a/libavutil/version.h b/libavutil/version.h index c7004601c8..6c5d90507c 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 57 -#define LIBAVUTIL_VERSION_MINOR 22 +#define LIBAVUTIL_VERSION_MINOR 23 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
From: Niklas Haas <git@haasn.dev> I arbitrarily decided to use the syntax 'opt=@filename' to match e.g. `curl -Ffield=@filename`, and also because @ is not a valid hex character, nor does it conflict with any other common shell or ffmpeg syntax. This is arguably somewhat clunky because it does not round-trip with av_opt_get - you will get back a hex interpretation of the loaded file, rather than the filename it was loaded from. It also implies a (perhaps unnecessary) memcpy from mapped file memory into a allocated memory. This is unfortunately necessary because there's no way for us to know whether av_free or av_file_unmap is needed to clean up previous option values. The motivating use case was the introduction of several new binary options for vf_libplacebo, but other filters that currently rely on manual file-path loading could benefit from it as well. --- doc/APIchanges | 4 ++++ libavutil/opt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++- libavutil/opt.h | 5 +++++ libavutil/version.h | 2 +- 4 files changed, 62 insertions(+), 2 deletions(-)