[FFmpeg-devel] avformat/matroskaenc: Allow changing the time stamp precision via option

From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>

Adds "timestamp_precision" to the available option for Matroska/WebM
muxing. The option enables users and developers to change the precision
of the time stamps in the Matroska/WebM container up to 1 nanosecond,
which can aid with the proper detection of constant and variable rate
content.

Work-around fix for: 259, 6406, 7927, 8909 and 9124.

Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
---
 doc/muxers.texi           |  8 ++++++++
 libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 8 deletions(-)

On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
> 
> Adds "timestamp_precision" to the available option for Matroska/WebM
> muxing. The option enables users and developers to change the precision
> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
> which can aid with the proper detection of constant and variable rate
> content.
> 
> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
> 
> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
> ---
>   doc/muxers.texi           |  8 ++++++++
>   libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>   2 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/doc/muxers.texi b/doc/muxers.texi
> index e1c6ad0829..d9f7badae1 100644
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>   which has to be done manually beforehand, e.g. by using the vflip filter.
>   Default is @var{false} and indicates bitmap is stored top down.
>   
> +@item timestamp_precision
> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
> +improve detection of constant rate content in demuxers. Note that some poorly
> +implemented demuxers may require a timestamp precision of 1 millisecond, so
> +increasing it past that point may result in playback issues. Higher precision
> +also reduces the maximum possible timestamp significantly.
> +Default is @var{1/1000000000} (1 nanosecond).

Like i said, the default must be the one defined in the spec. This 
version not only would need FATE test updates, it also like you 
described makes all new files by default have a lot of overhead from 
having one block per cluster.

I'll apply the previous version later if no one has any comment about 
the implementation, or the option name (I'd prefer timestamp_scale 
instead, following the element name).

> +
>   @end table
>   
>   @anchor{md5}
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 186a25d920..bff8c8e7f2 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -20,6 +20,7 @@
>    */
>   
>   #include <stdint.h>
> +#include <float.h>
>   
>   #include "av1.h"
>   #include "avc.h"
> @@ -158,6 +159,8 @@ typedef struct MatroskaMuxContext {
>       int                 default_mode;
>   
>       uint32_t            segment_uid[4];
> +
> +    AVRational          time_base;
>   } MatroskaMuxContext;
>   
>   /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
> @@ -1814,6 +1817,7 @@ static int mkv_write_header(AVFormatContext *s)
>       const AVDictionaryEntry *tag;
>       int ret, i, version = 2;
>       int64_t creation_time;
> +    int64_t time_base = 1;
>   
>       if (mkv->mode != MODE_WEBM ||
>           av_dict_get(s->metadata, "stereo_mode", NULL, 0) ||
> @@ -1850,7 +1854,10 @@ static int mkv_write_header(AVFormatContext *s)
>           return ret;
>       pb = mkv->info.bc;
>   
> -    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, 1000000);
> +    time_base = av_rescale_q(time_base, mkv->time_base, (AVRational){1, 1000000000});
> +    av_log(s, AV_LOG_DEBUG, "TimestampScale is: %" PRId64 " ns\n", time_base);
> +    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, time_base);
> +
>       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)) {
> @@ -1883,11 +1890,11 @@ static int mkv_write_header(AVFormatContext *s)
>           int64_t metadata_duration = get_metadata_duration(s);
>   
>           if (s->duration > 0) {
> -            int64_t scaledDuration = av_rescale(s->duration, 1000, AV_TIME_BASE);
> +            int64_t scaledDuration = av_rescale_q(s->duration, AV_TIME_BASE_Q, mkv->time_base);
>               put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>               av_log(s, AV_LOG_DEBUG, "Write early duration from recording time = %" PRIu64 "\n", scaledDuration);
>           } else if (metadata_duration > 0) {
> -            int64_t scaledDuration = av_rescale(metadata_duration, 1000, AV_TIME_BASE);
> +            int64_t scaledDuration = av_rescale_q(metadata_duration, AV_TIME_BASE_Q, mkv->time_base);
>               put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>               av_log(s, AV_LOG_DEBUG, "Write early duration from metadata = %" PRIu64 "\n", scaledDuration);
>           } else if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
> @@ -1948,12 +1955,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 = av_rescale_q(5000, (AVRational){1, 1000}, mkv->time_base);
>           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 = av_rescale_q(1000, (AVRational){1, 1000}, mkv->time_base);
>           if (mkv->cluster_size_limit < 0)
>               mkv->cluster_size_limit = 32 * 1024;
>       }
> @@ -2323,7 +2330,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt)
>               ret = mkv_end_cluster(s);
>               if (ret < 0)
>                   return ret;
> -            av_log(s, AV_LOG_WARNING, "Starting new cluster due to timestamp\n");
> +            av_log(s, AV_LOG_VERBOSE, "Starting new cluster as timestamp offset exceeds signed 16-bit integer range.\n");
>           }
>       }
>   
> @@ -2738,8 +2745,8 @@ static int mkv_init(struct AVFormatContext *s)
>               track->uid = mkv_get_uid(mkv->tracks, i, &c);
>           }
>   
> -        // ms precision is the de-facto standard timescale for mkv files
> -        avpriv_set_pts_info(st, 64, 1, 1000);
> +        // Use user-defined timescale.
> +        avpriv_set_pts_info(st, 64, mkv->time_base.num, mkv->time_base.den);
>   
>           if (st->codecpar->codec_type == AVMEDIA_TYPE_ATTACHMENT) {
>               if (mkv->mode == MODE_WEBM) {
> @@ -2759,6 +2766,10 @@ static int mkv_init(struct AVFormatContext *s)
>           track->track_num_size = ebml_num_size(track->track_num);
>       }
>   
> +    // Scale the configured cluster_time_limit.
> +    if (mkv->cluster_time_limit >= 0)
> +        mkv->cluster_time_limit = av_rescale_q(mkv->cluster_time_limit, (AVRational){1, 1000}, mkv->time_base);
> +
>       if (mkv->is_dash && nb_tracks != 1)
>           return AVERROR(EINVAL);
>   
> @@ -2826,6 +2837,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" },
> +    { "timestamp_precision", "Timestamp precision to use for the entire container", OFFSET(time_base), AV_OPT_TYPE_RATIONAL, { .dbl = 0.000000001 }, 0.000000001, DBL_MAX, FLAGS},
>       { NULL },
>   };
>   
>

