[PR] avformat/mp3enc: buffer ID3v2 for non-seekable outputs (PR #23996)

PR #23996 opened by iSoldLeo
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/23996
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/23996.patch

ID3v2 finalization seeks back to update the tag size. After data has been written to a non-seekable output, that seek fails and leaves the tag size at zero. This affects attached pictures and sufficiently large metadata.

Build ID3v2 tags in a dynamic buffer for non-seekable MP3 outputs, then write the finalized tag to the output. Seekable outputs keep the existing path.

Two FATE tests cover non-seekable output with an attached picture and with large metadata padding.

Fixes #23917


>From 9373581d18bc83d160b815a7bda960625b20062d Mon Sep 17 00:00:00 2001
From: iSold Leo <[email protected]>
Date: Mon, 3 Aug 2026 22:48:32 +0800
Subject: [PATCH] avformat/mp3enc: buffer ID3v2 for non-seekable outputs

ID3v2 writes the tag size by seeking back to its header. This fails after output has been flushed to a non-seekable AVIO context, leaving a zero-sized tag. Attached pictures expose this reliably; sufficiently large metadata is affected too.

Build the tag in a dynamic buffer for non-seekable MP3 outputs and copy it after finalization. Keep the direct path for seekable outputs.

Fixes: #23917
---
 libavformat/aiffenc.c    |  4 +--
 libavformat/id3v2.h      |  6 ++--
 libavformat/id3v2enc.c   | 32 +++++++++++---------
 libavformat/mp3enc.c     | 64 ++++++++++++++++++++++++++++++++++------
 tests/fate/cover-art.mak |  6 ++++
 tests/fate/id3v2.mak     |  5 ++++
 6 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/libavformat/aiffenc.c b/libavformat/aiffenc.c
index 3368e28404..e27a2ca5e6 100644
--- a/libavformat/aiffenc.c
+++ b/libavformat/aiffenc.c
@@ -59,9 +59,9 @@ static int put_id3v2_tags(AVFormatContext *s, AIFFOutputContext *aiff)
     pos = avio_tell(pb);
 
     ff_id3v2_start(&id3v2, pb, aiff->id3v2_version, ID3v2_DEFAULT_MAGIC);
-    ff_id3v2_write_metadata(s, &id3v2);
+    ff_id3v2_write_metadata(s, pb, &id3v2);
     while (list_entry) {
-        if ((ret = ff_id3v2_write_apic(s, &id3v2, &list_entry->pkt)) < 0)
+        if ((ret = ff_id3v2_write_apic(s, pb, &id3v2, &list_entry->pkt)) < 0)
             return ret;
         list_entry = list_entry->next;
     }
diff --git a/libavformat/id3v2.h b/libavformat/id3v2.h
index 9afa5a2ddc..f31d9115e8 100644
--- a/libavformat/id3v2.h
+++ b/libavformat/id3v2.h
@@ -137,12 +137,14 @@ void ff_id3v2_start(ID3v2EncContext *id3, AVIOContext *pb, int id3v2_version,
 /**
  * Convert and write all global metadata from s into an ID3v2 tag.
  */
-int ff_id3v2_write_metadata(AVFormatContext *s, ID3v2EncContext *id3);
+int ff_id3v2_write_metadata(AVFormatContext *s, AVIOContext *pb,
+                            ID3v2EncContext *id3);
 
 /**
  * Write an attached picture from pkt into an ID3v2 tag.
  */
-int ff_id3v2_write_apic(AVFormatContext *s, ID3v2EncContext *id3, AVPacket *pkt);
+int ff_id3v2_write_apic(AVFormatContext *s, AVIOContext *pb,
+                        ID3v2EncContext *id3, AVPacket *pkt);
 
 /**
  * Finalize an opened ID3v2 tag.
diff --git a/libavformat/id3v2enc.c b/libavformat/id3v2enc.c
index 12b0400541..015a767202 100644
--- a/libavformat/id3v2enc.c
+++ b/libavformat/id3v2enc.c
@@ -408,7 +408,8 @@ static int write_metadata(AVIOContext *pb, AVDictionary **metadata,
     return 0;
 }
 
-static int write_ctoc(AVFormatContext *s, ID3v2EncContext *id3, int enc)
+static int write_ctoc(AVFormatContext *s, AVIOContext *pb,
+                      ID3v2EncContext *id3, int enc)
 {
     AVIOContext *dyn_bc;
     char name[123];
@@ -428,11 +429,12 @@ static int write_ctoc(AVFormatContext *s, ID3v2EncContext *id3, int enc)
         avio_put_str(dyn_bc, name);
     }
 
-    return id3v2_put_frame(id3, s->pb, dyn_bc,
+    return id3v2_put_frame(id3, pb, dyn_bc,
                            MKBETAG('C', 'T', 'O', 'C'), 0);
 }
 
-static int write_chapter(AVFormatContext *s, ID3v2EncContext *id3, int id, int enc)
+static int write_chapter(AVFormatContext *s, AVIOContext *pb,
+                         ID3v2EncContext *id3, int id, int enc)
 {
     const AVRational time_base = {1, 1000};
     AVChapter *ch = s->chapters[id];
@@ -460,10 +462,10 @@ static int write_chapter(AVFormatContext *s, ID3v2EncContext *id3, int id, int e
     len = avio_get_dyn_buf(dyn_bc, &dyn_buf);
     id3->len += 16 + ID3v2_HEADER_SIZE;
 
-    avio_wb32(s->pb, MKBETAG('C', 'H', 'A', 'P'));
-    avio_wb32(s->pb, len);
-    avio_wb16(s->pb, 0);
-    avio_write(s->pb, dyn_buf, len);
+    avio_wb32(pb, MKBETAG('C', 'H', 'A', 'P'));
+    avio_wb32(pb, len);
+    avio_wb16(pb, 0);
+    avio_write(pb, dyn_buf, len);
 
 fail:
     ffio_free_dyn_buf(&dyn_bc);
@@ -471,28 +473,30 @@ fail:
     return ret;
 }
 
-int ff_id3v2_write_metadata(AVFormatContext *s, ID3v2EncContext *id3)
+int ff_id3v2_write_metadata(AVFormatContext *s, AVIOContext *pb,
+                            ID3v2EncContext *id3)
 {
     int enc = id3->version == 3 ? ID3v2_ENCODING_UTF16BOM :
                                   ID3v2_ENCODING_UTF8;
     int i, ret;
 
     ff_standardize_creation_time(s);
-    if ((ret = write_metadata(s->pb, &s->metadata, id3, enc)) < 0)
+    if ((ret = write_metadata(pb, &s->metadata, id3, enc)) < 0)
         return ret;
 
-    if ((ret = write_ctoc(s, id3, enc)) < 0)
+    if ((ret = write_ctoc(s, pb, id3, enc)) < 0)
         return ret;
 
     for (i = 0; i < s->nb_chapters; i++) {
-        if ((ret = write_chapter(s, id3, i, enc)) < 0)
+        if ((ret = write_chapter(s, pb, id3, i, enc)) < 0)
             return ret;
     }
 
     return 0;
 }
 
-int ff_id3v2_write_apic(AVFormatContext *s, ID3v2EncContext *id3, AVPacket *pkt)
+int ff_id3v2_write_apic(AVFormatContext *s, AVIOContext *pb,
+                        ID3v2EncContext *id3, AVPacket *pkt)
 {
     AVStream *st = s->streams[pkt->stream_index];
     AVDictionaryEntry *e;
@@ -545,7 +549,7 @@ int ff_id3v2_write_apic(AVFormatContext *s, ID3v2EncContext *id3, AVPacket *pkt)
     id3v2_encode_string(dyn_buf, desc, enc);
     avio_write(dyn_buf, pkt->data, pkt->size);
 
-    return id3v2_put_frame(id3, s->pb, dyn_buf,
+    return id3v2_put_frame(id3, pb, dyn_buf,
                            MKBETAG('A', 'P', 'I', 'C'), 0);
 }
 
@@ -580,7 +584,7 @@ int ff_id3v2_write_simple(struct AVFormatContext *s, int id3v2_version,
     int ret;
 
     ff_id3v2_start(&id3, s->pb, id3v2_version, magic);
-    if ((ret = ff_id3v2_write_metadata(s, &id3)) < 0)
+    if ((ret = ff_id3v2_write_metadata(s, s->pb, &id3)) < 0)
         return ret;
     ff_id3v2_finish(&id3, s->pb, s->metadata_header_padding);
 
diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c
index 131fb231fc..796e18d481 100644
--- a/libavformat/mp3enc.c
+++ b/libavformat/mp3enc.c
@@ -100,6 +100,7 @@ static int id3v1_create_tag(AVFormatContext *s, uint8_t *buf)
 typedef struct MP3Context {
     const AVClass *class;
     ID3v2EncContext id3;
+    AVIOContext *id3_pb;
     int id3v2_version;
     int write_id3v1;
     int write_xing;
@@ -379,14 +380,41 @@ static int mp3_write_audio_packet(AVFormatContext *s, AVPacket *pkt)
     return ff_raw_write_packet(s, pkt);
 }
 
+static int mp3_finish_id3v2(AVFormatContext *s)
+{
+    MP3Context *mp3 = s->priv_data;
+    AVIOContext *pb = mp3->id3_pb ? mp3->id3_pb : s->pb;
+    uint8_t *buf = NULL;
+    int ret, size;
+
+    ff_id3v2_finish(&mp3->id3, pb, s->metadata_header_padding);
+    ret = pb->error;
+
+    if (!mp3->id3_pb)
+        return ret;
+
+    size = avio_get_dyn_buf(mp3->id3_pb, &buf);
+    ret = mp3->id3_pb->error;
+    if (ret >= 0) {
+        avio_write(s->pb, buf, size);
+        ret = s->pb->error;
+    }
+    ffio_free_dyn_buf(&mp3->id3_pb);
+
+    return ret;
+}
+
 static int mp3_queue_flush(AVFormatContext *s)
 {
     MP3Context *mp3 = s->priv_data;
     AVPacket *const pkt = ffformatcontext(s)->pkt;
     int ret = 0, write = 1;
 
-    ff_id3v2_finish(&mp3->id3, s->pb, s->metadata_header_padding);
-    mp3_write_xing(s);
+    ret = mp3_finish_id3v2(s);
+    if (ret < 0)
+        write = 0;
+    else
+        mp3_write_xing(s);
 
     while (mp3->queue.head) {
         ff_packet_list_get(&mp3->queue, pkt);
@@ -474,11 +502,13 @@ static int mp3_write_trailer(struct AVFormatContext *s)
 {
     uint8_t buf[ID3v1_TAG_SIZE];
     MP3Context *mp3 = s->priv_data;
+    int ret;
 
     if (mp3->pics_to_write) {
         av_log(s, AV_LOG_WARNING, "No packets were sent for some of the "
                "attached pictures.\n");
-        mp3_queue_flush(s);
+        if ((ret = mp3_queue_flush(s)) < 0)
+            return ret;
     }
 
     /* write the id3v1 tag */
@@ -536,12 +566,14 @@ static int mp3_write_packet(AVFormatContext *s, AVPacket *pkt)
             if (ret < 0) {
                 av_log(s, AV_LOG_WARNING, "Not enough memory to buffer audio. Skipping picture streams\n");
                 mp3->pics_to_write = 0;
-                mp3_queue_flush(s);
+                if ((ret = mp3_queue_flush(s)) < 0)
+                    return ret;
                 return mp3_write_audio_packet(s, pkt);
             }
         } else
             return mp3_write_audio_packet(s, pkt);
     } else {
+        AVIOContext *pb = mp3->id3_pb ? mp3->id3_pb : s->pb;
         int ret;
 
         /* warn only once for each stream */
@@ -552,8 +584,11 @@ static int mp3_write_packet(AVFormatContext *s, AVPacket *pkt)
         if (!mp3->pics_to_write || s->streams[pkt->stream_index]->nb_frames >= 1)
             return 0;
 
-        if ((ret = ff_id3v2_write_apic(s, &mp3->id3, pkt)) < 0)
+        ret = ff_id3v2_write_apic(s, pb, &mp3->id3, pkt);
+        if (ret < 0)
             return ret;
+        if (pb->error < 0)
+            return pb->error;
         mp3->pics_to_write--;
 
         /* flush the buffered audio packets */
@@ -617,18 +652,28 @@ static int mp3_init(struct AVFormatContext *s)
 static int mp3_write_header(struct AVFormatContext *s)
 {
     MP3Context  *mp3 = s->priv_data;
+    AVIOContext *pb = s->pb;
     int ret;
 
     if (mp3->id3v2_version) {
-        ff_id3v2_start(&mp3->id3, s->pb, mp3->id3v2_version, ID3v2_DEFAULT_MAGIC);
-        ret = ff_id3v2_write_metadata(s, &mp3->id3);
+        if (!(s->pb->seekable & AVIO_SEEKABLE_NORMAL)) {
+            ret = avio_open_dyn_buf(&mp3->id3_pb);
+            if (ret < 0)
+                return ret;
+            pb = mp3->id3_pb;
+        }
+
+        ff_id3v2_start(&mp3->id3, pb, mp3->id3v2_version, ID3v2_DEFAULT_MAGIC);
+        ret = ff_id3v2_write_metadata(s, pb, &mp3->id3);
         if (ret < 0)
             return ret;
+        if (pb->error < 0)
+            return pb->error;
     }
 
     if (!mp3->pics_to_write) {
-        if (mp3->id3v2_version)
-            ff_id3v2_finish(&mp3->id3, s->pb, s->metadata_header_padding);
+        if (mp3->id3v2_version && (ret = mp3_finish_id3v2(s)) < 0)
+            return ret;
         mp3_write_xing(s);
     }
 
@@ -640,6 +685,7 @@ static void mp3_deinit(struct AVFormatContext *s)
     MP3Context *mp3 = s->priv_data;
 
     ff_packet_list_free(&mp3->queue);
+    ffio_free_dyn_buf(&mp3->id3_pb);
     av_freep(&mp3->xing_frame);
 }
 
diff --git a/tests/fate/cover-art.mak b/tests/fate/cover-art.mak
index b56d254bdd..febdec6f04 100644
--- a/tests/fate/cover-art.mak
+++ b/tests/fate/cover-art.mak
@@ -46,6 +46,12 @@ FATE_COVER_ART_REMUX-$(call ALLYES, MP3_DEMUXER MJPEG_DECODER \
                        += fate-cover-art-mp3-id3v2-remux
 fate-cover-art-mp3-id3v2-remux: CMD = transcode mp3 $(TARGET_SAMPLES)/exif/embedded_small.mp3 mp3 "-map 0 -map 0:v -map 0:v -c:a copy -filter:v:0 scale -filter:v:2 scale -c:v:0 bmp -c:v:1 copy -c:v:2 png -metadata:s:v:0 comment=Band/Orchestra" "-map 0 -c copy -t 0.1" "-show_entries stream_tags:stream_disposition=attached_pic:stream=index,codec_name"
 
+FATE_COVER_ART_REMUX-$(call ALLYES, MP3_DEMUXER MP3_MUXER MD5_PROTOCOL) \
+                       += fate-cover-art-mp3-id3v2-nonseekable
+fate-cover-art-mp3-id3v2-nonseekable: CMD = md5pipe -i $(TARGET_SAMPLES)/exif/embedded_small.mp3 -map 0 -c copy -fflags +bitexact -write_xing 0 -f mp3
+fate-cover-art-mp3-id3v2-nonseekable: CMP = oneline
+fate-cover-art-mp3-id3v2-nonseekable: REF = 7d7fc4c0a5fe89a418bab6c74b6cda08
+
 # Also covers muxing and demuxing of nonstandard channel layouts into FLAC
 # as well as the unorthodox multi_dim_quant option of the FLAC encoder.
 FATE_COVER_ART_REMUX-$(call ALLYES, MOV_DEMUXER OGG_DEMUXER   \
diff --git a/tests/fate/id3v2.mak b/tests/fate/id3v2.mak
index 4bde6225b1..76c2b76be3 100644
--- a/tests/fate/id3v2.mak
+++ b/tests/fate/id3v2.mak
@@ -18,6 +18,11 @@ fate-id3v2-keep-metadata-invalid-stream-spec: REF = Trailing garbage at the end
 FATE_ID3V2_FFMPEG_FFPROBE-$(call REMUX, MP3) += fate-id3v2-priv-remux
 fate-id3v2-priv-remux: CMD = transcode mp3 $(TARGET_SAMPLES)/id3v2/id3v2_priv.mp3 mp3 "-c copy" "-c copy -t 0.1" "-show_entries format_tags"
 
+FATE_ID3V2_FFMPEG-$(call ALLYES, MP3_DEMUXER MP3_MUXER MD5_PROTOCOL) += fate-id3v2-padding-nonseekable
+fate-id3v2-padding-nonseekable: CMD = md5pipe -i $(TARGET_SAMPLES)/exif/embedded_small.mp3 -map 0:a -map_metadata -1 -metadata title=padding-test -c copy -fflags +bitexact -write_xing 0 -metadata_header_padding 40000 -f mp3
+fate-id3v2-padding-nonseekable: CMP = oneline
+fate-id3v2-padding-nonseekable: REF = 6a53649febb6345d3752445dcd3f3159
+
 ID3V2_TESTBIN = libavformat/tests/id3v2$(EXESUF)
 
 FATE_ID3V2_RAW-$(call REMUX, MP3) += fate-id3v2-comm
-- 
2.52.0

_______________________________________________
ffmpeg-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]