[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