diff mbox series

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

Message ID 20210113174623.1397914-1-tfoucu@gmail.com
State New
Headers show
Series [FFmpeg-devel] libavformat/matroskaenc.c: Add option to set timecodescale
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Thierry Foucu Jan. 13, 2021, 5:46 p.m. UTC
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(-)

Comments

Lynne Jan. 13, 2021, 6:30 p.m. UTC | #1
Jan 13, 2021, 18:46 by tfoucu@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?
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!).
Also WebM mode should disable this, because the spec is dumb and specifies
a 1ms precision for no good reason.
Thierry Foucu Jan. 13, 2021, 7:58 p.m. UTC | #2
HI Lynne

On Wed, Jan 13, 2021 at 10:30 AM Lynne <dev@lynne.ee> wrote:

> Jan 13, 2021, 18:46 by tfoucu@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.


> 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)


> Also WebM mode should disable this, because the spec is dumb and specifies
> a 1ms precision for no good reason.
>

If we keep the default value of ms, then this will not change any webm
unless a user decides to generate a Webm file and set this flag.
Do you want me to log an error saying that using this flag to set something
other than ms for webm is incorrect?


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Jan. 13, 2021, 8:02 p.m. UTC | #3
On 1/13/2021 2:46 PM, Thierry Foucu wrote:
> 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);

Either 1000000000LL, or use av_rescale(). Same for the two below

>           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 },
>   };

Does this cover all cases? A timecodescale of 1000000 was implied until 
now, so many parts of the code could be hardcoding it in a non obvious way.

I see for example MATROSKA_ID_DURATION is set as av_rescale(s->duration, 
1000, AV_TIME_BASE). Should that AV_TIME_BASE be changed to 
mkv->timecodescale? The spec says "Duration of the Segment in 
nanoseconds based on TimestampScale."
Thierry Foucu Jan. 13, 2021, 8:50 p.m. UTC | #4
a note with this change:
If we set the timecodescale to microsecond, and we encode a 30 fps video,
the duration of each frame are then 33333 us.
In this case,
(int16_t)cluster_time != cluster_time
Will almost every time faile and we will need to create a new block per
frame (it seems to me at least)
Because in the block header, there is a timestamp relative to Cluster
timestamp (signed int16) which cannot represent 33333

On Wed, Jan 13, 2021 at 12:02 PM James Almer <jamrial@gmail.com> wrote:

> On 1/13/2021 2:46 PM, Thierry Foucu wrote:
> > 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);
>
> Either 1000000000LL, or use av_rescale(). Same for the two below
>
> >           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 },
> >   };
>
> Does this cover all cases? A timecodescale of 1000000 was implied until
> now, so many parts of the code could be hardcoding it in a non obvious way.
>
> I see for example MATROSKA_ID_DURATION is set as av_rescale(s->duration,
> 1000, AV_TIME_BASE). Should that AV_TIME_BASE be changed to
> mkv->timecodescale? The spec says "Duration of the Segment in
> nanoseconds based on TimestampScale."
>

Interesting. I did not have to change this because if I check the Metadata
DURATION coming from the demuxer with a default ms and set to microsecond,
I do see in both:
DURATION        : 00:00:01.001000000

or is it another duration I need to look at, like this one:
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskaenc.c#L1863

See output:
# default behavoir
./ffmpeg -f lavfi -i "testsrc=r=30000/1001" -debug_ts -t 1 -y -f matroska
/tmp/test.mkv
./ffmpeg -i /tmp/test.mkv
ffmpeg version N-100413-g3799e77f93 Copyright (c) 2000-2020 the FFmpeg
developers
  built with gcc 10 (Debian 10.2.0-19)
  configuration: --disable-optimizations
  libavutil      56. 62.100 / 56. 62.100
  libavcodec     58.115.102 / 58.115.102
  libavformat    58. 65.100 / 58. 65.100
  libavdevice    58. 11.103 / 58. 11.103
  libavfilter     7. 93.100 /  7. 93.100
  libswscale      5.  8.100 /  5.  8.100
  libswresample   3.  8.100 /  3.  8.100
