Message ID | 20210831122209.586348-2-george@nsup.org |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/6] lavf/concat: refactor parsing | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
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,
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);
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(+)