On 2021-05-20 19:18, michael.dirks@xaymar.com wrote:
> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>
> Adds "timestamp_precision" to the available option for Matroska/WebM
> muxing. The option enables users and developers to change the precision
> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
> which can aid with the proper detection of constant and variable rate
> content.
>
> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>
> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
> ---
>   doc/muxers.texi           |  8 ++++++++
>   libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>   2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/doc/muxers.texi b/doc/muxers.texi
> index e1c6ad0829..d9f7badae1 100644
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>   which has to be done manually beforehand, e.g. by using the vflip filter.
>   Default is @var{false} and indicates bitmap is stored top down.
>   
> +@item timestamp_precision
> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
> +improve detection of constant rate content in demuxers. Note that some poorly
> +implemented demuxers may require a timestamp precision of 1 millisecond, so
> +increasing it past that point may result in playback issues. Higher precision
> +also reduces the maximum possible timestamp significantly.
> +Default is @var{1/1000000000} (1 nanosecond).
> +
>   @end table
>   
>   @anchor{md5}
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 186a25d920..bff8c8e7f2 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -20,6 +20,7 @@
>    */
>   
>   #include <stdint.h>
> +#include <float.h>
>   
>   #include "av1.h"
>   #include "avc.h"
> @@ -158,6 +159,8 @@ typedef struct MatroskaMuxContext {
>       int                 default_mode;
>   
>       uint32_t            segment_uid[4];
> +
> +    AVRational          time_base;
>   } MatroskaMuxContext;
>   
>   /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
> @@ -1814,6 +1817,7 @@ static int mkv_write_header(AVFormatContext *s)
>       const AVDictionaryEntry *tag;
>       int ret, i, version = 2;
>       int64_t creation_time;
> +    int64_t time_base = 1;
>   
>       if (mkv->mode != MODE_WEBM ||
>           av_dict_get(s->metadata, "stereo_mode", NULL, 0) ||
> @@ -1850,7 +1854,10 @@ static int mkv_write_header(AVFormatContext *s)
>           return ret;
>       pb = mkv->info.bc;
>   
> -    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, 1000000);
> +    time_base = av_rescale_q(time_base, mkv->time_base, (AVRational){1, 1000000000});
> +    av_log(s, AV_LOG_DEBUG, "TimestampScale is: %" PRId64 " ns\n", time_base);
> +    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, time_base);
> +
>       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)) {
> @@ -1883,11 +1890,11 @@ static int mkv_write_header(AVFormatContext *s)
>           int64_t metadata_duration = get_metadata_duration(s);
>   
>           if (s->duration > 0) {
> -            int64_t scaledDuration = av_rescale(s->duration, 1000, AV_TIME_BASE);
> +            int64_t scaledDuration = av_rescale_q(s->duration, AV_TIME_BASE_Q, mkv->time_base);
>               put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>               av_log(s, AV_LOG_DEBUG, "Write early duration from recording time = %" PRIu64 "\n", scaledDuration);
>           } else if (metadata_duration > 0) {
> -            int64_t scaledDuration = av_rescale(metadata_duration, 1000, AV_TIME_BASE);
> +            int64_t scaledDuration = av_rescale_q(metadata_duration, AV_TIME_BASE_Q, mkv->time_base);
>               put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>               av_log(s, AV_LOG_DEBUG, "Write early duration from metadata = %" PRIu64 "\n", scaledDuration);
>           } else if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
> @@ -1948,12 +1955,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 = av_rescale_q(5000, (AVRational){1, 1000}, mkv->time_base);
>           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 = av_rescale_q(1000, (AVRational){1, 1000}, mkv->time_base);
>           if (mkv->cluster_size_limit < 0)
>               mkv->cluster_size_limit = 32 * 1024;
>       }
> @@ -2323,7 +2330,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt)
>               ret = mkv_end_cluster(s);
>               if (ret < 0)
>                   return ret;
> -            av_log(s, AV_LOG_WARNING, "Starting new cluster due to timestamp\n");
> +            av_log(s, AV_LOG_VERBOSE, "Starting new cluster as timestamp offset exceeds signed 16-bit integer range.\n");
>           }
>       }
>   
> @@ -2738,8 +2745,8 @@ static int mkv_init(struct AVFormatContext *s)
>               track->uid = mkv_get_uid(mkv->tracks, i, &c);
>           }
>   
> -        // ms precision is the de-facto standard timescale for mkv files
> -        avpriv_set_pts_info(st, 64, 1, 1000);
> +        // Use user-defined timescale.
> +        avpriv_set_pts_info(st, 64, mkv->time_base.num, mkv->time_base.den);
>   
>           if (st->codecpar->codec_type == AVMEDIA_TYPE_ATTACHMENT) {
>               if (mkv->mode == MODE_WEBM) {
> @@ -2759,6 +2766,10 @@ static int mkv_init(struct AVFormatContext *s)
>           track->track_num_size = ebml_num_size(track->track_num);
>       }
>   
> +    // Scale the configured cluster_time_limit.
> +    if (mkv->cluster_time_limit >= 0)
> +        mkv->cluster_time_limit = av_rescale_q(mkv->cluster_time_limit, (AVRational){1, 1000}, mkv->time_base);
> +
>       if (mkv->is_dash && nb_tracks != 1)
>           return AVERROR(EINVAL);
>   
> @@ -2826,6 +2837,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" },
> +    { "timestamp_precision", "Timestamp precision to use for the entire container", OFFSET(time_base), AV_OPT_TYPE_RATIONAL, { .dbl = 0.000000001 }, 0.000000001, DBL_MAX, FLAGS},
>       { NULL },
>   };

This should have all the requested changes, including the update to documentation. Unfortunately I was not able to get rid of the new cluster spam, so I moved it down a few logging levels into "verbose". While I was at it, I also extended the positive range to an infinitely large number, as I found no valid reason to arbitrarily limit it when the format itself supports and positive unsigned 64-bit integer for the time stamp scale.

This should hopefully be the last change to the patch.

Michael Fabian 'Xaymar' Dirks (12021-05-20):
> Unfortunately I was not able to get rid of the new cluster spam, so I
> moved it down a few logging levels into "verbose".

So you silenced the warning, but did not fix the issue it was warning
about.

If you were a physician, you would just have prescribed morphine for a
broken leg: hide the pain, leave the fracture, likely to worsen.

Regards,

On 2021-05-20 19:38, Nicolas George wrote:
> Michael Fabian 'Xaymar' Dirks (12021-05-20):
>> Unfortunately I was not able to get rid of the new cluster spam, so I
>> moved it down a few logging levels into "verbose".
> So you silenced the warning, but did not fix the issue it was warning
> about.
>
> If you were a physician, you would just have prescribed morphine for a
> broken leg: hide the pain, leave the fracture, likely to worsen.
>
> Regards,
>
>
> _______________________________________________
> 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".

The issue can't be fixed, as it is a problem with the Matroska specification only allowing signed 16-bit integer offsets for the block timestamps. Your analogy is not only wrong, but also misplaced here, if you want to voice your distaste for the Matroska limitations, the IETF standardization process is the go-to:

- https://github.com/ietf-wg-cellar/matroska-specification/issues/422
- https://github.com/ietf-wg-cellar/matroska-specification/pull/425
- https://github.com/ietf-wg-cellar/matroska-specification/pull/437

Michael Fabian 'Xaymar' Dirks (12021-05-20):
> The issue can't be fixed

Then you should not be hiding the warning. It means something.

I have yet to see a convincing explanation of a case where 1µs would not
be enough, by the way. AFAICS, the issue is mostly people handling
rounding incorrectly; changing the time base would not fix the code that
does improper rounding, it will just let the rounding errors accumulate
slower. Fix the code, and there will be no more rounding errors.

Please don't quote mailing-list footers.

Regards,

On 2021-05-20 19:26, James Almer wrote:
> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>
>> Adds "timestamp_precision" to the available option for Matroska/WebM
>> muxing. The option enables users and developers to change the precision
>> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
>> which can aid with the proper detection of constant and variable rate
>> content.
>>
>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>
>> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>> ---
>>   doc/muxers.texi           |  8 ++++++++
>>   libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>   2 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>> index e1c6ad0829..d9f7badae1 100644
>> --- a/doc/muxers.texi
>> +++ b/doc/muxers.texi
>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>>   which has to be done manually beforehand, e.g. by using the vflip filter.
>>   Default is @var{false} and indicates bitmap is stored top down.
>>   +@item timestamp_precision
>> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
>> +improve detection of constant rate content in demuxers. Note that some poorly
>> +implemented demuxers may require a timestamp precision of 1 millisecond, so
>> +increasing it past that point may result in playback issues. Higher precision
>> +also reduces the maximum possible timestamp significantly.
>> +Default is @var{1/1000000000} (1 nanosecond).
>
> Like i said, the default must be the one defined in the spec. This version not only would need FATE test updates, it also like you described makes all new files by default have a lot of overhead from having one block per cluster.

I am aware of what you said and I am also aware of Lynne said. I do not know who has the actual final say in this, all I know is that the maintainers for matroskaenc.c are "David Conrad" and "Andreas Rheinhardt" - both of which have not commented on this yet.

>
> I'll apply the previous version later if no one has any comment about the implementation, or the option name (I'd prefer timestamp_scale instead, following the element name).

I chose "timestamp_precision" over "timestamp_scale", as the latter has different meanings when read. "timestamp_precision" directly implies that the option modifies how accurate the time stamps will be, while "timestamp_scale" has no such meaning to readers.