Input #0, matroska,webm, from '/tmp/test.mkv':
  Metadata:
    ENCODER         : Lavf58.65.100
  Duration: 00:00:01.00, start: 0.000000, bitrate: 412 kb/s
    Stream #0:0: Video: mpeg4 (Simple Profile), yuv420p(tv, progressive),
320x240 [SAR 1:1 DAR 4:3], 29.97 fps, 29.97 tbr, 1k tbn, 30k tbc (default)
    Metadata:
      ENCODER         : Lavc58.115.102 mpeg4
      DURATION        : 00:00:01.001000000

# set the timecodescale to microsecond
./ffmpeg -f lavfi -i "testsrc=r=30000/1001" -debug_ts -t 1 -timecodescale
1000 -y -f matroska /tmp/test_2.mkv
 ./ffmpeg -i /tmp/test_2.mkv
ffmpeg version N-100413-g3799e77f93 Copyright (c) 2000-2020 the FFmpeg
developers
  built with gcc 10 (Debian 10.2.0-19)
  configuration: --disable-optimizations
  libavutil      56. 62.100 / 56. 62.100
  libavcodec     58.115.102 / 58.115.102
  libavformat    58. 65.100 / 58. 65.100
  libavdevice    58. 11.103 / 58. 11.103
  libavfilter     7. 93.100 /  7. 93.100
  libswscale      5.  8.100 /  5.  8.100
  libswresample   3.  8.100 /  3.  8.100
Input #0, matroska,webm, from '/tmp/test_2.mkv':
  Metadata:
    ENCODER         : Lavf58.65.100
  Duration: 00:00:01.00, start: 0.000000, bitrate: 416 kb/s
    Stream #0:0: Video: mpeg4 (Simple Profile), yuv420p(tv, progressive),
320x240 [SAR 1:1 DAR 4:3], 29.97 fps, 29.97 tbr, 1000k tbn, 30k tbc
(default)
    Metadata:
      ENCODER         : Lavc58.115.102 mpeg4
      DURATION        : 00:00:01.001000000
James Almer Jan. 13, 2021, 9:21 p.m. UTC | #5
On 1/13/2021 5:50 PM, Thierry Foucu wrote:
> a note with this change:
> If we set the timecodescale to microsecond, and we encode a 30 fps video,
> the duration of each frame are then 33333 us.
> In this case,
> (int16_t)cluster_time != cluster_time
> Will almost every time faile and we will need to create a new block per
> frame (it seems to me at least)
> Because in the block header, there is a timestamp relative to Cluster
> timestamp (signed int16) which cannot represent 33333
> 
> On Wed, Jan 13, 2021 at 12:02 PM James Almer <jamrial@gmail.com> wrote:
> 
>> On 1/13/2021 2:46 PM, Thierry Foucu wrote:
>>> 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);
>>
>> Either 1000000000LL, or use av_rescale(). Same for the two below
>>
>>>            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 },
>>>    };
>>
>> Does this cover all cases? A timecodescale of 1000000 was implied until
>> now, so many parts of the code could be hardcoding it in a non obvious way.
>>
>> I see for example MATROSKA_ID_DURATION is set as av_rescale(s->duration,
>> 1000, AV_TIME_BASE). Should that AV_TIME_BASE be changed to
>> mkv->timecodescale? The spec says "Duration of the Segment in
>> nanoseconds based on TimestampScale."
>>
> 
> Interesting. I did not have to change this because if I check the Metadata
> DURATION coming from the demuxer with a default ms and set to microsecond,
> I do see in both:
> DURATION        : 00:00:01.001000000
> 
> or is it another duration I need to look at, like this one:
> https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskaenc.c#L1863

That and the one immediately after it are the ones i was talking about, 
yes. In any case, it seems to be a temporary value written to the output 
that will afterwards be overwritten in 
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2559 
when writing the trailer.

The commit that added the code in mkv_write_header() is 70c1647a350. I 
guess you could ensure the temp value written there is correct by 
forcing the muxing process to stop without calling mkv_write_trailer(), 
or commenting out the code writing the final duration value.

