[FFmpeg-devel] [PATCH] libavformat/matroskaenc.c: Add option to set timecodescale

[Lynne]

Jan 13, 2021, 20:58 by tfoucu at gmail.com:

> HI Lynne
>
> On Wed, Jan 13, 2021 at 10:30 AM Lynne <> dev at lynne.ee> > wrote:
>
>> Jan 13, 2021, 18:46 by >> tfoucu at gmail.com>> :
>>
>> > By default the time code scale in a MKV file in millisecond. With this
>> > option we can set the time code scale to microsecond or nanoseconds for
>> > very high frame rate.
>> > ---
>> >  libavformat/matroskaenc.c | 11 +++++++----
>> >  1 file changed, 7 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> > index 233c472b8f..cfad6a4693 100644
>> > --- a/libavformat/matroskaenc.c
>> > +++ b/libavformat/matroskaenc.c
>> > @@ -158,6 +158,8 @@ typedef struct MatroskaMuxContext {
>> >  int                 default_mode;
>> >
>> >  uint32_t            segment_uid[4];
>> > +
>> > +    int64_t             timecodescale;
>> >  } MatroskaMuxContext;
>> >
>> >  /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
>> > @@ -1827,7 +1829,7 @@ static int mkv_write_header(AVFormatContext *s)
>> >  return ret;
>> >  pb = mkv->info.bc;
>> >
>> > -    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, 1000000);
>> > +    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, mkv->timecodescale);
>> >  if ((tag = av_dict_get(s->metadata, "title", NULL, 0)))
>> >  put_ebml_string(pb, MATROSKA_ID_TITLE, tag->value);
>> >  if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
>> > @@ -1927,12 +1929,12 @@ static int mkv_write_header(AVFormatContext *s)
>> >  // after 4k and on a keyframe
>> >  if (IS_SEEKABLE(pb, mkv)) {
>> >  if (mkv->cluster_time_limit < 0)
>> > -            mkv->cluster_time_limit = 5000;
>> > +            mkv->cluster_time_limit = 5*(1000000000/mkv->timecodescale);
>> >  if (mkv->cluster_size_limit < 0)
>> >  mkv->cluster_size_limit = 5 * 1024 * 1024;
>> >  } else {
>> >  if (mkv->cluster_time_limit < 0)
>> > -            mkv->cluster_time_limit = 1000;
>> > +            mkv->cluster_time_limit = 1*(1000000000/mkv->timecodescale);
>> >  if (mkv->cluster_size_limit < 0)
>> >  mkv->cluster_size_limit = 32 * 1024;
>> >  }
>> > @@ -2708,7 +2710,7 @@ static int mkv_init(struct AVFormatContext *s)
>> >  }
>> >
>> >  // ms precision is the de-facto standard timescale for mkv files
>> > -        avpriv_set_pts_info(st, 64, 1, 1000);
>> > +        avpriv_set_pts_info(st, 64, 1, 1000000000/mkv->timecodescale);
>> >
>> >  if (st->codecpar->codec_type == AVMEDIA_TYPE_ATTACHMENT) {
>> >  if (mkv->mode == MODE_WEBM) {
>> > @@ -2795,6 +2797,7 @@ static const AVOption options[] = {
>> >  { "infer", "For each track type, mark the first track of disposition
>> default as default; if none exists, mark the first track as default.", 0,
>> AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_INFER }, 0, 0, FLAGS,
>> "default_mode" },
>> >  { "infer_no_subs", "For each track type, mark the first track of
>> disposition default as default; for audio and video: if none exists, mark
>> the first track as default.", 0, AV_OPT_TYPE_CONST, { .i64 =
>> DEFAULT_MODE_INFER_NO_SUBS }, 0, 0, FLAGS, "default_mode" },
>> >  { "passthrough", "Use the disposition flag as-is", 0,
>> AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_PASSTHROUGH }, 0, 0, FLAGS,
>> "default_mode" },
>> > +    { "timecodescale", "Time code scale for all tracks in nanoseconds",
>> OFFSET(timecodescale), AV_OPT_TYPE_INT64, { .i64 = 1000000 }, 1,
>> 1000000000, FLAGS },
>> >  { NULL },
>> >  };
>> >
>>
>> This, x10000000000000000!
>> Can we make it 1ns by default? Or maybe autodetect the highest precision
>> timebase of all streams and use it?
>>
>
> The MKV spec seems to indicate a default value of ms. I did not want to
> change it in this patch.
>

The spec was written in 2001ish. Mobile phones didn't even exist back then,
I think, and internet video must have been 12fps mpeg2. 1ms didn't make
much sense back then, and it makes exactly no sense at at all right now
with phones regularly shooting at hundreds of frames per second.


>> Let's not keep generating garbage files with shitty rounding that players
>> have to work around to recover the original timestamp based on framerate
>> to reduce jitter (mpv does this!).
>>
>
> +1 on this, but I'm worry, we could break a lot of decoder which may not
> follow the correct timecodescale (Which we found out in our system a while
> back)
>

I think that's a bug in their code, and we should help them fix it by changing
the default.
It's kind of important, as nothing will change unless we start to generate files
with better precision by default. For decades the main complaint about
Matroska has been the ridiculously low timestamp precision most files have. Even
the spec authors agree. There were talks to add hacks to correct the precision
of the timestamps in a compatible way with buggy demuxers some years ago, but
nothing came of it.

I'd really like to not have to tell users "please enable this magical demuxer option
to not mess up high framerate files or induce jitter". I'd be so much better to say
"oh, your files don't play correctly? You can enable this magic option and remux the file,
and also report this to the authors so they can fix it.
The former is reversible and preserves timestamp precision. The latter is irreversible
without guesses.