[FFmpeg-devel,v3] avformat/concatdec: add support for setting input options

This way protocol or format related options can be set for all
of the files opened during concatenation both globally as well
as per-file.
---

Changes from v2:

1. Added an example, although I had issues figuring out something useful
   that is not a hack for something. Ended up doing a disablement of
   advanced edit list functionality, since that can sometimes lead to
   unwanted behavior.

   * First idea was to override the input format for a file without an
     extension. For that, we have no AVOption it seems.
   * Then came the idea of utilizing the framerate option in the raw
     h264 demuxer. That didn't work because apparently if there is a header
     in there that probed/parsed frame rate field gets utilized.
   * Third idea was either the one I picked, or analyzeduration/probesize
     for MPEG-TS. I opted for the mp4 case.

2. Quoted the : in documentation with @code{:}.
3. Fixed a duplicate space in a log message.
---

 doc/demuxers.texi       | 24 ++++++++++++++
 libavformat/concatdec.c | 69 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 92 insertions(+), 1 deletion(-)

Jan Ekström (12021-02-08):
> This way protocol or format related options can be set for all
> of the files opened during concatenation both globally as well
> as per-file.
> ---
> 
> Changes from v2:
> 
> 1. Added an example, although I had issues figuring out something useful
>    that is not a hack for something. Ended up doing a disablement of
>    advanced edit list functionality, since that can sometimes lead to
>    unwanted behavior.
> 

>    * First idea was to override the input format for a file without an
>      extension. For that, we have no AVOption it seems.

Indeed, lavf does not accept it as an option. It needs to be added
separately. Or we can make it an option.

>    * Then came the idea of utilizing the framerate option in the raw
>      h264 demuxer. That didn't work because apparently if there is a header
>      in there that probed/parsed frame rate field gets utilized.

You could set the frame rate for a raw video stream.

>    * Third idea was either the one I picked, or analyzeduration/probesize
>      for MPEG-TS. I opted for the mp4 case.
> 
> 2. Quoted the : in documentation with @code{:}.
> 3. Fixed a duplicate space in a log message.
> ---
> 
>  doc/demuxers.texi       | 24 ++++++++++++++
>  libavformat/concatdec.c | 69 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> index 3c15ab9eee..20601f9575 100644
> --- a/doc/demuxers.texi
> +++ b/doc/demuxers.texi
> @@ -149,6 +149,14 @@ 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.
>  
> +@item @code{input_options @var{key=value:key2=value2}}

> +Input options passed on when reading a specific file, using a @code{:}-separated
> +list of key=value pairs.

No! Why would you do that? It adds one level of escaping, we already
have way too much of them.

input option key=value

singular, and can be set as many times as necessary.

> Requires @code{safe} to be non-positive.

Zero would be safer.

> Global options
> +for all files can be set with the @code{input_options} demuxer option. When using
> +both options on the list of files as well as globally via the demuxer option,
> +the global ones get applied first and the file-specific options are then applied
> +on top of them.

I am not sure it is the right order. Options added on the fly should
take precedence over options written in a file, in general. But the
global / specific distinction makes it less clear-cut.

Other opinions?

> +
>  @item @code{stream}
>  Introduce a stream in the virtual file.
>  All subsequent stream-related directives apply to the last introduced
> @@ -204,6 +212,10 @@ expressed in microseconds. The duration metadata is only set if it is known
>  based on the concat file.
>  The default is 0.
>  
> +@item input_options
> +Input options to be passed on for all opened inputs using

> a :-separated list of

