[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