[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.