It should say "a dictionary", with a link to where the syntax of
dictionaries is documented, but whoever added AV_OPT_TYPE_DICT neglected
to add the corresponding documentation paragraph :(

> +key=value pairs.
> +
>  @end table
>  
>  @subsection Examples
> @@ -231,6 +243,18 @@ duration 20.0
>  
>  file subdir/file-2.wav
>  @end example
> +
> +@item
> +Disabling advanced edit list capability for the first input file via
> +input_options:
> +@example
> +ffconcat version 1.0
> +
> +file file-1.mp4
> +input_options advanced_editlist=false
> +
> +file file-2.mp4
> +@end example
>  @end itemize
>  
>  @section dash
> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
> index 6d5b9914f9..89d75cedc6 100644
> --- a/libavformat/concatdec.c
> +++ b/libavformat/concatdec.c
> @@ -52,6 +52,7 @@ typedef struct {
>      int64_t outpoint;
>      AVDictionary *metadata;
>      int nb_streams;
> +    AVDictionary *input_options;
>  } ConcatFile;
>  
>  typedef struct {
> @@ -66,6 +67,7 @@ typedef struct {
>      ConcatMatchMode stream_match_mode;
>      unsigned auto_convert;
>      int segment_time_metadata;
> +    AVDictionary *input_options;
>  } ConcatContext;
>  
>  static int concat_probe(const AVProbeData *probe)
> @@ -329,6 +331,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 +347,37 @@ 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 ||
> +    // Apply global AVOptions first
> +    if (cat->input_options &&
> +        (ret = av_dict_copy(&options, cat->input_options, 0) < 0))
> +        return ret;
> +
> +    // then apply file-specific AVOptions
> +    if (file->input_options &&
> +        (ret = av_dict_copy(&options, file->input_options, 0) < 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);
> +        av_dict_free(&options);
>          return ret;
>      }
> +
> +    if (av_dict_count(options)) {
> +        AVDictionaryEntry *en = NULL;
> +
> +        while ((en = av_dict_get(options, "", en, AV_DICT_IGNORE_SUFFIX))) {
> +            av_log(avf, AV_LOG_WARNING,
> +                   "Option '%s' set to '%s' was ignored when opening %s "
> +                   "with the %s reader!\n",
> +                   en->key, en->value, file->url, cat->avf->iformat->name);
> +        }
> +    }
> +
> +    av_dict_free(&options);
> +
>      cat->cur_file = file;
>      file->start_time = !fileno ? 0 :
>                         cat->files[fileno - 1].start_time +
> @@ -386,6 +414,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].input_options);
>      }
>      if (cat->avf)
>          avformat_close_input(&cat->avf);
> @@ -457,6 +486,41 @@ static int concat_read_header(AVFormatContext *avf)
>                  FAIL(AVERROR_INVALIDDATA);
>              }
>              av_freep(&metadata);
> +        } else if (!strncmp(keyword, "input_options", 13)) {
> +            if (!file) {
> +                av_log(avf, AV_LOG_ERROR, "Line %d: %s without file\n",
> +                       line, keyword);
> +                FAIL(AVERROR_INVALIDDATA);
> +            }
> +
> +            if (cat->safe > 0) {
> +                av_log(avf, AV_LOG_ERROR,
> +                       "Line %d: Input options cannot be set in file list in "
> +                       "safe mode!\n", line);
> +                FAIL(AVERROR(EPERM));
> +            }
> +
> +            {
> +                char *input_options = av_get_token((const char **)&cursor,
> +                                                   SPACE_CHARS);
> +                if (!input_options) {
> +                    av_log(avf, AV_LOG_ERROR,
> +                           "Line %d: key=value pairs required!\n", line);
> +                    FAIL(AVERROR_INVALIDDATA);
> +                }
> +

> +                if ((ret =
> +                     av_dict_parse_string(&file->input_options, input_options,
> +                                          "=", ":", 0)) < 0) {

Just find the first = and add a single key-value.

> +                    av_log(avf, AV_LOG_ERROR,
> +                           "Line %d: failed to parse input options string\n",
> +                           line);
> +                    av_freep(&input_options);
> +                    FAIL(AVERROR_INVALIDDATA);
> +                }
> +
> +                av_freep(&input_options);
> +            }
>          } else if (!strcmp(keyword, "stream")) {
>              if (!avformat_new_stream(avf, NULL))
>                  FAIL(AVERROR(ENOMEM));
> @@ -764,6 +828,9 @@ static const AVOption options[] = {
>        OFFSET(auto_convert), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, DEC },
>      { "segment_time_metadata", "output file segment start time and duration as packet metadata",
>        OFFSET(segment_time_metadata), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, DEC },
> +    { "input_options",
> +      "set options for all opened inputs using a :-separated list of key=value pairs",
> +      OFFSET(input_options), AV_OPT_TYPE_DICT, { .str = NULL }, 0, 0, DEC },
>      { NULL }
>  };
>  

Regards,

