[FFmpeg-devel] lavf/mkvtimestamp_v2: review mkvtimestamp_v2 implementation, add documentation

[FFmpeg-devel] lavf/mkvtimestamp_v2: review mkvtimestamp_v2 implementation, add documentation

[Stefano Sabatini]

[PATCH 1/3] lavf/mkvtimestamp_v2: use name in place of description in
[PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match
[PATCH 3/3] doc/muxers: add mkvtimestamp_v2

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

[FFmpeg-devel] [PATCH 1/3] lavf/mkvtimestamp_v2: use name in place of description in long name

[Stefano Sabatini]

---
 libavformat/mkvtimestamp_v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mkvtimestamp_v2.c b/libavformat/mkvtimestamp_v2.c
index dde431ab7d..1eb2daf10a 100644
--- a/libavformat/mkvtimestamp_v2.c
+++ b/libavformat/mkvtimestamp_v2.c
@@ -43,7 +43,7 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt)
 
 const FFOutputFormat ff_mkvtimestamp_v2_muxer = {
     .p.name        = "mkvtimestamp_v2",
-    .p.long_name   = NULL_IF_CONFIG_SMALL("extract pts as timecode v2 format, as defined by mkvtoolnix"),
+    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timecode format"),
     .p.audio_codec = AV_CODEC_ID_NONE,
     .p.video_codec = AV_CODEC_ID_RAWVIDEO,
     .write_header = write_header,
-- 
2.34.1

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

[FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior

[Stefano Sabatini]

Harmonize internal implementation with the mkvextract behavior:
- print PTS in place of DTS values
- ignore NOPTS values
- sort PTS values
---
 libavformat/mkvtimestamp_v2.c | 69 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/libavformat/mkvtimestamp_v2.c b/libavformat/mkvtimestamp_v2.c
index 1eb2daf10a..c6446ed489 100644
--- a/libavformat/mkvtimestamp_v2.c
+++ b/libavformat/mkvtimestamp_v2.c
@@ -22,30 +22,91 @@
 #include "avformat.h"
 #include "internal.h"
 #include "mux.h"
+#include "libavutil/qsort.h"
+
+#define PTSS_MAX_SIZE 128
+#define PTSS_HALF_SIZE (PTSS_MAX_SIZE >> 1)
+
+struct MkvTimestampContext {
+    int64_t ptss[PTSS_MAX_SIZE];
+    size_t ptss_size;
+};
 
 static int write_header(AVFormatContext *s)
 {
-    static const char *header = "# timecode format v2\n";
+    static const char *header = "# timestamp format v2\n";
     avio_write(s->pb, header, strlen(header));
     avpriv_set_pts_info(s->streams[0], 64, 1, 1000);
+
     return 0;
 }
 
+static int cmp_int64(const void *p1, const void *p2)
+{
+    int64_t left  = *(const int64_t *)p1;
+    int64_t right = *(const int64_t *)p2;
+    return FFDIFFSIGN(left, right);
+}
+
 static int write_packet(AVFormatContext *s, AVPacket *pkt)
 {
     char buf[256];
+    int i;
+    struct MkvTimestampContext *m = s->priv_data;
+
     if (pkt->stream_index)
         av_log(s, AV_LOG_WARNING, "More than one stream unsupported\n");
-    snprintf(buf, sizeof(buf), "%" PRId64 "\n", pkt->dts);
-    avio_write(s->pb, buf, strlen(buf));
+
+    if (pkt->pts == AV_NOPTS_VALUE) {
+        av_log(s, AV_LOG_WARNING, "Found PTS with no value, ignored\n");
+        return 0;
+    }
+
+    if (m->ptss_size > PTSS_MAX_SIZE) {
+        // sort all PTSs
+        AV_QSORT(m->ptss, PTSS_MAX_SIZE, int64_t, cmp_int64);
+
+        // write only the first half and copy the second half to the
+        // beginning of the array
+        for (i = 0; i < PTSS_HALF_SIZE; i++) {
+            snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
+            avio_write(s->pb, buf, strlen(buf));
+            m->ptss[i] = m->ptss[i + PTSS_HALF_SIZE];
+        }
+
+        m->ptss_size = PTSS_HALF_SIZE;
+    } else {
+        m->ptss[m->ptss_size++] = pkt->pts;
+    }
+
+    return 0;
+}
+
+static int write_trailer(struct AVFormatContext *s)
+{
+    struct MkvTimestampContext *m = s->priv_data;
+    char buf[256];
+    int i;
+
+    // sort all PTSs
+    AV_QSORT(m->ptss, m->ptss_size, int64_t, cmp_int64);
+
+    /* flush remaining timestamps */
+    for (i = 0; i < m->ptss_size; i++) {
+        snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
+        avio_write(s->pb, buf, strlen(buf));
+    }
+
     return 0;
 }
 
 const FFOutputFormat ff_mkvtimestamp_v2_muxer = {
     .p.name        = "mkvtimestamp_v2",
-    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timecode format"),
+    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timestamp format"),
     .p.audio_codec = AV_CODEC_ID_NONE,
     .p.video_codec = AV_CODEC_ID_RAWVIDEO,
     .write_header = write_header,
     .write_packet = write_packet,
+    .write_trailer = write_trailer,
+    .priv_data_size = sizeof(struct MkvTimestampContext),
 };
-- 
2.34.1

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

Re: [FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior

[Andreas Rheinhardt]

Stefano Sabatini:
> Harmonize internal implementation with the mkvextract behavior:
> - print PTS in place of DTS values
> - ignore NOPTS values
> - sort PTS values
> ---
>  libavformat/mkvtimestamp_v2.c | 69 +++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/mkvtimestamp_v2.c b/libavformat/mkvtimestamp_v2.c
> index 1eb2daf10a..c6446ed489 100644
> --- a/libavformat/mkvtimestamp_v2.c
> +++ b/libavformat/mkvtimestamp_v2.c
> @@ -22,30 +22,91 @@
>  #include "avformat.h"
>  #include "internal.h"
>  #include "mux.h"
> +#include "libavutil/qsort.h"
> +
> +#define PTSS_MAX_SIZE 128
> +#define PTSS_HALF_SIZE (PTSS_MAX_SIZE >> 1)
> +
> +struct MkvTimestampContext {
> +    int64_t ptss[PTSS_MAX_SIZE];
> +    size_t ptss_size;
> +};
>  
>  static int write_header(AVFormatContext *s)
>  {
> -    static const char *header = "# timecode format v2\n";
> +    static const char *header = "# timestamp format v2\n";
>      avio_write(s->pb, header, strlen(header));
>      avpriv_set_pts_info(s->streams[0], 64, 1, 1000);
> +
>      return 0;
>  }
>  
> +static int cmp_int64(const void *p1, const void *p2)
> +{
> +    int64_t left  = *(const int64_t *)p1;
> +    int64_t right = *(const int64_t *)p2;
> +    return FFDIFFSIGN(left, right);
> +}
> +
>  static int write_packet(AVFormatContext *s, AVPacket *pkt)
>  {
>      char buf[256];
> +    int i;
> +    struct MkvTimestampContext *m = s->priv_data;
> +
>      if (pkt->stream_index)
>          av_log(s, AV_LOG_WARNING, "More than one stream unsupported\n");
> -    snprintf(buf, sizeof(buf), "%" PRId64 "\n", pkt->dts);
> -    avio_write(s->pb, buf, strlen(buf));
> +
> +    if (pkt->pts == AV_NOPTS_VALUE) {
> +        av_log(s, AV_LOG_WARNING, "Found PTS with no value, ignored\n");
> +        return 0;
> +    }
> +
> +    if (m->ptss_size > PTSS_MAX_SIZE) {
> +        // sort all PTSs
> +        AV_QSORT(m->ptss, PTSS_MAX_SIZE, int64_t, cmp_int64);
> +
> +        // write only the first half and copy the second half to the
> +        // beginning of the array
> +        for (i = 0; i < PTSS_HALF_SIZE; i++) {
> +            snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
> +            avio_write(s->pb, buf, strlen(buf));
> +            m->ptss[i] = m->ptss[i + PTSS_HALF_SIZE];
> +        }
> +
> +        m->ptss_size = PTSS_HALF_SIZE;
> +    } else {
> +        m->ptss[m->ptss_size++] = pkt->pts;
> +    }
> +
> +    return 0;
> +}
> +
> +static int write_trailer(struct AVFormatContext *s)
> +{
> +    struct MkvTimestampContext *m = s->priv_data;
> +    char buf[256];
> +    int i;
> +
> +    // sort all PTSs
> +    AV_QSORT(m->ptss, m->ptss_size, int64_t, cmp_int64);
> +
> +    /* flush remaining timestamps */
> +    for (i = 0; i < m->ptss_size; i++) {
> +        snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
> +        avio_write(s->pb, buf, strlen(buf));
> +    }
> +
>      return 0;
>  }
>  
>  const FFOutputFormat ff_mkvtimestamp_v2_muxer = {
>      .p.name        = "mkvtimestamp_v2",
> -    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timecode format"),
> +    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timestamp format"),
>      .p.audio_codec = AV_CODEC_ID_NONE,
>      .p.video_codec = AV_CODEC_ID_RAWVIDEO,
>      .write_header = write_header,
>      .write_packet = write_packet,
> +    .write_trailer = write_trailer,
> +    .priv_data_size = sizeof(struct MkvTimestampContext),
>  };

1. This does not match mkvextract behaviour. mkvextract does not force a
1ms timebase.
2. AV_QSORT should only be used in speed-critical code, which this here
is definitely not.
3. I still don't think that this muxer should exist at all.

- Andreas

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

Re: [FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior

[Stefano Sabatini]

On date Saturday 2024-04-20 15:18:39 +0200, Andreas Rheinhardt wrote:
> Stefano Sabatini:
> > Harmonize internal implementation with the mkvextract behavior:
> > - print PTS in place of DTS values
> > - ignore NOPTS values
> > - sort PTS values
> > ---
> >  libavformat/mkvtimestamp_v2.c | 69 +++++++++++++++++++++++++++++++++--
> >  1 file changed, 65 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavformat/mkvtimestamp_v2.c b/libavformat/mkvtimestamp_v2.c
> > index 1eb2daf10a..c6446ed489 100644
> > --- a/libavformat/mkvtimestamp_v2.c
> > +++ b/libavformat/mkvtimestamp_v2.c
> > @@ -22,30 +22,91 @@
> >  #include "avformat.h"
> >  #include "internal.h"
> >  #include "mux.h"
> > +#include "libavutil/qsort.h"
> > +
> > +#define PTSS_MAX_SIZE 128
> > +#define PTSS_HALF_SIZE (PTSS_MAX_SIZE >> 1)
> > +
> > +struct MkvTimestampContext {
> > +    int64_t ptss[PTSS_MAX_SIZE];
> > +    size_t ptss_size;
> > +};
> >  
> >  static int write_header(AVFormatContext *s)
> >  {
> > -    static const char *header = "# timecode format v2\n";
> > +    static const char *header = "# timestamp format v2\n";
> >      avio_write(s->pb, header, strlen(header));
> >      avpriv_set_pts_info(s->streams[0], 64, 1, 1000);
> > +
> >      return 0;
> >  }
> >  
> > +static int cmp_int64(const void *p1, const void *p2)
> > +{
> > +    int64_t left  = *(const int64_t *)p1;
> > +    int64_t right = *(const int64_t *)p2;
> > +    return FFDIFFSIGN(left, right);
> > +}
> > +
> >  static int write_packet(AVFormatContext *s, AVPacket *pkt)
> >  {
> >      char buf[256];
> > +    int i;
> > +    struct MkvTimestampContext *m = s->priv_data;
> > +
> >      if (pkt->stream_index)
> >          av_log(s, AV_LOG_WARNING, "More than one stream unsupported\n");
> > -    snprintf(buf, sizeof(buf), "%" PRId64 "\n", pkt->dts);
> > -    avio_write(s->pb, buf, strlen(buf));
> > +
> > +    if (pkt->pts == AV_NOPTS_VALUE) {
> > +        av_log(s, AV_LOG_WARNING, "Found PTS with no value, ignored\n");
> > +        return 0;
> > +    }
> > +
> > +    if (m->ptss_size > PTSS_MAX_SIZE) {
> > +        // sort all PTSs
> > +        AV_QSORT(m->ptss, PTSS_MAX_SIZE, int64_t, cmp_int64);
> > +
> > +        // write only the first half and copy the second half to the
> > +        // beginning of the array
> > +        for (i = 0; i < PTSS_HALF_SIZE; i++) {
> > +            snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
> > +            avio_write(s->pb, buf, strlen(buf));
> > +            m->ptss[i] = m->ptss[i + PTSS_HALF_SIZE];
> > +        }
> > +
> > +        m->ptss_size = PTSS_HALF_SIZE;
> > +    } else {
> > +        m->ptss[m->ptss_size++] = pkt->pts;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int write_trailer(struct AVFormatContext *s)
> > +{
> > +    struct MkvTimestampContext *m = s->priv_data;
> > +    char buf[256];
> > +    int i;
> > +
> > +    // sort all PTSs
> > +    AV_QSORT(m->ptss, m->ptss_size, int64_t, cmp_int64);
> > +
> > +    /* flush remaining timestamps */
> > +    for (i = 0; i < m->ptss_size; i++) {
> > +        snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
> > +        avio_write(s->pb, buf, strlen(buf));
> > +    }
> > +
> >      return 0;
> >  }
> >  
> >  const FFOutputFormat ff_mkvtimestamp_v2_muxer = {
> >      .p.name        = "mkvtimestamp_v2",
> > -    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timecode format"),
> > +    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timestamp format"),
> >      .p.audio_codec = AV_CODEC_ID_NONE,
> >      .p.video_codec = AV_CODEC_ID_RAWVIDEO,
> >      .write_header = write_header,
> >      .write_packet = write_packet,
> > +    .write_trailer = write_trailer,
> > +    .priv_data_size = sizeof(struct MkvTimestampContext),
> >  };
> 

> 1. This does not match mkvextract behaviour. mkvextract does not force a
> 1ms timebase.

From your past comment:
>The accuracy of the timestamps output by mkvextract is determined by the
>TimestampScale of the file in question; it is most often 1ms when the
>file has video.

Note that this is only used for video. I don't know how this
TimestampScale is computed, but I wonder what was the point of this
muxer. Probably it was only used to extract the timestamps with a
fixed timescale, and the fact that it was similar to mkvextract was
not really an important factor.

> 2. AV_QSORT should only be used in speed-critical code, which this here
> is definitely not.

Why? What should I use instead?

> 3. I still don't think that this muxer should exist at all.

I tend to agree. But:

We don't really know why this weird muxer was added, today we have
better tools for that (we could use ffprobe, or even a bitstream
filter similar to showinfo to get the same result), but if it was
added probably there was a reason. So my plan is to make the format a
bit more useful (DTS => PTS+sort), and possibly deprecate it and point
to better available tools and drop this in two major releses.

I don't think the point of the format was to really make the behavior
exactly equal to mkvextract, the thing with TimeScale is probably not
very important, at least for the original author that was not really
an issue, he was probably only looking for some way to dump timestamps
and took free inspiration from mkvtoolnix.

If this is true, we might point that this format is not exactly
equivalent to timestamp_v2 in the doc.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior

[Andreas Rheinhardt]

Stefano Sabatini:
> On date Saturday 2024-04-20 15:18:39 +0200, Andreas Rheinhardt wrote:
>> Stefano Sabatini:
>>> Harmonize internal implementation with the mkvextract behavior:
>>> - print PTS in place of DTS values
>>> - ignore NOPTS values
>>> - sort PTS values
>>> ---
>>>  libavformat/mkvtimestamp_v2.c | 69 +++++++++++++++++++++++++++++++++--
>>>  1 file changed, 65 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavformat/mkvtimestamp_v2.c b/libavformat/mkvtimestamp_v2.c
>>> index 1eb2daf10a..c6446ed489 100644
>>> --- a/libavformat/mkvtimestamp_v2.c
>>> +++ b/libavformat/mkvtimestamp_v2.c
>>> @@ -22,30 +22,91 @@
>>>  #include "avformat.h"
>>>  #include "internal.h"
>>>  #include "mux.h"
>>> +#include "libavutil/qsort.h"
>>> +
>>> +#define PTSS_MAX_SIZE 128
>>> +#define PTSS_HALF_SIZE (PTSS_MAX_SIZE >> 1)
>>> +
>>> +struct MkvTimestampContext {
>>> +    int64_t ptss[PTSS_MAX_SIZE];
>>> +    size_t ptss_size;
>>> +};
>>>  
>>>  static int write_header(AVFormatContext *s)
>>>  {
>>> -    static const char *header = "# timecode format v2\n";
>>> +    static const char *header = "# timestamp format v2\n";
>>>      avio_write(s->pb, header, strlen(header));
>>>      avpriv_set_pts_info(s->streams[0], 64, 1, 1000);
>>> +
>>>      return 0;
>>>  }
>>>  
>>> +static int cmp_int64(const void *p1, const void *p2)
>>> +{
>>> +    int64_t left  = *(const int64_t *)p1;
>>> +    int64_t right = *(const int64_t *)p2;
>>> +    return FFDIFFSIGN(left, right);
>>> +}
>>> +
>>>  static int write_packet(AVFormatContext *s, AVPacket *pkt)
>>>  {
>>>      char buf[256];
>>> +    int i;
>>> +    struct MkvTimestampContext *m = s->priv_data;
>>> +
>>>      if (pkt->stream_index)
>>>          av_log(s, AV_LOG_WARNING, "More than one stream unsupported\n");
>>> -    snprintf(buf, sizeof(buf), "%" PRId64 "\n", pkt->dts);
>>> -    avio_write(s->pb, buf, strlen(buf));
>>> +
>>> +    if (pkt->pts == AV_NOPTS_VALUE) {
>>> +        av_log(s, AV_LOG_WARNING, "Found PTS with no value, ignored\n");
>>> +        return 0;
>>> +    }
>>> +
>>> +    if (m->ptss_size > PTSS_MAX_SIZE) {
>>> +        // sort all PTSs
>>> +        AV_QSORT(m->ptss, PTSS_MAX_SIZE, int64_t, cmp_int64);
>>> +
>>> +        // write only the first half and copy the second half to the
>>> +        // beginning of the array
>>> +        for (i = 0; i < PTSS_HALF_SIZE; i++) {
>>> +            snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
>>> +            avio_write(s->pb, buf, strlen(buf));
>>> +            m->ptss[i] = m->ptss[i + PTSS_HALF_SIZE];
>>> +        }
>>> +
>>> +        m->ptss_size = PTSS_HALF_SIZE;
>>> +    } else {
>>> +        m->ptss[m->ptss_size++] = pkt->pts;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int write_trailer(struct AVFormatContext *s)
>>> +{
>>> +    struct MkvTimestampContext *m = s->priv_data;
>>> +    char buf[256];
>>> +    int i;
>>> +
>>> +    // sort all PTSs
>>> +    AV_QSORT(m->ptss, m->ptss_size, int64_t, cmp_int64);
>>> +
>>> +    /* flush remaining timestamps */
>>> +    for (i = 0; i < m->ptss_size; i++) {
>>> +        snprintf(buf, sizeof(buf), "%" PRId64 "\n", m->ptss[i]);
>>> +        avio_write(s->pb, buf, strlen(buf));
>>> +    }
>>> +
>>>      return 0;
>>>  }
>>>  
>>>  const FFOutputFormat ff_mkvtimestamp_v2_muxer = {
>>>      .p.name        = "mkvtimestamp_v2",
>>> -    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timecode format"),
>>> +    .p.long_name   = NULL_IF_CONFIG_SMALL("mkvtoolnix v2 timestamp format"),
>>>      .p.audio_codec = AV_CODEC_ID_NONE,
>>>      .p.video_codec = AV_CODEC_ID_RAWVIDEO,
>>>      .write_header = write_header,
>>>      .write_packet = write_packet,
>>> +    .write_trailer = write_trailer,
>>> +    .priv_data_size = sizeof(struct MkvTimestampContext),
>>>  };
>>
> 
>> 1. This does not match mkvextract behaviour. mkvextract does not force a
>> 1ms timebase.
> 
> From your past comment:
>> The accuracy of the timestamps output by mkvextract is determined by the
>> TimestampScale of the file in question; it is most often 1ms when the
>> file has video.
> 

"most often" != "force"

> Note that this is only used for video. I don't know how this
> TimestampScale is computed, but I wonder what was the point of this
> muxer. Probably it was only used to extract the timestamps with a
> fixed timescale, and the fact that it was similar to mkvextract was
> not really an important factor.
> 

mkvextract simply shows the timestamps as decimal number in ms (given
that the timebase of Matroska files is always a multiple of 1
nanosecond, at most six digits after the decimal point are necessary for
this). One would need to remove the avpriv_set_pts_info and convert the
numbers to fractional ms representation to do likewise.

>> 2. AV_QSORT should only be used in speed-critical code, which this here
>> is definitely not.
> 
> Why? What should I use instead?

qsort

> 
>> 3. I still don't think that this muxer should exist at all.
> 
> I tend to agree. But:
> 
> We don't really know why this weird muxer was added, today we have
> better tools for that (we could use ffprobe, or even a bitstream
> filter similar to showinfo to get the same result), but if it was
> added probably there was a reason. So my plan is to make the format a
> bit more useful (DTS => PTS+sort), and possibly deprecate it and point
> to better available tools and drop this in two major releses.
> 
> I don't think the point of the format was to really make the behavior
> exactly equal to mkvextract, the thing with TimeScale is probably not
> very important, at least for the original author that was not really
> an issue, he was probably only looking for some way to dump timestamps
> and took free inspiration from mkvtoolnix.
> 

This thing has been added in 1c5670dbb204369477ee1b5d967f9e8b4f4a33b8. I
can't find any discussion in the mailing list archives for this, but the
file description was "extract pts as timecode v2, as defined by
mkvtoolnix" at the time. Furthermore, its author is the one who started
the Matroska muxer. So I think its aim was really to mimic mkvextract.
(I am not certain wrt MKVToolNix handling of fractional millisecond; old
versions of mkvextract may really have simply rounded/truncated to
milliseconds.)

> If this is true, we might point that this format is not exactly
> equivalent to timestamp_v2 in the doc.

Which makes this thing even more pointless.

- Andreas

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

Re: [FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior

[Stefano Sabatini]

On date Saturday 2024-04-20 18:47:58 +0200, Andreas Rheinhardt wrote:
> Stefano Sabatini:
> > On date Saturday 2024-04-20 15:18:39 +0200, Andreas Rheinhardt wrote:
> >> Stefano Sabatini:
> >>> Harmonize internal implementation with the mkvextract behavior:
> >>> - print PTS in place of DTS values
> >>> - ignore NOPTS values
> >>> - sort PTS values
> >>> ---
> >>>  libavformat/mkvtimestamp_v2.c | 69 +++++++++++++++++++++++++++++++++--
> >>>  1 file changed, 65 insertions(+), 4 deletions(-)
[...]
> >> 3. I still don't think that this muxer should exist at all.
> > 
> > I tend to agree. But:
> > 
> > We don't really know why this weird muxer was added, today we have
> > better tools for that (we could use ffprobe, or even a bitstream
> > filter similar to showinfo to get the same result), but if it was
> > added probably there was a reason. So my plan is to make the format a
> > bit more useful (DTS => PTS+sort), and possibly deprecate it and point
> > to better available tools and drop this in two major releses.
> > 
> > I don't think the point of the format was to really make the behavior
> > exactly equal to mkvextract, the thing with TimeScale is probably not
> > very important, at least for the original author that was not really
> > an issue, he was probably only looking for some way to dump timestamps
> > and took free inspiration from mkvtoolnix.
> > 
> 
> This thing has been added in 1c5670dbb204369477ee1b5d967f9e8b4f4a33b8. I
> can't find any discussion in the mailing list archives for this, but the
> file description was "extract pts as timecode v2, as defined by
> mkvtoolnix" at the time. Furthermore, its author is the one who started
> the Matroska muxer. So I think its aim was really to mimic mkvextract.
> (I am not certain wrt MKVToolNix handling of fractional millisecond; old
> versions of mkvextract may really have simply rounded/truncated to
> milliseconds.)
> 
> > If this is true, we might point that this format is not exactly
> > equivalent to timestamp_v2 in the doc.
> 
> Which makes this thing even more pointless.

I'm not against removing this muxer, but it is something we should do?
Removing a working component (even if suboptimal) even without
deprecation. Probably if there is agreement about this, especially
given that there are better alternatives at this time.

If not, I can fix the missing bits so we have a better implementation,
but we might want to deprecated and drop later.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior

[Stefano Sabatini]

On date Saturday 2024-04-20 18:47:58 +0200, Andreas Rheinhardt wrote:
> Stefano Sabatini:
[...]
> >> 1. This does not match mkvextract behaviour. mkvextract does not force a
> >> 1ms timebase.
> > 
> > From your past comment:
> >> The accuracy of the timestamps output by mkvextract is determined by the
> >> TimestampScale of the file in question; it is most often 1ms when the
> >> file has video.
> > 
> 
> "most often" != "force"
[...]

> (I am not certain wrt MKVToolNix handling of fractional millisecond; old
> versions of mkvextract may really have simply rounded/truncated to
> milliseconds.)

It doesn't, at least with version:
$ mkvextract --version
mkvextract v65.0.0 ('Too Much') 64-bit

So far, mkvextract seems to output DTSs as in the original
implementation - therefore no need to change implementation, but for
the "timecode" => "timestamp" issue.

$ ./ffprobe -hide_banner slow.mkv -of csv=nk=1:p=0 -select_streams v:0 -show_entries packet=pts  | head -n 20 
[...]
0
1201
1235
1268
1368
1301
1335
1401
1435
1535
1468
1502
1602
1568
1735
1668
1635
1702
1768
1869

$ ./ffprobe -hide_banner slow.mkv -of csv=nk=1:p=0 -select_streams v:0 -show_entries packet=dts  | head -n 20 
N/A
N/A
0
1201
1235
1268
1301
1335
1368
1401
1435
1468
1502
1535
1568
1602
1635
1668
1702
1735

$ mkvextract slow.mkv timestamps_v2 0:slow.mkv.out
Progress: 100%
$ head -n 21 slow.mkv.out 
# timestamp format v2
0
1201
1235
1268
1301
1335
1368
1401
1435
1468
1502
1535
1568
1602
1635
1668
1702
1735
1768
1802
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior

[Andreas Rheinhardt]

Stefano Sabatini:
> On date Saturday 2024-04-20 18:47:58 +0200, Andreas Rheinhardt wrote:
>> Stefano Sabatini:
> [...]
>>>> 1. This does not match mkvextract behaviour. mkvextract does not force a
>>>> 1ms timebase.
>>>
>>> From your past comment:
>>>> The accuracy of the timestamps output by mkvextract is determined by the
>>>> TimestampScale of the file in question; it is most often 1ms when the
>>>> file has video.
>>>
>>
>> "most often" != "force"
> [...]
> 
>> (I am not certain wrt MKVToolNix handling of fractional millisecond; old
>> versions of mkvextract may really have simply rounded/truncated to
>> milliseconds.)
> 
> It doesn't, at least with version:
> $ mkvextract --version
> mkvextract v65.0.0 ('Too Much') 64-bit
> 

With old I actually meant really old. As old as this muxer.

> So far, mkvextract seems to output DTSs as in the original
> implementation - therefore no need to change implementation, but for
> the "timecode" => "timestamp" issue.
> 

What is the TimestampScale of said file? Given that we are dealing with
a video stream it is highly likely to be 1ms, so that there will be no
difference.

The following is from a 44100Hz with 4608 samples per packet and a
TimestampScale of 22674 (equivalent to a timebase of 22674/1000000000 s).

$ mkvextract fl.mka timestamps_v2 0:fl.txt
Progress: 100%
$ head -n 7 fl.txt
# timestamp format v2
0
104.489795
208.97959
313.469385
417.95918
522.448975

(The following is with the current implementation, but your suggested
patch won't change anything for it.)

$ ./ffmpeg -i fl.mka -map 0:a -c copy -f mkvtimestamp_v2 - 2>/dev/null |
head -7
# timecode format v2
0
104
209
313
418
522

$ ./ffprobe -hide_banner fl.mka -of csv=nk=1:p=0 -select_streams a:0
-show_entries packet=pts 2>/dev/null | head -n 6
0
4608
9216
13824
18433
23041

$ ./ffprobe -hide_banner fl.mka -of csv=nk=1:p=0 -select_streams a:0
-show_entries packet=pts_time 2>/dev/null | head -n 6
0.000000
0.104482
0.208964
0.313445
0.417950
0.522432

(The slight differences here with mkvextract is due to lacing:
mkvextract calculates the timestamps of the non-leading packets in a
lace with the default duration and uses nanosecond precision for this.)

> $ ./ffprobe -hide_banner slow.mkv -of csv=nk=1:p=0 -select_streams v:0 -show_entries packet=pts  | head -n 20 
> [...]
> 0
> 1201
> 1235
> 1268
> 1368
> 1301
> 1335
> 1401
> 1435
> 1535
> 1468
> 1502
> 1602
> 1568
> 1735
> 1668
> 1635
> 1702
> 1768
> 1869
> 
> $ ./ffprobe -hide_banner slow.mkv -of csv=nk=1:p=0 -select_streams v:0 -show_entries packet=dts  | head -n 20 
> N/A
> N/A
> 0
> 1201
> 1235
> 1268
> 1301
> 1335
> 1368
> 1401
> 1435
> 1468
> 1502
> 1535
> 1568
> 1602
> 1635
> 1668
> 1702
> 1735
> 
> $ mkvextract slow.mkv timestamps_v2 0:slow.mkv.out
> Progress: 100%
> $ head -n 21 slow.mkv.out 
> # timestamp format v2
> 0
> 1201
> 1235
> 1268
> 1301
> 1335
> 1368
> 1401
> 1435
> 1468
> 1502
> 1535
> 1568
> 1602
> 1635
> 1668
> 1702
> 1735
> 1768
> 1802

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

[FFmpeg-devel] [PATCH 3/3] doc/muxers: add mkvtimestamp_v2

[Stefano Sabatini]

---
 doc/muxers.texi | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/doc/muxers.texi b/doc/muxers.texi
index 6340c8e54d..4cd53a4449 100644
--- a/doc/muxers.texi
+++ b/doc/muxers.texi
@@ -2933,14 +2933,13 @@ MicroDVD subtitle format muxer.
 
 This muxer accepts a single @samp{microdvd} subtitles stream.
 
-@section mmf
-Synthetic music Mobile Application Format (SMAF) format muxer.
+@section mkvtimestamp_v2
+mkvtoolnix v2 timestamp format muxer.
 
-SMAF is a music data format specified by Yamaha for portable
-electronic devices, such as mobile phones and personal digital
-assistants.
+Write the PTS rawvideo frame to the output, as implemented by the
+@command{mkvextract} tool from the @command{mkvtoolnix} suite.
 
-This muxer accepts a single @samp{adpcm_yamaha} audio stream.
+This muxer accepts a single @samp{rawvideo} stream.
 
 @section mp3
 
-- 
2.34.1

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

Re: [FFmpeg-devel] [PATCH 3/3] doc/muxers: add mkvtimestamp_v2

[Stefano Sabatini]

On date Saturday 2024-04-20 13:48:35 +0200, Stefano Sabatini wrote:
> ---
>  doc/muxers.texi | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

Sorry, discard this in favor of the new patch.
_______________________________________________
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".