[FFmpeg-devel] [PATCH] ffmpeg_opt: remove the stats metadata added by mkvmerge.

[wm4]

On Thu,  6 Apr 2017 12:27:43 +0200
Nicolas George <george at nsup.org> wrote:

> They are probably not valid for the resulting file.
> They look like this:
>       BPS             : 40665
>       BPS-eng         : 40665
>       DURATION        : 00:00:20.000000000
>       DURATION-eng    : 00:00:20.000000000
>       NUMBER_OF_FRAMES: 10
>       NUMBER_OF_FRAMES-eng: 10
>       NUMBER_OF_BYTES : 101664
>       NUMBER_OF_BYTES-eng: 101664
>       _STATISTICS_WRITING_APP: mkvmerge v9.8.0 ('Kuglblids') 64bit
>       _STATISTICS_WRITING_APP-eng: mkvmerge v9.8.0 ('Kuglblids') 64bit
>       _STATISTICS_WRITING_DATE_UTC: 2017-04-06 08:22:08
>       _STATISTICS_WRITING_DATE_UTC-eng: 2017-04-06 08:22:08
>       _STATISTICS_TAGS: BPS DURATION NUMBER_OF_FRAMES NUMBER_OF_BYTES
>       _STATISTICS_TAGS-eng: BPS DURATION NUMBER_OF_FRAMES NUMBER_OF_BYTES
> 
> Signed-off-by: Nicolas George <george at nsup.org>
> ---
>  ffmpeg_opt.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> 
> Note: as written in a comment, this code ignores the return value of
> av_dict_set(), because the surrounding code ignores the return value of
> av_dict_copy(). A lot of unchecked errors happen in this function, but I am
> not reworking a 2700-lines function today.
> 
> 
> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> index d1fe8742ff..f72f38796a 100644
> --- a/ffmpeg_opt.c
> +++ b/ffmpeg_opt.c
> @@ -497,6 +497,53 @@ static void parse_meta_type(char *arg, char *type, int *index, const char **stre
>          *type = 'g';
>  }
>  
> +/**
> + * Copy a dictionary except the stats metadata added by mkvmerge.
> + * They are probably not valid for the resulting file.
> + *
> + * FIXME Return values are ignored by the surrounding code and here.
> + */
> +static void dict_copy_nostats(AVDictionary **dst, const AVDictionary *src)
> +{
> +    AVDictionaryEntry *t;
> +    const char *tags = NULL, *p, *q;
> +    size_t len;
> +
> +    t = av_dict_get(src, "_STATISTICS_TAGS", NULL, AV_DICT_MATCH_CASE);
> +    if (t)
> +        tags = t->value;
> +    t = NULL;
> +    while ((t = av_dict_get(src, "", t, AV_DICT_IGNORE_SUFFIX))) {
> +        if (av_strstart(t->key, "_STATISTICS_", NULL))
> +            continue;
> +        if (tags) {
> +            /* tags is a space-separated list of stats tags */
> +            p = tags;
> +            while (*p) {
> +                q = strchr(p, ' ');
> +                len = q ? q - p : strlen(p);
> +                if (!q)
> +                    q = p + strlen(p);
> +#define ff_islower(c) ((unsigned)((c) - 'a') < 26)
> +                if (!memcmp(t->key, p, len) &&
> +                    (!t->key[len] ||
> +                     /* also remove TAG-lng */
> +                     (t->key[len] == '-' &&
> +                      ff_islower(t->key[len + 1]) &&
> +                      ff_islower(t->key[len + 2]) &&
> +                      ff_islower(t->key[len + 3]) &&
> +                      !t->key[len + 4])))
> +                    break;
> +#undef ff_islower
> +                for (p += len; *p == ' '; p++);
> +            }
> +            if (*p) /* match found => do not copy */
> +                continue;
> +        }
> +        av_dict_set(dst, t->key, t->value, AV_DICT_DONT_OVERWRITE);
> +    }
> +}
> +
>  static int copy_metadata(char *outspec, char *inspec, AVFormatContext *oc, AVFormatContext *ic, OptionsContext *o)
>  {
>      AVDictionary **meta_in = NULL;
> @@ -2546,7 +2593,7 @@ loop_end:
>              if (output_streams[i]->source_index < 0)         /* this is true e.g. for attached files */
>                  continue;
>              ist = input_streams[output_streams[i]->source_index];
> -            av_dict_copy(&output_streams[i]->st->metadata, ist->st->metadata, AV_DICT_DONT_OVERWRITE);
> +            dict_copy_nostats(&output_streams[i]->st->metadata, ist->st->metadata);
>              if (!output_streams[i]->stream_copy) {
>                  av_dict_set(&output_streams[i]->st->metadata, "encoder", NULL, 0);
>              }

I sure love format-specific hacks in ffmpeg CLI!

I don't think this patch is acceptable.

I went on trying to find out how Matroska readers are supposed to
handle this:

<wm4> mosu: what's the correct way to discard _STATISTICS tags if you want only "actual" tags the user added?
<wm4> matching by name doesn't work, because there are many names to be considered and they don't all start with _ or _STATISTICS
<mosu> You mean in the context of programs other than mkvmerge?
<wm4> yes
<mosu> mkvmerge adds one tag called _STATISTICS_TAGS which includes the a space-separated list of names of "normal" tags it has added. Additionally it adds _STATISTICS_WRITING_APP and _STATISTICS_WRITING_DATE.
<wm4> that's... painful
<mosu> There's nothing in the specs about this, though; it's a convention created by/invented for mkvmerge

So that's what you have to implement in the demuxer.