[FFmpeg-devel] [PATCH 2/5] lavf/concatdec: support per-file options.

[Andreas Rheinhardt]

Nicolas George:
> Signed-off-by: Nicolas George <george at nsup.org>
> ---
>  doc/demuxers.texi       |  4 ++++
>  libavformat/concatdec.c | 37 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> 
> I intend to refactor the parser some time later.
> 
> 
> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> index 5f18e4551b..eb3351833a 100644
> --- a/doc/demuxers.texi
> +++ b/doc/demuxers.texi
> @@ -151,6 +151,10 @@ Metadata of the packets of the file. The specified metadata will be set for
>  each file packet. You can specify this directive multiple times to add multiple
>  metadata entries.
>  
> + at item @code{option @var{key} @var{value}}
> +Option to access, open and probe the file.
> +Can be present multiple times.
> +
>  @item @code{stream}
>  Introduce a stream in the virtual file.
>  All subsequent stream-related directives apply to the last introduced
> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
> index 3db1693725..0203b1e6dc 100644
> --- a/libavformat/concatdec.c
> +++ b/libavformat/concatdec.c
> @@ -51,6 +51,7 @@ typedef struct {
>      int64_t inpoint;
>      int64_t outpoint;
>      AVDictionary *metadata;
> +    AVDictionary *options;
>      int nb_streams;
>  } ConcatFile;
>  
> @@ -329,6 +330,7 @@ static int open_file(AVFormatContext *avf, unsigned fileno)
>  {
>      ConcatContext *cat = avf->priv_data;
>      ConcatFile *file = &cat->files[fileno];
> +    AVDictionary *options = NULL;
>      int ret;
>  
>      if (cat->avf)
> @@ -344,12 +346,21 @@ static int open_file(AVFormatContext *avf, unsigned fileno)
>      if ((ret = ff_copy_whiteblacklists(cat->avf, avf)) < 0)
>          return ret;
>  
> -    if ((ret = avformat_open_input(&cat->avf, file->url, NULL, NULL)) < 0 ||
> +    ret = av_dict_copy(&options, file->options, 0);
> +    if (ret < 0)
> +        return ret;
> +
> +    if ((ret = avformat_open_input(&cat->avf, file->url, NULL, &options)) < 0 ||
>          (ret = avformat_find_stream_info(cat->avf, NULL)) < 0) {
>          av_log(avf, AV_LOG_ERROR, "Impossible to open '%s'\n", file->url);
>          avformat_close_input(&cat->avf);
>          return ret;

options would leak here.

>      }
> +    if (options) {
> +        av_log(avf, AV_LOG_WARNING, "Unused options for '%s'.\n", file->url);
> +        /* TODO log unused options once we have a proper string API */
> +        av_dict_free(&options);
> +    }
>      cat->cur_file = file;
>      file->start_time = !fileno ? 0 :
>                         cat->files[fileno - 1].start_time +
> @@ -386,6 +397,7 @@ static int concat_read_close(AVFormatContext *avf)
>          }
>          av_freep(&cat->files[i].streams);
>          av_dict_free(&cat->files[i].metadata);
> +        av_dict_free(&cat->files[i].options);
>      }
>      if (cat->avf)
>          avformat_close_input(&cat->avf);
> @@ -457,6 +469,29 @@ static int concat_read_header(AVFormatContext *avf)
>                  FAIL(AVERROR_INVALIDDATA);
>              }
>              av_freep(&metadata);
> +        } else if (!strcmp(keyword, "option")) {
> +            char *key, *val;
> +            if (cat->safe) {
> +                av_log(avf, AV_LOG_ERROR, "Options not permitted in safe mode.\n");
> +                FAIL(AVERROR(EPERM));
> +            }
> +            if (!file) {
> +                av_log(avf, AV_LOG_ERROR, "Line %d: %s without file\n",
> +                       line, keyword);
> +                FAIL(AVERROR_INVALIDDATA);
> +            }
> +            if (!(key = av_get_token((const char **)&cursor, SPACE_CHARS)) ||
> +                !(val = av_get_token((const char **)&cursor, SPACE_CHARS))) {> +                av_log(avf, AV_LOG_ERROR, "Line %d: key and val
required\n", line);
> +                FAIL(AVERROR_INVALIDDATA);

1. key might leak here.

2. The error message and the return code are wrong: av_get_token() only
fails upon allocation error; it does not fail when *cursor == \0.
(The code already suffers from this misconception.)

> +            }
> +            ret = av_dict_set(&file->options, key, val,
> +                              AV_DICT_DONT_STRDUP_KEY | AV_DICT_DONT_STRDUP_VAL);
> +            if (ret < 0) {
> +                av_freep(&key);
> +                av_freep(&val);

Double free.

> +                FAIL(ret);
> +            }
>          } else if (!strcmp(keyword, "stream")) {
>              if (!avformat_new_stream(avf, NULL))
>                  FAIL(AVERROR(ENOMEM));
>