On Wed, Feb 10, 2021 at 8:33 PM Nicolas George <george@nsup.org> wrote:
>
> Jan Ekström (12021-02-08):
> > This way protocol or format related options can be set for all
> > of the files opened during concatenation both globally as well
> > as per-file.
> > ---
> >
> > Changes from v2:
> >
> > 1. Added an example, although I had issues figuring out something useful
> >    that is not a hack for something. Ended up doing a disablement of
> >    advanced edit list functionality, since that can sometimes lead to
> >    unwanted behavior.
> >
>
> >    * First idea was to override the input format for a file without an
> >      extension. For that, we have no AVOption it seems.
>
> Indeed, lavf does not accept it as an option. It needs to be added
> separately. Or we can make it an option.
>
> >    * Then came the idea of utilizing the framerate option in the raw
> >      h264 demuxer. That didn't work because apparently if there is a header
> >      in there that probed/parsed frame rate field gets utilized.
>
> You could set the frame rate for a raw video stream.
>

Sure.

> >    * Third idea was either the one I picked, or analyzeduration/probesize
> >      for MPEG-TS. I opted for the mp4 case.
> >
> > 2. Quoted the : in documentation with @code{:}.
> > 3. Fixed a duplicate space in a log message.
> > ---
> >
> >  doc/demuxers.texi       | 24 ++++++++++++++
> >  libavformat/concatdec.c | 69 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 92 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> > index 3c15ab9eee..20601f9575 100644
> > --- a/doc/demuxers.texi
> > +++ b/doc/demuxers.texi
> > @@ -149,6 +149,14 @@ 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.
> >
> > +@item @code{input_options @var{key=value:key2=value2}}
>
> > +Input options passed on when reading a specific file, using a @code{:}-separated
> > +list of key=value pairs.
>
> No! Why would you do that? It adds one level of escaping, we already
> have way too much of them.

Because:
1. we have the functionality for it
2. it matches the AVOption.

Please do not take it as if I did not have any logic behind my actions
whatsoever, which is what the exclamation mark looks like.

>
> input option key=value
>
> singular, and can be set as many times as necessary.
>
> > Requires @code{safe} to be non-positive.
>
> Zero would be safer.
>
> > Global options
> > +for all files can be set with the @code{input_options} demuxer option. When using
> > +both options on the list of files as well as globally via the demuxer option,
> > +the global ones get applied first and the file-specific options are then applied
> > +on top of them.
>
> I am not sure it is the right order. Options added on the fly should
> take precedence over options written in a file, in general. But the
> global / specific distinction makes it less clear-cut.
>

You set the AVOption for option(s) to be applied for all files, and if
you want to only set some option for specific files you set them in
the list.

*You* requested the latter capability, I added it.

> Other opinions?
>
> > +
> >  @item @code{stream}
> >  Introduce a stream in the virtual file.
> >  All subsequent stream-related directives apply to the last introduced
> > @@ -204,6 +212,10 @@ expressed in microseconds. The duration metadata is only set if it is known
> >  based on the concat file.
> >  The default is 0.
> >
> > +@item input_options
> > +Input options to be passed on for all opened inputs using
>
> > a :-separated list of
>
> It should say "a dictionary", with a link to where the syntax of
> dictionaries is documented, but whoever added AV_OPT_TYPE_DICT neglected
> to add the corresponding documentation paragraph :(
>

Yes. This just matches the rest of the documentation strings etc for
such things.

Jan

Jan Ekström (12021-02-10):
> > No! Why would you do that? It adds one level of escaping, we already
> > have way too much of them.
> 
> Because:
> 1. we have the functionality for it
> 2. it matches the AVOption.
> 
> Please do not take it as if I did not have any logic behind my actions
> whatsoever, which is what the exclamation mark looks like.

I am sorry it gave that impression. I wanted just to express my dismay
at having to reject this version once again.

The logic you applied makes sense, but only if you forget that this
key-value parser is there to work around a limitation of the options
parsing code, i.e. the fact that only one value can be set for each
option. And it comes with drawbacks, mainly the need to escape. If our
options parser was more powerful or better designed, we could gladly get
rid of this parsed.

The parser for the concat script does not suffer from this limitations,
and therefore does not need to be saddled with the corresponding
drawbacks.

> > I am not sure it is the right order. Options added on the fly should
> > take precedence over options written in a file, in general. But the
> > global / specific distinction makes it less clear-cut.
> >
> 
> You set the AVOption for option(s) to be applied for all files, and if
> you want to only set some option for specific files you set them in
> the list.
> 
> *You* requested the latter capability, I added it.

And I was talking about the precedence order in which you added it.

> Yes. This just matches the rest of the documentation strings etc for
> such things.

Again, the lack of care for anything that is not directly related to the
core functionality makes our work harder and less good later.

Regards,