PR #25374 opened by Jun Zhao (mypopydev) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25374 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25374.patch
write_chapter() and read_chapter() always used plain 32-bit integers for CHAP and embedded subframe sizes, even in ID3v2.4 where frame sizes are synchsafe, causing chapters with subframes of 128 bytes or more to be dropped. Fix both paths to honor the ID3v2 version, keep chapters when a subframe size is invalid, and add a sample-free FATE round-trip test for v2.3 and v2.4. >From e828b52165440b885d3faf48fd70530fc8fd28ea Mon Sep 17 00:00:00 2001 From: Jun Zhao <[email protected]> Date: Fri, 23 Oct 2026 07:06:02 +0800 Subject: [PATCH 1/2] avformat/id3v2: honor the tag version in CHAP frame sizes ID3v2.3 stores a frame size as a plain 32-bit integer. ID3v2.4 stores it as a synchsafe integer. write_chapter() always wrote the plain form, and read_chapter() always read it that way. Once an embedded text frame reached 128 bytes, the size no longer fitted and the chapter was dropped. Write the CHAP header with the same version rule as other frames. Read an embedded frame with that rule, and keep the chapter when a subframe size does not fit. Fixes: https://code.ffmpeg.org/FFmpeg/FFmpeg/issues/25357 Signed-off-by: Jun Zhao <[email protected]> --- libavformat/id3v2.c | 42 ++++++++++++++++++++++++++---------------- libavformat/id3v2enc.c | 26 ++++++++++++++++---------- 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 5ae931da9ba..59493033db3 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -487,7 +487,7 @@ static void list_append(ID3v2ExtraMeta *new_elem, ExtraMetaList *list) * Parse GEOB tag into a ID3v2ExtraMetaGEOB struct. */ static void read_geobtag(AVFormatContext *s, AVIOContext *pb, int taglen, - const char *tag, ExtraMetaList *extra_meta, int isv34) + const char *tag, ExtraMetaList *extra_meta, int version) { ID3v2ExtraMetaGEOB *geob_data = NULL; ID3v2ExtraMeta *new_extra = NULL; @@ -618,7 +618,7 @@ static void rstrip_spaces(char *buf) } static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen, - const char *tag, ExtraMetaList *extra_meta, int isv34) + const char *tag, ExtraMetaList *extra_meta, int version) { int enc, pic_type; char mimetype[64] = {0}; @@ -628,7 +628,7 @@ static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen, ID3v2ExtraMeta *new_extra = NULL; int64_t end = avio_tell(pb) + taglen; - if (taglen <= 4 || (!isv34 && taglen <= 6)) + if (taglen <= 4 || (version == 2 && taglen <= 6)) goto fail; new_extra = av_mallocz(sizeof(*new_extra)); @@ -641,7 +641,7 @@ static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen, taglen--; /* mimetype */ - if (isv34) { + if (version != 2) { int ret = avio_get_str(pb, taglen, mimetype, sizeof(mimetype)); if (ret < 0 || ret >= taglen) goto fail; @@ -714,9 +714,8 @@ static void free_chapter(void *obj) } static void read_chapter(AVFormatContext *s, AVIOContext *pb, int len, - const char *ttag, ExtraMetaList *extra_meta, int isv34) + const char *ttag, ExtraMetaList *extra_meta, int version) { - int taglen; char tag[5]; ID3v2ExtraMeta *new_extra = NULL; ID3v2ExtraMetaCHAP *chap = NULL; @@ -739,19 +738,30 @@ static void read_chapter(AVFormatContext *s, AVIOContext *pb, int len, len -= 16; while (len > 10) { + unsigned tlen; + if (avio_read(pb, tag, 4) < 4) - goto fail; + break; tag[4] = 0; - taglen = avio_rb32(pb); + /* Embedded frames use the same header as the surrounding tag. + * 2.2 has no CHAP. 2.3 sizes are plain; 2.4 sizes are synchsafe. */ + if (version == 4) + tlen = get_size(pb, 4); + else + tlen = avio_rb32(pb); avio_skip(pb, 2); len -= 10; - if (taglen < 0 || taglen > len) - goto fail; + if (tlen > (unsigned)len) { + av_log(s, AV_LOG_WARNING, + "Invalid ID3v2 chapter subframe %.4s size %u, keeping the chapter\n", + tag, tlen); + break; + } if (tag[0] == 'T') - read_ttag(s, pb, taglen, &chap->meta, tag); + read_ttag(s, pb, tlen, &chap->meta, tag); else - avio_skip(pb, taglen); - len -= taglen; + avio_skip(pb, tlen); + len -= tlen; } ff_metadata_conv(&chap->meta, NULL, ff_id3v2_34_metadata_conv); @@ -775,7 +785,7 @@ static void free_priv(void *obj) } static void read_priv(AVFormatContext *s, AVIOContext *pb, int taglen, - const char *tag, ExtraMetaList *extra_meta, int isv34) + const char *tag, ExtraMetaList *extra_meta, int version) { ID3v2ExtraMeta *meta; ID3v2ExtraMetaPRIV *priv; @@ -813,7 +823,7 @@ typedef struct ID3v2EMFunc { const char *tag4; void (*read)(AVFormatContext *s, AVIOContext *pb, int taglen, const char *tag, ExtraMetaList *extra_meta, - int isv34); + int version); void (*free)(void *obj); } ID3v2EMFunc; @@ -1092,7 +1102,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, read_lang_descr_tag(s, pbx, "comment", tlen, metadata); else /* parse special meta tag */ - extra_func->read(s, pbx, tlen, tag, extra_meta, isv34); + extra_func->read(s, pbx, tlen, tag, extra_meta, version); } else if (!tag[0]) { if (tag[1]) av_log(s, AV_LOG_WARNING, "invalid frame id, assuming padding\n"); diff --git a/libavformat/id3v2enc.c b/libavformat/id3v2enc.c index 9206af88379..c538d13829b 100644 --- a/libavformat/id3v2enc.c +++ b/libavformat/id3v2enc.c @@ -60,6 +60,18 @@ static void id3v2_encode_string(AVIOContext *pb, const uint8_t *str, put(pb, str); } +/* ID3v2.3 frame size is not synchsafe; 2.4 is. */ +static void id3v2_put_frame_header(ID3v2EncContext *id3, AVIOContext *pb, + uint32_t tag, int len, int flags) +{ + avio_wb32(pb, tag); + if (id3->version == 3) + avio_wb32(pb, len); + else + id3v2_put_size(pb, len); + avio_wb16(pb, flags); +} + static int id3v2_put_frame(ID3v2EncContext *id3, AVIOContext *avioc, AVIOContext *dyn_buf, const uint32_t tag, const uint8_t flags) { @@ -67,13 +79,7 @@ static int id3v2_put_frame(ID3v2EncContext *id3, AVIOContext *avioc, AVIOContext uint8_t *pb; len = avio_get_dyn_buf(dyn_buf, &pb); - avio_wb32(avioc, tag); - /* ID3v2.3 frame size is not sync-safe */ - if (id3->version == 3) - avio_wb32(avioc, len); - else - id3v2_put_size(avioc, len); - avio_wb16(avioc, flags); + id3v2_put_frame_header(id3, avioc, tag, len, flags); avio_write(avioc, pb, len); id3->len += len + ID3v2_HEADER_SIZE; @@ -463,9 +469,9 @@ static int write_chapter(AVFormatContext *s, AVIOContext *pb, len = avio_get_dyn_buf(dyn_bc, &dyn_buf); id3->len += 16 + ID3v2_HEADER_SIZE; - avio_wb32(pb, MKBETAG('C', 'H', 'A', 'P')); - avio_wb32(pb, len); - avio_wb16(pb, 0); + /* Nested frames already updated id3->len, so this header is written + * directly instead of through id3v2_put_frame(). */ + id3v2_put_frame_header(id3, pb, MKBETAG('C', 'H', 'A', 'P'), len, 0); avio_write(pb, dyn_buf, len); fail: -- 2.54.0 >From eb60ad273552c36749781ea6834df3f7f5084b59 Mon Sep 17 00:00:00 2001 From: Jun Zhao <[email protected]> Date: Fri, 23 Oct 2026 07:06:02 +0800 Subject: [PATCH 2/2] tests/fate: add an ID3v2 chapter size round-trip Cover ID3v2.3 and ID3v2.4 with a chapter title long enough that a plain size and a synchsafe size differ. Check the CHAP size high bit, the declared tag length, and the two chapters read back. Signed-off-by: Jun Zhao <[email protected]> --- libavformat/Makefile | 2 +- libavformat/tests/id3v2chap.c | 105 ++++++++++++++++++++++++++++++++++ tests/fate/libavformat.mak | 4 ++ tests/ref/fate/id3v2chap | 2 + 4 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 libavformat/tests/id3v2chap.c create mode 100644 tests/ref/fate/id3v2chap diff --git a/libavformat/Makefile b/libavformat/Makefile index 61b4cfb26cb..3c875d2da68 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -802,7 +802,7 @@ TESTPROGS-$(CONFIG_HTTP_PROTOCOL) += http TESTPROGS-$(CONFIG_NETWORK) += noproxy TESTPROGS-$(CONFIG_SRTP) += srtp TESTPROGS-$(CONFIG_IMF_DEMUXER) += imf -TESTPROGS-$(CONFIG_MP3_MUXER) += id3v2enc +TESTPROGS-$(CONFIG_MP3_MUXER) += id3v2enc id3v2chap TESTPROGS-$(CONFIG_OGG_MUXER) += ogg_pages TOOLS = aviocat \ diff --git a/libavformat/tests/id3v2chap.c b/libavformat/tests/id3v2chap.c new file mode 100644 index 00000000000..b36c516ddd0 --- /dev/null +++ b/libavformat/tests/id3v2chap.c @@ -0,0 +1,105 @@ +/* + * Copyright (c) 2026 Jun Zhao + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <stdio.h> +#include <string.h> + +#include "libavutil/mem.h" +#include "libavformat/avformat.h" +#include "libavformat/avio_internal.h" +#include "libavformat/demux.h" +#include "libavformat/id3v2.h" + +/* The first chapter title is long enough that its TIT2 frame is at least + * 128 bytes, so a plain size and a synchsafe size differ. */ +static int roundtrip(int version) +{ + char long_title[151]; + AVFormatContext *out = NULL, *in = NULL; + AVIOContext *pb = NULL; + FFIOContext read_pb; + ID3v2EncContext id3 = { 0 }; + ID3v2ExtraMeta *extra = NULL; + AVDictionary *meta = NULL; + const AVDictionaryEntry *t0, *t1; + uint8_t *buf = NULL; + int size, high_bit = -1, ret = 1; + + memset(long_title, 'L', sizeof(long_title) - 1); + long_title[sizeof(long_title) - 1] = 0; + + out = avformat_alloc_context(); + if (!out || + !avpriv_new_chapter(out, 0, av_make_q(1, 1000), 0, 1000, long_title) || + !avpriv_new_chapter(out, 1, av_make_q(1, 1000), 1000, 2000, "End") || + avio_open_dyn_buf(&pb) < 0) + goto done; + + ff_id3v2_start(&id3, pb, version, ID3v2_DEFAULT_MAGIC); + if (ff_id3v2_write_metadata(out, pb, &id3) < 0) + goto done; + ff_id3v2_finish(&id3, pb, 10); + size = avio_close_dyn_buf(pb, &buf); + pb = NULL; + if (size < ID3v2_HEADER_SIZE || ff_id3v2_tag_len(buf) != size) + goto done; + + for (int i = 0; i + 8 <= size; i++) { + if (!memcmp(buf + i, "CHAP", 4)) { + high_bit = (buf[i + 4] | buf[i + 5] | buf[i + 6] | buf[i + 7]) >> 7; + break; + } + } + if (high_bit != (version == 3)) + goto done; + + in = avformat_alloc_context(); + if (!in) + goto done; + ffio_init_read_context(&read_pb, buf, size); + ff_id3v2_read_dict(in, &read_pb.pub, &meta, ID3v2_DEFAULT_MAGIC, &extra); + if (ff_id3v2_parse_chapters(in, extra) < 0 || in->nb_chapters != 2) + goto done; + + t0 = av_dict_get(in->chapters[0]->metadata, "title", NULL, 0); + t1 = av_dict_get(in->chapters[1]->metadata, "title", NULL, 0); + if (!t0 || strcmp(t0->value, long_title) || + !t1 || strcmp(t1->value, "End") || + in->chapters[0]->start != 0 || in->chapters[0]->end != 1000 || + in->chapters[1]->start != 1000 || in->chapters[1]->end != 2000) + goto done; + + printf("version=%d chapters=2 title=150 high_bit=%d\n", version, high_bit); + ret = 0; + +done: + ffio_free_dyn_buf(&pb); + av_free(buf); + av_dict_free(&meta); + ff_id3v2_free_extra_meta(&extra); + avformat_free_context(in); + avformat_free_context(out); + return ret; +} + +int main(void) +{ + return roundtrip(3) || roundtrip(4); +} diff --git a/tests/fate/libavformat.mak b/tests/fate/libavformat.mak index 6459057a311..b8ab012334c 100644 --- a/tests/fate/libavformat.mak +++ b/tests/fate/libavformat.mak @@ -6,6 +6,10 @@ FATE_LIBAVFORMAT-$(CONFIG_MP3_MUXER) += fate-id3v2enc fate-id3v2enc: libavformat/tests/id3v2enc$(EXESUF) fate-id3v2enc: CMD = run libavformat/tests/id3v2enc$(EXESUF) +FATE_LIBAVFORMAT-$(CONFIG_MP3_MUXER) += fate-id3v2chap +fate-id3v2chap: libavformat/tests/id3v2chap$(EXESUF) +fate-id3v2chap: CMD = run libavformat/tests/id3v2chap$(EXESUF) + FATE_LIBAVFORMAT += fate-mkdir fate-mkdir: libavformat/tests/mkdir$(EXESUF) fate-mkdir: CMD = run libavformat/tests/mkdir$(EXESUF) diff --git a/tests/ref/fate/id3v2chap b/tests/ref/fate/id3v2chap new file mode 100644 index 00000000000..1f229a97d52 --- /dev/null +++ b/tests/ref/fate/id3v2chap @@ -0,0 +1,2 @@ +version=3 chapters=2 title=150 high_bit=1 +version=4 chapters=2 title=150 high_bit=0 -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
