From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id D95E64986B for ; Sat, 20 Apr 2024 16:00:39 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1B25C68D208; Sat, 20 Apr 2024 19:00:37 +0300 (EEST) Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id B9F0B688189 for ; Sat, 20 Apr 2024 19:00:30 +0300 (EEST) Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-a52aa665747so353351566b.2 for ; Sat, 20 Apr 2024 09:00:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1713628829; x=1714233629; darn=ffmpeg.org; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:to:from:date:from:to:cc:subject :date:message-id:reply-to; bh=wq6WGzseiMDRs5ZYJcLXisG3x6jGNwUhF8NWQ7eSBWY=; b=BKoIpYmCS72mLC568nQvgIomGZWn2GiLI3nvNjnw1WoDM/kLdFOzQJf1r0LdY02GDd hkM1MZMImXZfcPbJUPPI93C6nImg3nIOFUfFRmT9itB2E8SIXMRtn5pqc85vhS456A76 +udFfuSLBd9tZ9vRc7qGz94/e+4ChaXacya7PA57x3KkSY95NL+dh3sQewtnegxcy/84 gUoutWNPDQ+RfO3MeZHCX4lfDnX7bBAWEhfy75Hv6Dz/T/XjDA9fbbqkt7F51nb3Ckeq gdaFmmqI8aSnz7AsNI0tggHlZ3+oO1qhFZh19pIrwdZyDa6W0RWZQAUgk9Ul0y+43xfr nXDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713628829; x=1714233629; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=wq6WGzseiMDRs5ZYJcLXisG3x6jGNwUhF8NWQ7eSBWY=; b=hbltG75sUKWuFF1pEQYeeO9gltPgQaElRzmhv3ESn8smxkFXHtrjn5TV+tIRK0z9A2 YDocS1WN4toMskKFpWKJWBIdas+RjNvMk/GxfSC83UAsZL91yZOMqgi8Acxn+M920Mve BnoO9WVb8kNm7uz3DjfcyKBEibWGR+RDBZqaQopXsm32r2n5KEKqDM3KPFibBtkw/2fG kFEOer/aniycqNxrES12Y6eNnsePFQWzAZ3IwmOedUYGcyhkAjzMXUP5X0y6Omm7Z/gt u14n66lGZabN6R4aAZGrWKTa5pzq++gYyF9zSXI/Dy80Yz5Jmn7uQ2eSUn26hjCVujH7 mbWQ== X-Gm-Message-State: AOJu0YwCdNU9CflvFmvDSqc+ChQ63vhrNXA/8gGiEKcUMzZ6rYfis/N0 OjIrcKXV7NKkPGwOBrqq5Nhdug6VfJbzAdJr4mI7jNAcnxzidoIhYvmDsw== X-Google-Smtp-Source: AGHT+IFi4iEXUaU3P0vlJLSFUrj85bLKyPDCHlrlFVd46dm1knyeEmoZ/vmlNYmZPm05P3IOJcZYzA== X-Received: by 2002:a17:907:78c2:b0:a52:33bb:afe6 with SMTP id kv2-20020a17090778c200b00a5233bbafe6mr3240860ejc.74.1713628828936; Sat, 20 Apr 2024 09:00:28 -0700 (PDT) Received: from mariano (host-87-17-49-61.retail.telecomitalia.it. [87.17.49.61]) by smtp.gmail.com with ESMTPSA id ah2-20020a1709069ac200b00a4e393b6349sm3515200ejc.5.2024.04.20.09.00.28 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 Apr 2024 09:00:28 -0700 (PDT) Received: by mariano (Postfix, from userid 1000) id 4B338BFCE8; Sat, 20 Apr 2024 18:00:27 +0200 (CEST) Date: Sat, 20 Apr 2024 18:00:27 +0200 From: Stefano Sabatini To: FFmpeg development discussions and patches Message-ID: Mail-Followup-To: FFmpeg development discussions and patches References: <20240420114835.35645-1-stefasab@gmail.com> <20240420114835.35645-3-stefasab@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.1.4 (2021-12-11) Subject: Re: [FFmpeg-devel] [PATCH 2/3] lavf/mkvtimestamp_v2: review implementation to match mkvextract behavior X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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".