PR #25554 opened by grapemane URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25554 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25554.patch
The sample is 1 s of 32 kbps mono with a Xing/Info tag, followed by a 4 KB APEv2 tag and an ID3v1 tag, so the trailing tags are larger than 1/16 of the audio. It must report the Info tag's 1.000 s, not a bitrate estimate. Sample: gapless/trailing-ape-tag.mp3 (8790 bytes), to be added to fate-suite. Co-Authored-By: Claude Opus 5.5 <[email protected]> >From 7b07c00350782218b5a6d46fb4582e87b39da113 Mon Sep 17 00:00:00 2001 From: grapemane <[email protected]> Date: Tue, 27 Oct 2026 23:02:36 -0400 Subject: [PATCH 1/2] avformat/mp3dec: exclude trailing ID3v1/APE tags from the Xing size check mp3_parse_info_tag() compares the Xing/Info byte count with the file size from the first frame to EOF, and if the file is more than 1/16 larger it assumes a concatenated file, drops the frame count and estimates the duration from the bitrate. The file size includes any ID3v1 and APEv2 tag at the end, which the Info byte count never covers, so a file with large trailing tags (an APE tag holding cover art is common) gets a duration that is too long by the tag size over the bitrate, and loses gapless trimming. Seen on a 320 kbps file with a 1.2 MB APE tag: 155.29 s reported, 125.15 s decoded. Subtract the trailing tags before the comparison. A genuinely concatenated file still trips the check. Move the APE tag flag defines to apetag.h so mp3dec can read the footer. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- libavformat/apetag.c | 5 ----- libavformat/apetag.h | 5 +++++ libavformat/mp3dec.c | 39 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/libavformat/apetag.c b/libavformat/apetag.c index 84645607de4..dd6ce619c91 100644 --- a/libavformat/apetag.c +++ b/libavformat/apetag.c @@ -31,11 +31,6 @@ #include "internal.h" #include "mux.h" -#define APE_TAG_FLAG_CONTAINS_HEADER (1U << 31) -#define APE_TAG_FLAG_LACKS_FOOTER (1 << 30) -#define APE_TAG_FLAG_IS_HEADER (1 << 29) -#define APE_TAG_FLAG_IS_BINARY (1 << 1) - static int ape_tag_read_field(AVFormatContext *s) { AVIOContext *pb = s->pb; diff --git a/libavformat/apetag.h b/libavformat/apetag.h index 49cd10eb01d..3b9a2842beb 100644 --- a/libavformat/apetag.h +++ b/libavformat/apetag.h @@ -30,6 +30,11 @@ #define APE_TAG_FOOTER_BYTES 32 #define APE_TAG_HEADER_BYTES 32 +#define APE_TAG_FLAG_CONTAINS_HEADER (1U << 31) +#define APE_TAG_FLAG_LACKS_FOOTER (1 << 30) +#define APE_TAG_FLAG_IS_HEADER (1 << 29) +#define APE_TAG_FLAG_IS_BINARY (1 << 1) + /** * Read and parse an APE tag * diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c index d326b9947fa..fa2b0fe43d1 100644 --- a/libavformat/mp3dec.c +++ b/libavformat/mp3dec.c @@ -29,6 +29,7 @@ #include "avformat.h" #include "internal.h" #include "avio_internal.h" +#include "apetag.h" #include "demux.h" #include "id3v2.h" #include "id3v1.h" @@ -49,6 +50,7 @@ typedef struct { AVClass *class; int64_t filesize; + int64_t trailing_tags_size; ///< ID3v1 and APEv2 tags after the last frame int xing_toc; int start_pad; int end_pad; @@ -161,6 +163,37 @@ static void read_xing_toc(AVFormatContext *s, int64_t filesize, int64_t duration mp3->xing_toc = 1; } +/** + * Size of the ID3v1 and APEv2 tags at the end of the file. They are not part + * of the MPEG stream, so the Xing/Info byte count does not include them. + */ +static int64_t mp3_trailing_tags_size(AVIOContext *pb, int64_t end) +{ + int64_t pos = avio_tell(pb), size = 0; + uint8_t buf[APE_TAG_FOOTER_BYTES]; + + if (end > ID3v1_TAG_SIZE && + avio_seek(pb, end - ID3v1_TAG_SIZE, SEEK_SET) >= 0 && + avio_read(pb, buf, 3) == 3 && !memcmp(buf, "TAG", 3)) + size = ID3v1_TAG_SIZE; + + if (end - size > APE_TAG_FOOTER_BYTES && + avio_seek(pb, end - size - APE_TAG_FOOTER_BYTES, SEEK_SET) >= 0 && + avio_read(pb, buf, APE_TAG_FOOTER_BYTES) == APE_TAG_FOOTER_BYTES && + !memcmp(buf, APE_TAG_PREAMBLE, 8) && + !(AV_RL32(buf + 20) & APE_TAG_FLAG_IS_HEADER)) { + /* the size field counts the items and the footer, not the header */ + int64_t tag_bytes = AV_RL32(buf + 12); + if (AV_RL32(buf + 20) & APE_TAG_FLAG_CONTAINS_HEADER) + tag_bytes += APE_TAG_HEADER_BYTES; + if (tag_bytes >= APE_TAG_FOOTER_BYTES && tag_bytes <= end - size) + size += tag_bytes; + } + + avio_seek(pb, pos, SEEK_SET); + return size; +} + static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader2 *c, uint32_t spf) { @@ -181,6 +214,7 @@ static void mp3_parse_info_tag(AVFormatContext *s, AVStream *st, uint64_t fsize = avio_size(s->pb); int64_t pos = avio_tell(s->pb); fsize = fsize >= pos ? fsize - pos : 0; + fsize -= FFMIN(fsize, mp3->trailing_tags_size); /* Check for Xing / Info tag */ avio_skip(s->pb, xing_offtbl[c->lsf == 1][c->nb_channels == 1]); @@ -482,8 +516,11 @@ static int mp3_read_header(AVFormatContext *s) if (!av_dict_count(s->metadata)) ff_id3v1_read(s); - if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) + if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) { mp3->filesize = avio_size(s->pb); + /* before mp3_parse_vbr_tags() starts the Info tag CRC */ + mp3->trailing_tags_size = mp3_trailing_tags_size(s->pb, mp3->filesize); + } if (mp3_parse_vbr_tags(s, st, off) < 0) avio_seek(s->pb, off, SEEK_SET); -- 2.54.0 >From 37bda32a5e84d25fd327782666d945c1360a5393 Mon Sep 17 00:00:00 2001 From: grapemane <[email protected]> Date: Tue, 27 Oct 2026 23:08:02 -0400 Subject: [PATCH 2/2] fate/gapless: add a test for an MP3 with a trailing APE tag The sample is 1 s of 32 kbps mono with a Xing/Info tag, followed by a 4 KB APEv2 tag and an ID3v1 tag, so the trailing tags are larger than 1/16 of the audio. It must report the Info tag's 1.000 s, not a bitrate estimate. Sample: gapless/trailing-ape-tag.mp3 (8790 bytes), to be added to fate-suite. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- tests/fate/gapless.mak | 5 +++++ tests/ref/fate/gapless-mp3-trailing-ape | 8 ++++++++ 2 files changed, 13 insertions(+) create mode 100644 tests/ref/fate/gapless-mp3-trailing-ape diff --git a/tests/fate/gapless.mak b/tests/fate/gapless.mak index 708c98eb2e3..1cdf68d08a7 100644 --- a/tests/fate/gapless.mak +++ b/tests/fate/gapless.mak @@ -4,6 +4,11 @@ fate-gapless-mp3: CMD = gapless $(TARGET_SAMPLES)/gapless/gapless.mp3 "-c:a mp3" FATE_GAPLESSINFO_PROBE-$(call ALLYES, MP3_DEMUXER MP3FLOAT_DECODER FILE_PROTOCOL) += fate-gapless-mp3-side-data fate-gapless-mp3-side-data: CMD = ffprobe_demux $(TARGET_SAMPLES)/gapless/gapless.mp3 +# A trailing APE tag larger than 1/16 of the audio is not a concatenated file: +# the duration still comes from the Info tag's frame count. +FATE_GAPLESSINFO_PROBE-$(call ALLYES, MP3_DEMUXER MP3FLOAT_DECODER FILE_PROTOCOL) += fate-gapless-mp3-trailing-ape +fate-gapless-mp3-trailing-ape: CMD = probe -show_entries stream=start_time,duration_ts,duration:format=duration $(TARGET_SAMPLES)/gapless/trailing-ape-tag.mp3 + # The trailing padding of this sample spans two packets. The duration of the # remuxed file has to stay the one fate-gapless-mp3-side-data reports. FATE_GAPLESSENC_PROBE-$(call ALLYES, MP3_DEMUXER MP3_MUXER NULL_MUXER) += fate-gapless-mp3-remux diff --git a/tests/ref/fate/gapless-mp3-trailing-ape b/tests/ref/fate/gapless-mp3-trailing-ape new file mode 100644 index 00000000000..4ff52a114dc --- /dev/null +++ b/tests/ref/fate/gapless-mp3-trailing-ape @@ -0,0 +1,8 @@ +[STREAM] +start_time=0.025057 +duration_ts=14112000 +duration=1.000000 +[/STREAM] +[FORMAT] +duration=1.000000 +[/FORMAT] -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