> 
> See output:
> # default behavoir
> ./ffmpeg -f lavfi -i "testsrc=r=30000/1001" -debug_ts -t 1 -y -f matroska
> /tmp/test.mkv
> ./ffmpeg -i /tmp/test.mkv
> ffmpeg version N-100413-g3799e77f93 Copyright (c) 2000-2020 the FFmpeg
> developers
>    built with gcc 10 (Debian 10.2.0-19)
>    configuration: --disable-optimizations
>    libavutil      56. 62.100 / 56. 62.100
>    libavcodec     58.115.102 / 58.115.102
>    libavformat    58. 65.100 / 58. 65.100
>    libavdevice    58. 11.103 / 58. 11.103
>    libavfilter     7. 93.100 /  7. 93.100
>    libswscale      5.  8.100 /  5.  8.100
>    libswresample   3.  8.100 /  3.  8.100
> Input #0, matroska,webm, from '/tmp/test.mkv':
>    Metadata:
>      ENCODER         : Lavf58.65.100
>    Duration: 00:00:01.00, start: 0.000000, bitrate: 412 kb/s
>      Stream #0:0: Video: mpeg4 (Simple Profile), yuv420p(tv, progressive),
> 320x240 [SAR 1:1 DAR 4:3], 29.97 fps, 29.97 tbr, 1k tbn, 30k tbc (default)
>      Metadata:
>        ENCODER         : Lavc58.115.102 mpeg4
>        DURATION        : 00:00:01.001000000
> 
> # set the timecodescale to microsecond
> ./ffmpeg -f lavfi -i "testsrc=r=30000/1001" -debug_ts -t 1 -timecodescale
> 1000 -y -f matroska /tmp/test_2.mkv
>   ./ffmpeg -i /tmp/test_2.mkv
> ffmpeg version N-100413-g3799e77f93 Copyright (c) 2000-2020 the FFmpeg
> developers
>    built with gcc 10 (Debian 10.2.0-19)
>    configuration: --disable-optimizations
>    libavutil      56. 62.100 / 56. 62.100
>    libavcodec     58.115.102 / 58.115.102
>    libavformat    58. 65.100 / 58. 65.100
>    libavdevice    58. 11.103 / 58. 11.103
>    libavfilter     7. 93.100 /  7. 93.100
>    libswscale      5.  8.100 /  5.  8.100
>    libswresample   3.  8.100 /  3.  8.100
> Input #0, matroska,webm, from '/tmp/test_2.mkv':
>    Metadata:
>      ENCODER         : Lavf58.65.100
>    Duration: 00:00:01.00, start: 0.000000, bitrate: 416 kb/s
>      Stream #0:0: Video: mpeg4 (Simple Profile), yuv420p(tv, progressive),
> 320x240 [SAR 1:1 DAR 4:3], 29.97 fps, 29.97 tbr, 1000k tbn, 30k tbc
> (default)
>      Metadata:
>        ENCODER         : Lavc58.115.102 mpeg4
>        DURATION        : 00:00:01.001000000
> 
> 
> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> 
>
Lynne Jan. 13, 2021, 9:46 p.m. UTC | #6
Jan 13, 2021, 20:58 by tfoucu@gmail.com:

> HI Lynne
>
> On Wed, Jan 13, 2021 at 10:30 AM Lynne <> dev@lynne.ee> > wrote:
>
>> Jan 13, 2021, 18:46 by >> tfoucu@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.
James Almer Jan. 13, 2021, 9:58 p.m. UTC | #7
On 1/13/2021 4:58 PM, Thierry Foucu wrote:
> HI Lynne
> 
> On Wed, Jan 13, 2021 at 10:30 AM Lynne <dev@lynne.ee> wrote:
> 
>> Jan 13, 2021, 18:46 by tfoucu@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.
> 
> 
>> 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)
> 
> 
>> Also WebM mode should disable this, because the spec is dumb and specifies
>> a 1ms precision for no good reason.
>>
> 
> If we keep the default value of ms, then this will not change any webm
> unless a user decides to generate a Webm file and set this flag.
> Do you want me to log an error saying that using this flag to set something
> other than ms for webm is incorrect?

The webm spec states "The TimecodeScale element SHOULD be set to a 
default of 1.000.000 nanoseconds.", so it's not forbidden per se.
Does libwebm handle files with a different TimecodeScale than the 
default? If so, then it's fine. Otherwise, I agree we should probably 
error out.

> 
> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> 
>
diff mbox series

Patch

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 },
 };