>
>> +
>>   @end table
>>     @anchor{md5}
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 186a25d920..bff8c8e7f2 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -20,6 +20,7 @@
>>    */
>>     #include <stdint.h>
>> +#include <float.h>
>>     #include "av1.h"
>>   #include "avc.h"
>> @@ -158,6 +159,8 @@ typedef struct MatroskaMuxContext {
>>       int                 default_mode;
>>         uint32_t            segment_uid[4];
>> +
>> +    AVRational          time_base;
>>   } MatroskaMuxContext;
>>     /** 2 bytes * 7 for EBML IDs, 7 1-byte EBML lengths, 6 1-byte uint,
>> @@ -1814,6 +1817,7 @@ static int mkv_write_header(AVFormatContext *s)
>>       const AVDictionaryEntry *tag;
>>       int ret, i, version = 2;
>>       int64_t creation_time;
>> +    int64_t time_base = 1;
>>         if (mkv->mode != MODE_WEBM ||
>>           av_dict_get(s->metadata, "stereo_mode", NULL, 0) ||
>> @@ -1850,7 +1854,10 @@ static int mkv_write_header(AVFormatContext *s)
>>           return ret;
>>       pb = mkv->info.bc;
>>   -    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, 1000000);
>> +    time_base = av_rescale_q(time_base, mkv->time_base, (AVRational){1, 1000000000});
>> +    av_log(s, AV_LOG_DEBUG, "TimestampScale is: %" PRId64 " ns\n", time_base);
>> +    put_ebml_uint(pb, MATROSKA_ID_TIMECODESCALE, time_base);
>> +
>>       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)) {
>> @@ -1883,11 +1890,11 @@ static int mkv_write_header(AVFormatContext *s)
>>           int64_t metadata_duration = get_metadata_duration(s);
>>             if (s->duration > 0) {
>> -            int64_t scaledDuration = av_rescale(s->duration, 1000, AV_TIME_BASE);
>> +            int64_t scaledDuration = av_rescale_q(s->duration, AV_TIME_BASE_Q, mkv->time_base);
>>               put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>>               av_log(s, AV_LOG_DEBUG, "Write early duration from recording time = %" PRIu64 "\n", scaledDuration);
>>           } else if (metadata_duration > 0) {
>> -            int64_t scaledDuration = av_rescale(metadata_duration, 1000, AV_TIME_BASE);
>> +            int64_t scaledDuration = av_rescale_q(metadata_duration, AV_TIME_BASE_Q, mkv->time_base);
>>               put_ebml_float(pb, MATROSKA_ID_DURATION, scaledDuration);
>>               av_log(s, AV_LOG_DEBUG, "Write early duration from metadata = %" PRIu64 "\n", scaledDuration);
>>           } else if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
>> @@ -1948,12 +1955,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 = av_rescale_q(5000, (AVRational){1, 1000}, mkv->time_base);
>>           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 = av_rescale_q(1000, (AVRational){1, 1000}, mkv->time_base);
>>           if (mkv->cluster_size_limit < 0)
>>               mkv->cluster_size_limit = 32 * 1024;
>>       }
>> @@ -2323,7 +2330,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt)
>>               ret = mkv_end_cluster(s);
>>               if (ret < 0)
>>                   return ret;
>> -            av_log(s, AV_LOG_WARNING, "Starting new cluster due to timestamp\n");
>> +            av_log(s, AV_LOG_VERBOSE, "Starting new cluster as timestamp offset exceeds signed 16-bit integer range.\n");
>>           }
>>       }
>>   @@ -2738,8 +2745,8 @@ static int mkv_init(struct AVFormatContext *s)
>>               track->uid = mkv_get_uid(mkv->tracks, i, &c);
>>           }
>>   -        // ms precision is the de-facto standard timescale for mkv files
>> -        avpriv_set_pts_info(st, 64, 1, 1000);
>> +        // Use user-defined timescale.
>> +        avpriv_set_pts_info(st, 64, mkv->time_base.num, mkv->time_base.den);
>>             if (st->codecpar->codec_type == AVMEDIA_TYPE_ATTACHMENT) {
>>               if (mkv->mode == MODE_WEBM) {
>> @@ -2759,6 +2766,10 @@ static int mkv_init(struct AVFormatContext *s)
>>           track->track_num_size = ebml_num_size(track->track_num);
>>       }
>>   +    // Scale the configured cluster_time_limit.
>> +    if (mkv->cluster_time_limit >= 0)
>> +        mkv->cluster_time_limit = av_rescale_q(mkv->cluster_time_limit, (AVRational){1, 1000}, mkv->time_base);
>> +
>>       if (mkv->is_dash && nb_tracks != 1)
>>           return AVERROR(EINVAL);
>>   @@ -2826,6 +2837,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" },
>> +    { "timestamp_precision", "Timestamp precision to use for the entire container", OFFSET(time_base), AV_OPT_TYPE_RATIONAL, { .dbl = 0.000000001 }, 0.000000001, DBL_MAX, FLAGS},
>>       { NULL },
>>   };
>>

On 2021-05-20 19:59, Nicolas George wrote:
> Michael Fabian 'Xaymar' Dirks (12021-05-20):
>> The issue can't be fixed
> Then you should not be hiding the warning. It means something.
I was asked to fix or remove it. Removing it breaks Matroska files entirely. Fixing it is not possible. What other option do I have when I'm not in control of the Matroska specification, yet somehow am supposed to fix the Matroska specification? I'm confused.
> I have yet to see a convincing explanation of a case where 1µs would not
> be enough, by the way. AFAICS, the issue is mostly people handling
> rounding incorrectly; changing the time base would not fix the code that
> does improper rounding, it will just let the rounding errors accumulate
> slower. Fix the code, and there will be no more rounding errors.
The default precision for Matroska is 1ms not 1µs. 1µs is plenty enough, but 1ms is not.

On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
> On 2021-05-20 19:26, James Almer wrote:
>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>
>>> Adds "timestamp_precision" to the available option for Matroska/WebM
>>> muxing. The option enables users and developers to change the precision
>>> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
>>> which can aid with the proper detection of constant and variable rate
>>> content.
>>>
>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>
>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>> ---
>>>   doc/muxers.texi           |  8 ++++++++
>>>   libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>   2 files changed, 28 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>> index e1c6ad0829..d9f7badae1 100644
>>> --- a/doc/muxers.texi
>>> +++ b/doc/muxers.texi
>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this 
>>> option does not flip the bitmap
>>>   which has to be done manually beforehand, e.g. by using the vflip 
>>> filter.
>>>   Default is @var{false} and indicates bitmap is stored top down.
>>>   +@item timestamp_precision
>>> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, 
>>> which can
>>> +improve detection of constant rate content in demuxers. Note that 
>>> some poorly
>>> +implemented demuxers may require a timestamp precision of 1 
>>> millisecond, so
>>> +increasing it past that point may result in playback issues. Higher 
>>> precision
>>> +also reduces the maximum possible timestamp significantly.
>>> +Default is @var{1/1000000000} (1 nanosecond).
>>
>> Like i said, the default must be the one defined in the spec. This 
>> version not only would need FATE test updates, it also like you 
>> described makes all new files by default have a lot of overhead from 
>> having one block per cluster.
> 
> I am aware of what you said and I am also aware of Lynne said. I do not 
> know who has the actual final say in this, all I know is that the 
> maintainers for matroskaenc.c are "David Conrad" and "Andreas 
> Rheinhardt" - both of which have not commented on this yet.

Lynne agreed on IRC that it can remain as 1ms. Probably should have said 
it here, but i guess neither him or i thought you'd send a new version. 
Sorry for the confusion.

> 
>>
>> I'll apply the previous version later if no one has any comment about 
>> the implementation, or the option name (I'd prefer timestamp_scale 
>> instead, following the element name).
> 
> I chose "timestamp_precision" over "timestamp_scale", as the latter has 
> different meanings when read. "timestamp_precision" directly implies 
> that the option modifies how accurate the time stamps will be, while 
> "timestamp_scale" has no such meaning to readers.

The option name can be timestamp_scale whereas the description can 
remain as you wrote it. But i don't have a strong opinion about it.

Michael Fabian 'Xaymar' Dirks (12021-05-20):
> The default precision for Matroska is 1ms not 1µs. 1µs is plenty
> enough, but 1ms is not.

I am not even convinced that most problems with 1ms are not caused by
bad math, but I am willing to see proof.

Regards,

On 5/20/2021 2:59 PM, Nicolas George wrote:
> Michael Fabian 'Xaymar' Dirks (12021-05-20):
>> The issue can't be fixed
> 
> Then you should not be hiding the warning. It means something.

It means that the container does not support values bigger than 
INT16_MAX for block timestamps (relative to the absolute timestamp of 
the start of the cluster), and it warns as such when it can't write one 
and needs to instead start a new cluster to write the current packet.
With a big timescale value like 1µs or 1ns, the muxer can't write the 
requested amount of blocks per cluster (5 seconds or 5mb by default), 
resulting in extreme cases like one block per cluster.

The default value for timescale should not trigger this code and warning 
when in combination with the default cluster size/duration values.
Making 1ns or 1µs the default timescale is definitely not a good idea in 
the current state of the Matroska spec.

> 
> I have yet to see a convincing explanation of a case where 1µs would not
> be enough, by the way. AFAICS, the issue is mostly people handling
> rounding incorrectly; changing the time base would not fix the code that
> does improper rounding, it will just let the rounding errors accumulate
> slower. Fix the code, and there will be no more rounding errors.
> 
> Please don't quote mailing-list footers.
> 
> Regards,
> 
> 
> _______________________________________________
> 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".
>

On 2021-05-20 20:14, Nicolas George wrote:
> Michael Fabian 'Xaymar' Dirks (12021-05-20):
>> The default precision for Matroska is 1ms not 1µs. 1µs is plenty
>> enough, but 1ms is not.
> I am not even convinced that most problems with 1ms are not caused by
> bad math, but I am willing to see proof.

Well, it's a combination of many things really, not bad math itself. 1ms works perfectly for the frame rates of 1, 2, 4, 5, 8, 10, 20, 25, 40, 50, 100, 125, 200, 250, 500, and 1000. For any other rate, you will have to look at the actual timestamp of N clusters and M blocks, where N and M are undefined. N and M are undefined because there is no solution that works for every rate, other than having a per-track rational time base (https://github.com/ietf-wg-cellar/matroska-specification/pull/425).

If there were zero dropped/skipped frames in the encoding process (we have a packet for every frame), then it can be worked around by looking ahead for 1 second, but that is a lot to ask from an application just to know the actual rate. Matroska is far from perfect as it is right now, so more vocal input on the IETF standardization process for Matroska/WebM might be helpful.

Michael Fabian 'Xaymar' Dirks (12021-05-20):
> Well, it's a combination of many things really, not bad math itself.
> 1ms works perfectly for the frame rates of 1, 2, 4, 5, 8, 10, 20, 25,
> 40, 50, 100, 125, 200, 250, 500, and 1000. For any other rate, you
> will have to look at the actual timestamp of N clusters and M blocks,
> where N and M are undefined. N and M are undefined because there is no
> solution that works for every rate, other than having a per-track
> rational time base
> (https://github.com/ietf-wg-cellar/matroska-specification/pull/425).
> 
> If there were zero dropped/skipped frames in the encoding process (we
> have a packet for every frame), then it can be worked around by
> looking ahead for 1 second, but that is a lot to ask from an
> application just to know the actual rate.

Why would an application need to know the actual frame rate with so much
precision?

> Matroska is far from perfect as it is right now, so more vocal input
> on the IETF standardization process for Matroska/WebM might be
> helpful.

To be really helpful, it is necessary to understand the issue properly.
In my experience, most people who complain about the precision of
timestamps are using them wrong.

Regards,

On 2021-05-20 20:23, James Almer wrote:
> On 5/20/2021 2:59 PM, Nicolas George wrote:
>> Michael Fabian 'Xaymar' Dirks (12021-05-20):
>>> The issue can't be fixed
>>
>> Then you should not be hiding the warning. It means something.
>
> It means that the container does not support values bigger than INT16_MAX for block timestamps (relative to the absolute timestamp of the start of the cluster), and it warns as such when it can't write one and needs to instead start a new cluster to write the current packet.
> With a big timescale value like 1µs or 1ns, the muxer can't write the requested amount of blocks per cluster (5 seconds or 5mb by default), resulting in extreme cases like one block per cluster.
I'm not sure if it's necessary as a warning, or should just be considered verbose messaging. I'm leaning to the latter, as it's more of an informational thing than "hey this is going to end up badly 99% of the time" kind of thing. At least in my testing with VLC, MPC-HC, Firefox, and an LG sound system from 2014, none of them actually had any issue playing it back.
>
> The default value for timescale should not trigger this code and warning when in combination with the default cluster size/duration values.
> Making 1ns or 1µs the default timescale is definitely not a good idea in the current state of the Matroska spec.
>
>>
>> I have yet to see a convincing explanation of a case where 1µs would not
>> be enough, by the way. AFAICS, the issue is mostly people handling
>> rounding incorrectly; changing the time base would not fix the code that
>> does improper rounding, it will just let the rounding errors accumulate
>> slower. Fix the code, and there will be no more rounding errors.
>>
>> Please don't quote mailing-list footers.
>>
>> Regards,

On 2021-05-20 20:54, Nicolas George wrote:
> Michael Fabian 'Xaymar' Dirks (12021-05-20):
>> Well, it's a combination of many things really, not bad math itself.
>> 1ms works perfectly for the frame rates of 1, 2, 4, 5, 8, 10, 20, 25,
>> 40, 50, 100, 125, 200, 250, 500, and 1000. For any other rate, you
>> will have to look at the actual timestamp of N clusters and M blocks,
>> where N and M are undefined. N and M are undefined because there is no
>> solution that works for every rate, other than having a per-track
>> rational time base
>> (https://github.com/ietf-wg-cellar/matroska-specification/pull/425).
>>
>> If there were zero dropped/skipped frames in the encoding process (we
>> have a packet for every frame), then it can be worked around by
>> looking ahead for 1 second, but that is a lot to ask from an
>> application just to know the actual rate.
> Why would an application need to know the actual frame rate with so much
> precision?

There are plenty of reasons to know the actual frame rate with high precision. Editing software needs to know this to initialize effects, timelines and projects. Players need it to queue the right amount of frames at the right time. Transcoding requires it to encode at the right rate. Remuxing requires it so that it doesn't end up as "variable" without requiring extensive extra parameters and scripting to get FFmpeg to like the MKV file. I don't see a single reason to not offer users the choice to increase the precision to their liking.

Hopefully in the future Matroska adopts rational time stamps for the tracks, in order to permanently fix the issue.

>> Matroska is far from perfect as it is right now, so more vocal input
>> on the IETF standardization process for Matroska/WebM might be
>> helpful.
> To be really helpful, it is necessary to understand the issue properly.
> In my experience, most people who complain about the precision of
> timestamps are using them wrong.

I have yet to see anyone use timestamps wrong. Timestamps should be as accurate as possible, especially when you consider that even with 1µs of precision in a 64-bit timestamp, you can still mux for at least 584000 years before you have to wrap around to zero again.

Michael Fabian 'Xaymar' Dirks (12021-05-20):
> There are plenty of reasons to know the actual frame rate with high
> precision.
> Editing software needs to know this to initialize effects, timelines
> and projects.

I doubt they do that without building an index in the first place.

> Players need it to queue the right amount of frames at the right time.

No, they need to queue _enough_ frames, and for that timestamps are
enough.

> Transcoding requires it to encode at the right rate.

Encoding mostly does not care about rate.

> Remuxing requires it so that it doesn't end up as "variable" without

Why would it be a problem?

> requiring extensive extra parameters and scripting to get FFmpeg to
> like the MKV file.

Please be more specific.

> I don't see a single reason to not offer users the choice to increase
> the precision to their liking.

I see no reason not to. But I also see no reason users should do it, and
you have not convinced me.

> I have yet to see anyone use timestamps wrong.

I suggest you spend a little time doing support on the users
mailing-list and you will see plenty.

> Timestamps should be as accurate as possible, especially when you
> consider that even with 1µs of precision in a 64-bit timestamp, you
> can still mux for at least 584000 years before you have to wrap around
> to zero again.

You need to learn about the difference between accuracy and precision.

Regards,

On 2021-05-20 21:20, Nicolas George wrote:
> Michael Fabian 'Xaymar' Dirks (12021-05-20):
>> There are plenty of reasons to know the actual frame rate with high
>> precision.
>> Editing software needs to know this to initialize effects, timelines
>> and projects.
> I doubt they do that without building an index in the first place.

At the current time there aren't many editors that support Matroska, but Black Magic Design DaVinci Resolve is one of them. Resolve approximates the 60 fps footage to be 59.94 fps, which is close but not exact - off by just 0.06 seconds. This is enough to create an audio drift of 1 second after just 16.67 seconds, as changing the "source" frame rate apparently means speeding up the footage.

You could consider this a "bug", but is this really a bug or is this just terrible time stamp support in Matroska? In my opinion it is the latter.

>> Players need it to queue the right amount of frames at the right time.
> No, they need to queue _enough_ frames, and for that timestamps are
> enough.

You probably have a different idea of a audio-video player than i do.

>> Transcoding requires it to encode at the right rate.
> Encoding mostly does not care about rate.

libx264, nvenc, amf, qsv, and probably plenty of other encoders disagree, as they base their bitrate decisions on how much bitrate is available at the current time.

>> Remuxing requires it so that it doesn't end up as "variable" without
> Why would it be a problem?
>
>> requiring extensive extra parameters and scripting to get FFmpeg to
>> like the MKV file.
> Please be more specific.

I think the next lines explain enough:

ffmpeg -i "raw60fps.y4m" -c:v libx264 (other parameters) -o "a.mkv"
ffmpeg -i "a.mkv" -c copy "a.mp4"

FFmpeg generates an mp4 file that is variable, not constant. This happens with every MKV file muxed as 1ms, as Matroska so far has no proper frame rate tags or rational per-track time bases. "-vsync cfr" does not fix this issue. "-framerate 60" does not fix this issue. None of the supposed workarounds are true fixes for the issue. This isn't a fix for the issue either, but it increases the precision enough to not require either of those parameters. It also works around most frame rate detection code.
>> I don't see a single reason to not offer users the choice to increase
>> the precision to their liking.
> I see no reason not to. But I also see no reason users should do it, and
> you have not convinced me.
Because 1ms is not enough to properly represent anything except the previously stated rates. I think we're going in circles here now.
>> I have yet to see anyone use timestamps wrong.
> I suggest you spend a little time doing support on the users
> mailing-list and you will see plenty.
>
>> Timestamps should be as accurate as possible, especially when you
>> consider that even with 1µs of precision in a 64-bit timestamp, you
>> can still mux for at least 584000 years before you have to wrap around
>> to zero again.
> You need to learn about the difference between accuracy and precision.

I am aware of the difference between accuracy and precision, even though English is my third language:
- https://www.merriam-webster.com/dictionary/precision, entry 1 of 2, 1.
- https://www.merriam-webster.com/dictionary/accuracy, 1.

I do not appreciate this hostility about two words that in my language end up being the exact same word. I will retreat from further discussion with you for this reason.

Michael Fabian 'Xaymar' Dirks (12021-05-20):
> At the current time there aren't many editors that support Matroska,
> but Black Magic Design DaVinci Resolve is one of them. Resolve
> approximates the 60 fps footage to be 59.94 fps, which is close but
> not exact - off by just 0.06 seconds. This is enough to create an
> audio drift of 1 second after just 16.67 seconds, as changing the
> "source" frame rate apparently means speeding up the footage.
> 
> You could consider this a "bug", but is this really a bug or is this
> just terrible time stamp support in Matroska? In my opinion it is the
> latter.

There is absolutely no doubt this is a bug, because timestamps do not
drift.

And just in case you are not convinced this is a bug, if the issue was
what you thought it was, you would get a drift of one second after two
thousand seconds, or at worst one thousand. Not sixteen.

> libx264, nvenc, amf, qsv, and probably plenty of other encoders
> disagree, as they base their bitrate decisions on how much bitrate is
> available at the current time.

They can make their bitrate decisions with timestamps.

> > > Remuxing requires it so that it doesn't end up as "variable" without
> > Why would it be a problem?

You neglected to answer this. It's a shame, because it was the most
important question.

> FFmpeg generates an mp4 file that is variable, not constant. This
> happens with every MKV file muxed as 1ms, as Matroska so far has no
> proper frame rate tags or rational per-track time bases. "-vsync cfr"
> does not fix this issue. "-framerate 60" does not fix this issue. None
> of the supposed workarounds are true fixes for the issue. This isn't a
> fix for the issue either, but it increases the precision enough to not
> require either of those parameters. It also works around most frame
> rate detection code.

I am not sure about this, but I am very dubious, because as far as I
know, our MP4 muxer only supports constant frame rate.

> Because 1ms is not enough to properly represent anything except the
> previously stated rates. I think we're going in circles here now.

I am sorry, but the arithmetic rules that limit 1 ms to exactly
represent only a handful of rates applies to 1 ns too, because the prime
factors are the same.

And we are indeed going in circles, because I have already explained
this: a smaller time base will not fix bad math, it will only make it
accumulate slower.

> > > I have yet to see anyone use timestamps wrong.
> > > Timestamps should be as accurate as possible, especially when you
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > consider that even with 1µs of precision in a 64-bit timestamp, you
> > > can still mux for at least 584000 years before you have to wrap around
> > > to zero again.
> > You need to learn about the difference between accuracy and precision.
> 
> I am aware of the difference between accuracy and precision, even though English is my third language:
> - https://www.merriam-webster.com/dictionary/precision, entry 1 of 2, 1.
> - https://www.merriam-webster.com/dictionary/accuracy, 1.
> 
> I do not appreciate this hostility about two words that in my language
> end up being the exact same word. I will retreat from further
> discussion with you for this reason.

How was I supposed to know about your language? And why should I care?
There was no hostility in my message, only a little tiredness. The only
thing that matters is that you used the wrong concept. Note: the wrong
concept, not just the wrong word. Your current mail confirms this.

So, let me state it clearly: one millisecond is not very precise, but it
is accurate enough for most uses, because most timings, about contents
and hardware, is significantly less accurate.

You have yet to produce arguments to contradict this.

Regards,

May 20, 2021, 20:08 by jamrial@gmail.com:

> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>
>> On 2021-05-20 19:26, James Almer wrote:
>>
>>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>>
>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>
>>>> Adds "timestamp_precision" to the available option for Matroska/WebM
>>>> muxing. The option enables users and developers to change the precision
>>>> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
>>>> which can aid with the proper detection of constant and variable rate
>>>> content.
>>>>
>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>
>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>> ---
>>>>   doc/muxers.texi           |  8 ++++++++
>>>>   libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>   2 files changed, 28 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>> index e1c6ad0829..d9f7badae1 100644
>>>> --- a/doc/muxers.texi
>>>> +++ b/doc/muxers.texi
>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>>>>   which has to be done manually beforehand, e.g. by using the vflip filter.
>>>>   Default is @var{false} and indicates bitmap is stored top down.
>>>>   +@item timestamp_precision
>>>> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
>>>> +improve detection of constant rate content in demuxers. Note that some poorly
>>>> +implemented demuxers may require a timestamp precision of 1 millisecond, so
>>>> +increasing it past that point may result in playback issues. Higher precision
>>>> +also reduces the maximum possible timestamp significantly.
>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>
>>>
>>> Like i said, the default must be the one defined in the spec. This version not only would need FATE test updates, it also like you described makes all new files by default have a lot of overhead from having one block per cluster.
>>>
>>
>> I am aware of what you said and I am also aware of Lynne said. I do not know who has the actual final say in this, all I know is that the maintainers for matroskaenc.c are "David Conrad" and "Andreas Rheinhardt" - both of which have not commented on this yet.
>>
>
> Lynne agreed on IRC that it can remain as 1ms.
>
If there's a chance it could be changed to 1us (the common internal lavf timebase),
I'd like for it to be, but not without a small partial study of compatibility with common
implementations.
Specifically, the one case I've heard where non-1ms timebases had issues was with VLC's
demuxer, where apparently giving an external .mks file as subtitles with a non-1ms timebase
caused the player to OOM. If this is/was still true, for shame, for shame.

Also, the patch is missing some error-checking. WebM hard-requires a 1ms timebase.
You should likely error out if the demuxer acts in WebM mode and the timebase is set
to a different value.

As for the whole "1ms is good enough for everything!", well, it's not for 960fps VFR video
and that's all I'm going to say. Where you get 960fps VFR videos? Smartphones.
Even a cheap one from 5 years ago can shoot at 240fps. They're really common, and
depending on what strategy you use to convert the video, rounding jitter can result in some
horrible slow-mo conversions.


> Probably should have said it here, but i guess neither him
>
she. Try to remember, this hasn't been the first time.

On 5/20/2021 7:20 PM, Lynne wrote:
> May 20, 2021, 20:08 by jamrial@gmail.com:
> 
>> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>
>>> On 2021-05-20 19:26, James Almer wrote:
>>>
>>>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>>>
>>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>
>>>>> Adds "timestamp_precision" to the available option for Matroska/WebM
>>>>> muxing. The option enables users and developers to change the precision
>>>>> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
>>>>> which can aid with the proper detection of constant and variable rate
>>>>> content.
>>>>>
>>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>>
>>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>> ---
>>>>>    doc/muxers.texi           |  8 ++++++++
>>>>>    libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>>> index e1c6ad0829..d9f7badae1 100644
>>>>> --- a/doc/muxers.texi
>>>>> +++ b/doc/muxers.texi
>>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>>>>>    which has to be done manually beforehand, e.g. by using the vflip filter.
>>>>>    Default is @var{false} and indicates bitmap is stored top down.
>>>>>    +@item timestamp_precision
>>>>> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
>>>>> +improve detection of constant rate content in demuxers. Note that some poorly
>>>>> +implemented demuxers may require a timestamp precision of 1 millisecond, so
>>>>> +increasing it past that point may result in playback issues. Higher precision
>>>>> +also reduces the maximum possible timestamp significantly.
>>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>>
>>>>
>>>> Like i said, the default must be the one defined in the spec. This version not only would need FATE test updates, it also like you described makes all new files by default have a lot of overhead from having one block per cluster.
>>>>
>>>
>>> I am aware of what you said and I am also aware of Lynne said. I do not know who has the actual final say in this, all I know is that the maintainers for matroskaenc.c are "David Conrad" and "Andreas Rheinhardt" - both of which have not commented on this yet.
>>>
>>
>> Lynne agreed on IRC that it can remain as 1ms.
>>
> If there's a chance it could be changed to 1us (the common internal lavf timebase),
> I'd like for it to be, but not without a small partial study of compatibility with common
> implementations.
> Specifically, the one case I've heard where non-1ms timebases had issues was with VLC's
> demuxer, where apparently giving an external .mks file as subtitles with a non-1ms timebase
> caused the player to OOM. If this is/was still true, for shame, for shame.

1us is incompatible with the default value for cluster_time_limit. It's 
in fact incompatible with any value for it, since it will force one 
block per cluster.
If the user wants that, then they are free to set 1us, but the default 
of one option should not invalidate the default of another. It's the 
entire point the warning exists for and is printed.

> 
> Also, the patch is missing some error-checking. WebM hard-requires a 1ms timebase.
> You should likely error out if the demuxer acts in WebM mode and the timebase is set
> to a different value.

Good catch.

> 
> As for the whole "1ms is good enough for everything!", well, it's not for 960fps VFR video
> and that's all I'm going to say. Where you get 960fps VFR videos? Smartphones.
> Even a cheap one from 5 years ago can shoot at 240fps. They're really common, and
> depending on what strategy you use to convert the video, rounding jitter can result in some
> horrible slow-mo conversions.
> 
> 
>> Probably should have said it here, but i guess neither him
>>
> she. Try to remember, this hasn't been the first time.

Sorry, i just default to that in general. Will try to not make that 
mistake again.

> _______________________________________________
> 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".
>

On 2021-05-21 00:25, James Almer wrote:
> On 5/20/2021 7:20 PM, Lynne wrote:
>> May 20, 2021, 20:08 by jamrial@gmail.com:
>>
>>> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>>
>>>> On 2021-05-20 19:26, James Almer wrote:
>>>>
>>>>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>>>>
>>>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>>
>>>>>> Adds "timestamp_precision" to the available option for Matroska/WebM
>>>>>> muxing. The option enables users and developers to change the precision
>>>>>> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
>>>>>> which can aid with the proper detection of constant and variable rate
>>>>>> content.
>>>>>>
>>>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>>>
>>>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>> ---
>>>>>>    doc/muxers.texi           |  8 ++++++++
>>>>>>    libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>>>> index e1c6ad0829..d9f7badae1 100644
>>>>>> --- a/doc/muxers.texi
>>>>>> +++ b/doc/muxers.texi
>>>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>>>>>>    which has to be done manually beforehand, e.g. by using the vflip filter.
>>>>>>    Default is @var{false} and indicates bitmap is stored top down.
>>>>>>    +@item timestamp_precision
>>>>>> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
>>>>>> +improve detection of constant rate content in demuxers. Note that some poorly
>>>>>> +implemented demuxers may require a timestamp precision of 1 millisecond, so
>>>>>> +increasing it past that point may result in playback issues. Higher precision
>>>>>> +also reduces the maximum possible timestamp significantly.
>>>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>>>
>>>>>
>>>>> Like i said, the default must be the one defined in the spec. This version not only would need FATE test updates, it also like you described makes all new files by default have a lot of overhead from having one block per cluster.
>>>>>
>>>>
>>>> I am aware of what you said and I am also aware of Lynne said. I do not know who has the actual final say in this, all I know is that the maintainers for matroskaenc.c are "David Conrad" and "Andreas Rheinhardt" - both of which have not commented on this yet.
>>>>
>>>
>>> Lynne agreed on IRC that it can remain as 1ms.
>>>
>> If there's a chance it could be changed to 1us (the common internal lavf timebase),
>> I'd like for it to be, but not without a small partial study of compatibility with common
>> implementations.
>> Specifically, the one case I've heard where non-1ms timebases had issues was with VLC's
>> demuxer, where apparently giving an external .mks file as subtitles with a non-1ms timebase
>> caused the player to OOM. If this is/was still true, for shame, for shame.
>
> 1us is incompatible with the default value for cluster_time_limit. It's in fact incompatible with any value for it, since it will force one block per cluster.
> If the user wants that, then they are free to set 1us, but the default of one option should not invalidate the default of another. It's the entire point the warning exists for and is printed.
>
>>
>> Also, the patch is missing some error-checking. WebM hard-requires a 1ms timebase.
>> You should likely error out if the demuxer acts in WebM mode and the timebase is set
>> to a different value.
>
> Good catch.

I may be misreading the WebM specification, but it only says it SHOULD be set 1.000.000 nanoseconds, not MUST, which going by the rest of the wording does not mean it is required, just very very recommended. Perhaps a warning for WebM would be better than an error-exit?

>
>>
>> As for the whole "1ms is good enough for everything!", well, it's not for 960fps VFR video
>> and that's all I'm going to say. Where you get 960fps VFR videos? Smartphones.
>> Even a cheap one from 5 years ago can shoot at 240fps. They're really common, and
>> depending on what strategy you use to convert the video, rounding jitter can result in some
>> horrible slow-mo conversions.
>>
>>
>>> Probably should have said it here, but i guess neither him
>>>
>> she. Try to remember, this hasn't been the first time.
>
> Sorry, i just default to that in general. Will try to not make that mistake again.

On 2021-05-21 00:30, Michael Fabian 'Xaymar' Dirks wrote:
> On 2021-05-21 00:25, James Almer wrote:
>> On 5/20/2021 7:20 PM, Lynne wrote:
>>> May 20, 2021, 20:08 by jamrial@gmail.com:
>>>
>>>> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>>>
>>>>> On 2021-05-20 19:26, James Almer wrote:
>>>>>
>>>>>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>>>>>
>>>>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>>>
>>>>>>> Adds "timestamp_precision" to the available option for Matroska/WebM
>>>>>>> muxing. The option enables users and developers to change the precision
>>>>>>> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
>>>>>>> which can aid with the proper detection of constant and variable rate
>>>>>>> content.
>>>>>>>
>>>>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>>>>
>>>>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>>> ---
>>>>>>>    doc/muxers.texi           |  8 ++++++++
>>>>>>>    libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>>>>> index e1c6ad0829..d9f7badae1 100644
>>>>>>> --- a/doc/muxers.texi
>>>>>>> +++ b/doc/muxers.texi
>>>>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>>>>>>>    which has to be done manually beforehand, e.g. by using the vflip filter.
>>>>>>>    Default is @var{false} and indicates bitmap is stored top down.
>>>>>>>    +@item timestamp_precision
>>>>>>> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
>>>>>>> +improve detection of constant rate content in demuxers. Note that some poorly
>>>>>>> +implemented demuxers may require a timestamp precision of 1 millisecond, so
>>>>>>> +increasing it past that point may result in playback issues. Higher precision
>>>>>>> +also reduces the maximum possible timestamp significantly.
>>>>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>>>>
>>>>>>
>>>>>> Like i said, the default must be the one defined in the spec. This version not only would need FATE test updates, it also like you described makes all new files by default have a lot of overhead from having one block per cluster.
>>>>>>
>>>>>
>>>>> I am aware of what you said and I am also aware of Lynne said. I do not know who has the actual final say in this, all I know is that the maintainers for matroskaenc.c are "David Conrad" and "Andreas Rheinhardt" - both of which have not commented on this yet.
>>>>>
>>>>
>>>> Lynne agreed on IRC that it can remain as 1ms.
>>>>
>>> If there's a chance it could be changed to 1us (the common internal lavf timebase),
>>> I'd like for it to be, but not without a small partial study of compatibility with common
>>> implementations.
>>> Specifically, the one case I've heard where non-1ms timebases had issues was with VLC's
>>> demuxer, where apparently giving an external .mks file as subtitles with a non-1ms timebase
>>> caused the player to OOM. If this is/was still true, for shame, for shame.
>>
>> 1us is incompatible with the default value for cluster_time_limit. It's in fact incompatible with any value for it, since it will force one block per cluster.
>> If the user wants that, then they are free to set 1us, but the default of one option should not invalidate the default of another. It's the entire point the warning exists for and is printed.
>>
>>>
>>> Also, the patch is missing some error-checking. WebM hard-requires a 1ms timebase.
>>> You should likely error out if the demuxer acts in WebM mode and the timebase is set
>>> to a different value.
>>
>> Good catch.
>
> I may be misreading the WebM specification, but it only says it SHOULD be set 1.000.000 nanoseconds, not MUST, which going by the rest of the wording does not mean it is required, just very very recommended. Perhaps a warning for WebM would be better than an error-exit?
>
I was misreading the specification. Shouldn't try to read technical documentation when tired. Will add the error-exit, ignore the patch sent before Lynnes message arrived here.
>>
>>>
>>> As for the whole "1ms is good enough for everything!", well, it's not for 960fps VFR video
>>> and that's all I'm going to say. Where you get 960fps VFR videos? Smartphones.
>>> Even a cheap one from 5 years ago can shoot at 240fps. They're really common, and
>>> depending on what strategy you use to convert the video, rounding jitter can result in some
>>> horrible slow-mo conversions.
>>>
>>>
>>>> Probably should have said it here, but i guess neither him
>>>>
>>> she. Try to remember, this hasn't been the first time.
>>
>> Sorry, i just default to that in general. Will try to not make that mistake again.
>

On 5/20/2021 7:33 PM, Michael Fabian 'Xaymar' Dirks wrote:
> On 2021-05-21 00:30, Michael Fabian 'Xaymar' Dirks wrote:
>> On 2021-05-21 00:25, James Almer wrote:
>>> On 5/20/2021 7:20 PM, Lynne wrote:
>>>> May 20, 2021, 20:08 by jamrial@gmail.com:
>>>>
>>>>> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>>>>
>>>>>> On 2021-05-20 19:26, James Almer wrote:
>>>>>>
>>>>>>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>>>>>>
>>>>>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>>>>
>>>>>>>> Adds "timestamp_precision" to the available option for 
>>>>>>>> Matroska/WebM
>>>>>>>> muxing. The option enables users and developers to change the 
>>>>>>>> precision
>>>>>>>> of the time stamps in the Matroska/WebM container up to 1 
>>>>>>>> nanosecond,
>>>>>>>> which can aid with the proper detection of constant and variable 
>>>>>>>> rate
>>>>>>>> content.
>>>>>>>>
>>>>>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks 
>>>>>>>> <michael.dirks@xaymar.com>
>>>>>>>> ---
>>>>>>>>    doc/muxers.texi           |  8 ++++++++
>>>>>>>>    libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>>>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>>>>>> index e1c6ad0829..d9f7badae1 100644
>>>>>>>> --- a/doc/muxers.texi
>>>>>>>> +++ b/doc/muxers.texi
>>>>>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that 
>>>>>>>> this option does not flip the bitmap
>>>>>>>>    which has to be done manually beforehand, e.g. by using the 
>>>>>>>> vflip filter.
>>>>>>>>    Default is @var{false} and indicates bitmap is stored top down.
>>>>>>>>    +@item timestamp_precision
>>>>>>>> +Sets the timestamp precision up to 1 nanosecond for 
>>>>>>>> Matroska/WebM, which can
>>>>>>>> +improve detection of constant rate content in demuxers. Note 
>>>>>>>> that some poorly
>>>>>>>> +implemented demuxers may require a timestamp precision of 1 
>>>>>>>> millisecond, so
>>>>>>>> +increasing it past that point may result in playback issues. 
>>>>>>>> Higher precision
>>>>>>>> +also reduces the maximum possible timestamp significantly.
>>>>>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>>>>>
>>>>>>>
>>>>>>> Like i said, the default must be the one defined in the spec. 
>>>>>>> This version not only would need FATE test updates, it also like 
>>>>>>> you described makes all new files by default have a lot of 
>>>>>>> overhead from having one block per cluster.
>>>>>>>
>>>>>>
>>>>>> I am aware of what you said and I am also aware of Lynne said. I 
>>>>>> do not know who has the actual final say in this, all I know is 
>>>>>> that the maintainers for matroskaenc.c are "David Conrad" and 
>>>>>> "Andreas Rheinhardt" - both of which have not commented on this yet.
>>>>>>
>>>>>
>>>>> Lynne agreed on IRC that it can remain as 1ms.
>>>>>
>>>> If there's a chance it could be changed to 1us (the common internal 
>>>> lavf timebase),
>>>> I'd like for it to be, but not without a small partial study of 
>>>> compatibility with common
>>>> implementations.
>>>> Specifically, the one case I've heard where non-1ms timebases had 
>>>> issues was with VLC's
>>>> demuxer, where apparently giving an external .mks file as subtitles 
>>>> with a non-1ms timebase
>>>> caused the player to OOM. If this is/was still true, for shame, for 
>>>> shame.
>>>
>>> 1us is incompatible with the default value for cluster_time_limit. 
>>> It's in fact incompatible with any value for it, since it will force 
>>> one block per cluster.
>>> If the user wants that, then they are free to set 1us, but the 
>>> default of one option should not invalidate the default of another. 
>>> It's the entire point the warning exists for and is printed.
>>>
>>>>
>>>> Also, the patch is missing some error-checking. WebM hard-requires a 
>>>> 1ms timebase.
>>>> You should likely error out if the demuxer acts in WebM mode and the 
>>>> timebase is set
>>>> to a different value.
>>>
>>> Good catch.
>>
>> I may be misreading the WebM specification, but it only says it SHOULD 
>> be set 1.000.000 nanoseconds, not MUST, which going by the rest of the 
>> wording does not mean it is required, just very very recommended. 
>> Perhaps a warning for WebM would be better than an error-exit?
>>
> I was misreading the specification. Shouldn't try to read technical 
> documentation when tired. Will add the error-exit, ignore the patch sent 
> before Lynnes message arrived here.

No, apparently you were right. In 
https://www.webmproject.org/docs/container/ there's a line that says 
"The TimecodeScale element SHOULD be set to a default of 1.000.000 
nanoseconds.", which means it's recommended but not required. Or is 
there another source for the spec?

Also, the spec uses TimecodeScale and TimestampScale interchangeably, 
which makes things a bit confusing.

>>>
>>>>
>>>> As for the whole "1ms is good enough for everything!", well, it's 
>>>> not for 960fps VFR video
>>>> and that's all I'm going to say. Where you get 960fps VFR videos? 
>>>> Smartphones.
>>>> Even a cheap one from 5 years ago can shoot at 240fps. They're 
>>>> really common, and
>>>> depending on what strategy you use to convert the video, rounding 
>>>> jitter can result in some
>>>> horrible slow-mo conversions.
>>>>
>>>>
>>>>> Probably should have said it here, but i guess neither him
>>>>>
>>>> she. Try to remember, this hasn't been the first time.
>>>
>>> Sorry, i just default to that in general. Will try to not make that 
>>> mistake again.
>>
>

On 2021-05-21 00:37, James Almer wrote:
> On 5/20/2021 7:33 PM, Michael Fabian 'Xaymar' Dirks wrote:
>> On 2021-05-21 00:30, Michael Fabian 'Xaymar' Dirks wrote:
>>> On 2021-05-21 00:25, James Almer wrote:
>>>> On 5/20/2021 7:20 PM, Lynne wrote:
>>>>> May 20, 2021, 20:08 by jamrial@gmail.com:
>>>>>
>>>>>> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>>>>>
>>>>>>> On 2021-05-20 19:26, James Almer wrote:
>>>>>>>
>>>>>>>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>>>>>>>
>>>>>>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>>>>>
>>>>>>>>> Adds "timestamp_precision" to the available option for Matroska/WebM
>>>>>>>>> muxing. The option enables users and developers to change the precision
>>>>>>>>> of the time stamps in the Matroska/WebM container up to 1 nanosecond,
>>>>>>>>> which can aid with the proper detection of constant and variable rate
>>>>>>>>> content.
>>>>>>>>>
>>>>>>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>>>>> ---
>>>>>>>>>    doc/muxers.texi           |  8 ++++++++
>>>>>>>>>    libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>>>>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>>>>>>> index e1c6ad0829..d9f7badae1 100644
>>>>>>>>> --- a/doc/muxers.texi
>>>>>>>>> +++ b/doc/muxers.texi
>>>>>>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that this option does not flip the bitmap
>>>>>>>>>    which has to be done manually beforehand, e.g. by using the vflip filter.
>>>>>>>>>    Default is @var{false} and indicates bitmap is stored top down.
>>>>>>>>>    +@item timestamp_precision
>>>>>>>>> +Sets the timestamp precision up to 1 nanosecond for Matroska/WebM, which can
>>>>>>>>> +improve detection of constant rate content in demuxers. Note that some poorly
>>>>>>>>> +implemented demuxers may require a timestamp precision of 1 millisecond, so
>>>>>>>>> +increasing it past that point may result in playback issues. Higher precision
>>>>>>>>> +also reduces the maximum possible timestamp significantly.
>>>>>>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>>>>>>
>>>>>>>>
>>>>>>>> Like i said, the default must be the one defined in the spec. This version not only would need FATE test updates, it also like you described makes all new files by default have a lot of overhead from having one block per cluster.
>>>>>>>>
>>>>>>>
>>>>>>> I am aware of what you said and I am also aware of Lynne said. I do not know who has the actual final say in this, all I know is that the maintainers for matroskaenc.c are "David Conrad" and "Andreas Rheinhardt" - both of which have not commented on this yet.
>>>>>>>
>>>>>>
>>>>>> Lynne agreed on IRC that it can remain as 1ms.
>>>>>>
>>>>> If there's a chance it could be changed to 1us (the common internal lavf timebase),
>>>>> I'd like for it to be, but not without a small partial study of compatibility with common
>>>>> implementations.
>>>>> Specifically, the one case I've heard where non-1ms timebases had issues was with VLC's
>>>>> demuxer, where apparently giving an external .mks file as subtitles with a non-1ms timebase
>>>>> caused the player to OOM. If this is/was still true, for shame, for shame.
>>>>
>>>> 1us is incompatible with the default value for cluster_time_limit. It's in fact incompatible with any value for it, since it will force one block per cluster.
>>>> If the user wants that, then they are free to set 1us, but the default of one option should not invalidate the default of another. It's the entire point the warning exists for and is printed.
>>>>
>>>>>
>>>>> Also, the patch is missing some error-checking. WebM hard-requires a 1ms timebase.
>>>>> You should likely error out if the demuxer acts in WebM mode and the timebase is set
>>>>> to a different value.
>>>>
>>>> Good catch.
>>>
>>> I may be misreading the WebM specification, but it only says it SHOULD be set 1.000.000 nanoseconds, not MUST, which going by the rest of the wording does not mean it is required, just very very recommended. Perhaps a warning for WebM would be better than an error-exit?
>>>
>> I was misreading the specification. Shouldn't try to read technical documentation when tired. Will add the error-exit, ignore the patch sent before Lynnes message arrived here.
>
> No, apparently you were right. In https://www.webmproject.org/docs/container/ there's a line that says "The TimecodeScale element SHOULD be set to a default of 1.000.000 nanoseconds.", which means it's recommended but not required. Or is there another source for the spec?
>
> Also, the spec uses TimecodeScale and TimestampScale interchangeably, which makes things a bit confusing.
>
Directly above it:

Muxers should treat all guidelines marked SHOULD in this section as MUST. This will foster consistency across WebM files in the real world.

I missed it as well as it just didn't look important at first.

>>>>
>>>>>
>>>>> As for the whole "1ms is good enough for everything!", well, it's not for 960fps VFR video
>>>>> and that's all I'm going to say. Where you get 960fps VFR videos? Smartphones.
>>>>> Even a cheap one from 5 years ago can shoot at 240fps. They're really common, and
>>>>> depending on what strategy you use to convert the video, rounding jitter can result in some
>>>>> horrible slow-mo conversions.
>>>>>
>>>>>
>>>>>> Probably should have said it here, but i guess neither him
>>>>>>
>>>>> she. Try to remember, this hasn't been the first time.
>>>>
>>>> Sorry, i just default to that in general. Will try to not make that mistake again.

On 5/20/2021 7:57 PM, Michael Fabian 'Xaymar' Dirks wrote:
> On 2021-05-21 00:37, James Almer wrote:
>> On 5/20/2021 7:33 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>> On 2021-05-21 00:30, Michael Fabian 'Xaymar' Dirks wrote:
>>>> On 2021-05-21 00:25, James Almer wrote:
>>>>> On 5/20/2021 7:20 PM, Lynne wrote:
>>>>>> May 20, 2021, 20:08 by jamrial@gmail.com:
>>>>>>
>>>>>>> On 5/20/2021 3:00 PM, Michael Fabian 'Xaymar' Dirks wrote:
>>>>>>>
>>>>>>>> On 2021-05-20 19:26, James Almer wrote:
>>>>>>>>
>>>>>>>>> On 5/20/2021 2:18 PM, michael.dirks@xaymar.com wrote:
>>>>>>>>>
>>>>>>>>>> From: Michael Fabian 'Xaymar' Dirks <michael.dirks@xaymar.com>
>>>>>>>>>>
>>>>>>>>>> Adds "timestamp_precision" to the available option for 
>>>>>>>>>> Matroska/WebM
>>>>>>>>>> muxing. The option enables users and developers to change the 
>>>>>>>>>> precision
>>>>>>>>>> of the time stamps in the Matroska/WebM container up to 1 
>>>>>>>>>> nanosecond,
>>>>>>>>>> which can aid with the proper detection of constant and 
>>>>>>>>>> variable rate
>>>>>>>>>> content.
>>>>>>>>>>
>>>>>>>>>> Work-around fix for: 259, 6406, 7927, 8909 and 9124.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Michael Fabian 'Xaymar' Dirks 
>>>>>>>>>> <michael.dirks@xaymar.com>
>>>>>>>>>> ---
>>>>>>>>>>    doc/muxers.texi           |  8 ++++++++
>>>>>>>>>>    libavformat/matroskaenc.c | 28 ++++++++++++++++++++--------
>>>>>>>>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>>>>>>>>>> index e1c6ad0829..d9f7badae1 100644
>>>>>>>>>> --- a/doc/muxers.texi
>>>>>>>>>> +++ b/doc/muxers.texi
>>>>>>>>>> @@ -1583,6 +1583,14 @@ bitmap is stored bottom-up. Note that 
>>>>>>>>>> this option does not flip the bitmap
>>>>>>>>>>    which has to be done manually beforehand, e.g. by using the 
>>>>>>>>>> vflip filter.
>>>>>>>>>>    Default is @var{false} and indicates bitmap is stored top 
>>>>>>>>>> down.
>>>>>>>>>>    +@item timestamp_precision
>>>>>>>>>> +Sets the timestamp precision up to 1 nanosecond for 
>>>>>>>>>> Matroska/WebM, which can
>>>>>>>>>> +improve detection of constant rate content in demuxers. Note 
>>>>>>>>>> that some poorly
>>>>>>>>>> +implemented demuxers may require a timestamp precision of 1 
>>>>>>>>>> millisecond, so
>>>>>>>>>> +increasing it past that point may result in playback issues. 
>>>>>>>>>> Higher precision
>>>>>>>>>> +also reduces the maximum possible timestamp significantly.
>>>>>>>>>> +Default is @var{1/1000000000} (1 nanosecond).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Like i said, the default must be the one defined in the spec. 
>>>>>>>>> This version not only would need FATE test updates, it also 
>>>>>>>>> like you described makes all new files by default have a lot of 
>>>>>>>>> overhead from having one block per cluster.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I am aware of what you said and I am also aware of Lynne said. I 
>>>>>>>> do not know who has the actual final say in this, all I know is 
>>>>>>>> that the maintainers for matroskaenc.c are "David Conrad" and 
>>>>>>>> "Andreas Rheinhardt" - both of which have not commented on this 
>>>>>>>> yet.
>>>>>>>>
>>>>>>>
>>>>>>> Lynne agreed on IRC that it can remain as 1ms.
>>>>>>>
>>>>>> If there's a chance it could be changed to 1us (the common 
>>>>>> internal lavf timebase),
>>>>>> I'd like for it to be, but not without a small partial study of 
>>>>>> compatibility with common
>>>>>> implementations.
>>>>>> Specifically, the one case I've heard where non-1ms timebases had 
>>>>>> issues was with VLC's
>>>>>> demuxer, where apparently giving an external .mks file as 
>>>>>> subtitles with a non-1ms timebase
>>>>>> caused the player to OOM. If this is/was still true, for shame, 
>>>>>> for shame.
>>>>>
>>>>> 1us is incompatible with the default value for cluster_time_limit. 
>>>>> It's in fact incompatible with any value for it, since it will 
>>>>> force one block per cluster.
>>>>> If the user wants that, then they are free to set 1us, but the 
>>>>> default of one option should not invalidate the default of another. 
>>>>> It's the entire point the warning exists for and is printed.
>>>>>
>>>>>>
>>>>>> Also, the patch is missing some error-checking. WebM hard-requires 
>>>>>> a 1ms timebase.
>>>>>> You should likely error out if the demuxer acts in WebM mode and 
>>>>>> the timebase is set
>>>>>> to a different value.
>>>>>
>>>>> Good catch.
>>>>
>>>> I may be misreading the WebM specification, but it only says it 
>>>> SHOULD be set 1.000.000 nanoseconds, not MUST, which going by the 
>>>> rest of the wording does not mean it is required, just very very 
>>>> recommended. Perhaps a warning for WebM would be better than an 
>>>> error-exit?
>>>>
>>> I was misreading the specification. Shouldn't try to read technical 
>>> documentation when tired. Will add the error-exit, ignore the patch 
>>> sent before Lynnes message arrived here.
>>
>> No, apparently you were right. In 
>> https://www.webmproject.org/docs/container/ there's a line that says 
>> "The TimecodeScale element SHOULD be set to a default of 1.000.000 
>> nanoseconds.", which means it's recommended but not required. Or is 
>> there another source for the spec?
>>
>> Also, the spec uses TimecodeScale and TimestampScale interchangeably, 
>> which makes things a bit confusing.
>>
> Directly above it:
> 
> Muxers should treat all guidelines marked SHOULD in this section as 
> MUST. This will foster consistency across WebM files in the real world.

Oh FFS, don't use rfc2119 keywords verbatim in your spec if you're not 
compliant with it.

> 
> I missed it as well as it just didn't look important at first.

Thanks for clearing that out.

> 
>>>>>
>>>>>>
>>>>>> As for the whole "1ms is good enough for everything!", well, it's 
>>>>>> not for 960fps VFR video
>>>>>> and that's all I'm going to say. Where you get 960fps VFR videos? 
>>>>>> Smartphones.
>>>>>> Even a cheap one from 5 years ago can shoot at 240fps. They're 
>>>>>> really common, and
>>>>>> depending on what strategy you use to convert the video, rounding 
>>>>>> jitter can result in some
>>>>>> horrible slow-mo conversions.
>>>>>>
>>>>>>
>>>>>>> Probably should have said it here, but i guess neither him
>>>>>>>
>>>>>> she. Try to remember, this hasn't been the first time.
>>>>>
>>>>> Sorry, i just default to that in general. Will try to not make that 
>>>>> mistake again.
> 
>