[FFmpeg-devel] [PATCH 4/5] concatdec: add metadata for file start time and duration

[Marton Balint]

On Sun, 25 Oct 2015, Nicolas George wrote:

> Le tridi 3 brumaire, an CCXXIV, Marton Balint a écrit :
>> Signed-off-by: Marton Balint <cus at passwd.hu>
>> ---
>>  libavformat/concatdec.c                    | 6 ++++++
>>  tests/ref/fate/concat-demuxer-lavf-mxf     | 2 +-
>>  tests/ref/fate/concat-demuxer-lavf-mxf_d10 | 2 +-
>>  tests/ref/fate/concat-demuxer-lavf-ts      | 2 +-
>>  4 files changed, 9 insertions(+), 3 deletions(-)
>
> I suspect that a basic run of ffmpeg to remux:
>
> ffmpeg -i list.concat -c copy out.mkv
>
> ... would result in out.mkv having the metadata strings in it, which would
> not be ok IMHO.

I couldn't actually reproduce this neither with mkv nor with nut, but I 
see that you are worried that the metadata may be kept in the output.

I'd rather keep this as it is, the user has to control the metadata he 
wants in his output, we never promised there won't be additional packet 
string metadata in a certain format, and the way I see it only NUT muxer 
actually parses AV_PKT_DATA_STRINGS_METADATA, and none of the encoders 
parse AVFrame metadata, so it does not really affect anybody.

If you still think this is problematic, I can do one or both of these 
options:
- Add a demuxer option which controls if the demuxer produces this
metadata or not.
- Add an ffconcat file global option which controls this.

>>
>> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
>> index f262d44..51b9703 100644
>> --- a/libavformat/concatdec.c
>> +++ b/libavformat/concatdec.c
>> @@ -289,6 +289,7 @@ static int open_file(AVFormatContext *avf, unsigned fileno)
>>  {
>>      ConcatContext *cat = avf->priv_data;
>>      ConcatFile *file = &cat->files[fileno];
>> +    AVDictionaryEntry *entry;
>>      int ret;
>>
>>      if (cat->avf)
>> @@ -324,6 +325,11 @@ static int open_file(AVFormatContext *avf, unsigned fileno)
>>              file->duration -= cat->avf->duration - (file->outpoint - file->file_start_time);
>>      }
>>
>
>> +    if (!(entry = av_dict_get(file->metadata, "lavf.concatdec.start_time", NULL, 0)))
>> +        av_dict_set_int(&file->metadata, "lavf.concatdec.start_time", file->start_time, 0);
>> +    if (!(entry = av_dict_get(file->metadata, "lavf.concatdec.duration", NULL, 0)))
>> +        av_dict_set_int(&file->metadata, "lavf.concatdec.duration", file->duration, 0);
>
> Since the metadata is properly namespaced, I do not think the test are
> necessary.

Ok.

>
>> +
>>      if ((ret = match_streams(avf)) < 0)
>>          return ret;
>>      if (file->inpoint != AV_NOPTS_VALUE) {
>> diff --git a/tests/ref/fate/concat-demuxer-lavf-mxf b/tests/ref/fate/concat-demuxer-lavf-mxf
>> index a6fa554..d6c82d6 100644
>> --- a/tests/ref/fate/concat-demuxer-lavf-mxf
>> +++ b/tests/ref/fate/concat-demuxer-lavf-mxf
>> @@ -1 +1 @@
>
>> -56359998da34c3957124a8928fb58f3d *tests/data/fate/concat-demuxer-lavf-mxf.ffprobe
>> +23cd3acf3db9ee19228f381f05f1f3b9 *tests/data/fate/concat-demuxer-lavf-mxf.ffprobe
>
> If the ref files were not hashed, it would be easier to be sure the change
> is valid.
>
>> diff --git a/tests/ref/fate/concat-demuxer-lavf-mxf_d10 b/tests/ref/fate/concat-demuxer-lavf-mxf_d10
>> index 018d631..08777f7 100644
>> --- a/tests/ref/fate/concat-demuxer-lavf-mxf_d10
>> +++ b/tests/ref/fate/concat-demuxer-lavf-mxf_d10
>> @@ -1 +1 @@
>> -89c81149b4673c60aba7cf5f27cec823 *tests/data/fate/concat-demuxer-lavf-mxf_d10.ffprobe
>> +bd1c6cc871fe5193186a03554ebc84c1 *tests/data/fate/concat-demuxer-lavf-mxf_d10.ffprobe
>> diff --git a/tests/ref/fate/concat-demuxer-lavf-ts b/tests/ref/fate/concat-demuxer-lavf-ts
>> index 2e8ba46..a01f712 100644
>> --- a/tests/ref/fate/concat-demuxer-lavf-ts
>> +++ b/tests/ref/fate/concat-demuxer-lavf-ts
>> @@ -1 +1 @@
>> -1993b3613952fa76da8c5c260a16a96a *tests/data/fate/concat-demuxer-lavf-ts.ffprobe
>> +728e773e5009f7f652c1677573b6c8d2 *tests/data/fate/concat-demuxer-lavf-ts.ffprobe
>

Regards,
Marton