[FFmpeg-devel,2/6] lavf/concat: add file_packet_meta directive

Same as file_packet_metadata without the double parsing.

Signed-off-by: Nicolas George <george@nsup.org>
---
 doc/demuxers.texi       | 5 +++++
 libavformat/concatdec.c | 8 ++++++++
 2 files changed, 13 insertions(+)

Nicolas George:
> Same as file_packet_metadata without the double parsing.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  doc/demuxers.texi       | 5 +++++
>  libavformat/concatdec.c | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> index eb3351833a..f338700396 100644
> --- a/doc/demuxers.texi
> +++ b/doc/demuxers.texi
> @@ -151,6 +151,11 @@ 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{file_packet_meta @var{key} @var{value}}
> +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{option @var{key} @var{value}}
>  Option to access, open and probe the file.
>  Can be present multiple times.
> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
> index 223c7e36c4..76f3fafa50 100644
> --- a/libavformat/concatdec.c
> +++ b/libavformat/concatdec.c
> @@ -425,6 +425,7 @@ typedef enum ParseDirective {
>     DIR_DURATION,
>     DIR_INPOINT,
>     DIR_OUTPOINT,
> +   DIR_FPMETA,
>     DIR_FPMETAS,
>     DIR_OPTION,
>     DIR_STREAM,
> @@ -437,6 +438,7 @@ static const ParseSyntax syntax[] = {
>      [DIR_DURATION ] = { "duration",             "d",  NEEDS_FILE },
>      [DIR_INPOINT  ] = { "inpoint",              "d",  NEEDS_FILE },
>      [DIR_OUTPOINT ] = { "outpoint",             "d",  NEEDS_FILE },
> +    [DIR_FPMETA   ] = { "file_packet_meta",     "ks", NEEDS_FILE },
>      [DIR_FPMETAS  ] = { "file_packet_metadata", "s",  NEEDS_FILE },
>      [DIR_OPTION   ] = { "option",               "ks", NEEDS_FILE | NEEDS_UNSAFE },
>      [DIR_STREAM   ] = { "stream",               "",   0 },
> @@ -544,6 +546,12 @@ static int concat_parse_script(AVFormatContext *avf)
>          case DIR_OUTPOINT:
>              file->outpoint = arg_int[0];
>              break;
> +        case DIR_FPMETA:
> +            ret = av_dict_set(&file->metadata, arg_kw[0], arg_str[1], AV_DICT_DONT_STRDUP_VAL);
> +            if (ret < 0)
> +                FAIL(ret);
> +            arg_str[1] = NULL;
> +            break;
>          case DIR_FPMETAS:
>              if ((ret = av_dict_parse_string(&file->metadata, arg_str[0], "=", "", 0)) < 0) {
>                  av_log(avf, AV_LOG_ERROR, "Line %d: failed to parse metadata string\n", line);
> 
You seem to misunderstand the semantics of the AV_DICT_DONT_STRDUP_*
flags: av_dict_set() takes complete ownership of the strings, even on
errors. So in case of errors the above code would lead to a double-free.
Patches #1 and #4 of this set are also affected by this.

- Andreas

Andreas Rheinhardt (12021-08-31):
> You seem to misunderstand the semantics of the AV_DICT_DONT_STRDUP_*
> flags: av_dict_set() takes complete ownership of the strings, even on
> errors. So in case of errors the above code would lead to a double-free.
> Patches #1 and #4 of this set are also affected by this.

Thanks, I made the same mistake last time. It is counter-intuitive:
realloc() does not free its argument if it fails, for example. Anyway,
fixed by moving the =NULL earlier.

Stand by for a new version with more features.

Regards,

Nicolas George:
> Andreas Rheinhardt (12021-08-31):
>> You seem to misunderstand the semantics of the AV_DICT_DONT_STRDUP_*
>> flags: av_dict_set() takes complete ownership of the strings, even on
>> errors. So in case of errors the above code would lead to a double-free.
>> Patches #1 and #4 of this set are also affected by this.
> 
> Thanks, I made the same mistake last time. It is counter-intuitive:
> realloc() does not free its argument if it fails, for example. Anyway,
> fixed by moving the =NULL earlier.
> 
realloc is not supposed to transfer ownership, av_dict_set() with these
flags is. For me its behaviour makes sense (and avoids lots of av_free()
on failure paths).

- Andreas

Andreas Rheinhardt (12021-08-31):
> realloc is not supposed to transfer ownership, av_dict_set() with these
> flags is. For me its behaviour makes sense (and avoids lots of av_free()
> on failure paths).

This is C, not Rust: ownership rules are in the eye of the beholder.

The objective thing to say is: av_dict_set() with DONT_STRDUP destroys
its argument in case of failure, realloc() does not.

Anyway, the problem is fixed, let us not split hairs furthers on this,
and thank you for spotting it.

Regards,