[FFmpeg-devel] [PATCH] lavf/mp3enc: write attached pictures in the stream order

[Anton Khirnov <anton@khirnov.net>]

Not in the order in which the packets were passed to the muxer, which
may be arbitrary. This guarantees stable and predictable output.

Changes the result of fate-cover-art-mp3-id3v2-remux, where the pictures
are now ordered correctly in the file.
---
 libavformat/mp3enc.c                     | 64 ++++++++++++++++++------
 tests/ref/fate/cover-art-mp3-id3v2-remux | 22 ++++----
 2 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c
index 5e81f72a59a..da33272e4e9 100644
--- a/libavformat/mp3enc.c
+++ b/libavformat/mp3enc.c
@@ -96,6 +96,10 @@ static int id3v1_create_tag(AVFormatContext *s, uint8_t *buf)
 // size of the XING/LAME data, starting from the Xing tag
 #define XING_SIZE 156
 
+typedef struct MP3StreamData {
+    AVPacket *apic;
+} MP3StreamData;
+
 typedef struct MP3Context {
     const AVClass *class;
     ID3v2EncContext id3;
@@ -129,8 +133,8 @@ typedef struct MP3Context {
 
     /* index of the audio stream */
     int audio_stream_idx;
-    /* number of attached pictures we still need to write */
-    int pics_to_write;
+    /* number of attached pictures for which we did not yet get a packet */
+    int pics_to_buffer;
 
     /* audio packets are queued here until we get all the attached pictures */
     PacketList queue;
@@ -385,6 +389,20 @@ static int mp3_queue_flush(AVFormatContext *s)
     AVPacket *const pkt = ffformatcontext(s)->pkt;
     int ret = 0, write = 1;
 
+    // write all buffered attached pictures
+    for (int i = 0; i < s->nb_streams; i++) {
+        MP3StreamData *stp = s->streams[i]->priv_data;
+
+        if (!stp || !stp->apic)
+            continue;
+
+        ret = ff_id3v2_write_apic(s, &mp3->id3, stp->apic);
+        if (ret < 0)
+            return ret;
+
+        av_packet_free(&stp->apic);
+    }
+
     ff_id3v2_finish(&mp3->id3, s->pb, s->metadata_header_padding);
     mp3_write_xing(s);
 
@@ -473,7 +491,7 @@ static int mp3_write_trailer(struct AVFormatContext *s)
     uint8_t buf[ID3v1_TAG_SIZE];
     MP3Context *mp3 = s->priv_data;
 
-    if (mp3->pics_to_write) {
+    if (mp3->pics_to_buffer) {
         av_log(s, AV_LOG_WARNING, "No packets were sent for some of the "
                "attached pictures.\n");
         mp3_queue_flush(s);
@@ -523,35 +541,40 @@ static int mp3_write_packet(AVFormatContext *s, AVPacket *pkt)
     MP3Context *mp3 = s->priv_data;
 
     if (pkt->stream_index == mp3->audio_stream_idx) {
-        if (mp3->pics_to_write) {
+        if (mp3->pics_to_buffer) {
             /* buffer audio packets until we get all the pictures */
             int ret = avpriv_packet_list_put(&mp3->queue, pkt, NULL, 0);
 
             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->pics_to_buffer = 0;
                 mp3_queue_flush(s);
                 return mp3_write_audio_packet(s, pkt);
             }
         } else
             return mp3_write_audio_packet(s, pkt);
     } else {
+        AVStream       *st = s->streams[pkt->stream_index];
+        MP3StreamData *stp = st->priv_data;
         int ret;
 
         /* warn only once for each stream */
-        if (s->streams[pkt->stream_index]->nb_frames == 1) {
+        if (st->nb_frames == 1) {
             av_log(s, AV_LOG_WARNING, "Got more than one picture in stream %d,"
                    " ignoring.\n", pkt->stream_index);
         }
-        if (!mp3->pics_to_write || s->streams[pkt->stream_index]->nb_frames >= 1)
+        if (!mp3->pics_to_buffer || st->nb_frames >= 1)
             return 0;
 
-        if ((ret = ff_id3v2_write_apic(s, &mp3->id3, pkt)) < 0)
-            return ret;
-        mp3->pics_to_write--;
+        av_assert0(!stp->apic);
+        stp->apic = av_packet_clone(pkt);
+        if (!stp->apic)
+            return AVERROR(ENOMEM);
+
+        mp3->pics_to_buffer--;
 
         /* flush the buffered audio packets */
-        if (!mp3->pics_to_write &&
+        if (!mp3->pics_to_buffer &&
             (ret = mp3_queue_flush(s)) < 0)
             return ret;
     }
@@ -588,7 +611,11 @@ static int mp3_init(struct AVFormatContext *s)
                 return AVERROR(EINVAL);
             }
             mp3->audio_stream_idx = i;
-        } else if (st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO) {
+        } else if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) {
+            st->priv_data = av_mallocz(sizeof(MP3StreamData));
+            if (!st->priv_data)
+                return AVERROR(ENOMEM);
+        } else {
             av_log(s, AV_LOG_ERROR, "Only audio streams and pictures are allowed in MP3.\n");
             return AVERROR(EINVAL);
         }
@@ -597,9 +624,9 @@ static int mp3_init(struct AVFormatContext *s)
         av_log(s, AV_LOG_ERROR, "No audio stream present.\n");
         return AVERROR(EINVAL);
     }
-    mp3->pics_to_write = s->nb_streams - 1;
+    mp3->pics_to_buffer = s->nb_streams - 1;
 
-    if (mp3->pics_to_write && !mp3->id3v2_version) {
+    if (mp3->pics_to_buffer && !mp3->id3v2_version) {
         av_log(s, AV_LOG_ERROR, "Attached pictures were requested, but the "
                "ID3v2 header is disabled.\n");
         return AVERROR(EINVAL);
@@ -620,7 +647,7 @@ static int mp3_write_header(struct AVFormatContext *s)
             return ret;
     }
 
-    if (!mp3->pics_to_write) {
+    if (!mp3->pics_to_buffer) {
         if (mp3->id3v2_version)
             ff_id3v2_finish(&mp3->id3, s->pb, s->metadata_header_padding);
         mp3_write_xing(s);
@@ -633,6 +660,13 @@ static void mp3_deinit(struct AVFormatContext *s)
 {
     MP3Context *mp3 = s->priv_data;
 
+    for (int i = 0; i < s->nb_streams; i++) {
+        MP3StreamData *stp = s->streams[i]->priv_data;
+
+        if (stp)
+            av_packet_free(&stp->apic);
+    }
+
     avpriv_packet_list_free(&mp3->queue);
     av_freep(&mp3->xing_frame);
 }
diff --git a/tests/ref/fate/cover-art-mp3-id3v2-remux b/tests/ref/fate/cover-art-mp3-id3v2-remux
index 906a6467994..52b7e72a569 100644
--- a/tests/ref/fate/cover-art-mp3-id3v2-remux
+++ b/tests/ref/fate/cover-art-mp3-id3v2-remux
@@ -1,4 +1,4 @@
-c1b55a9a92226cd72d3f53ccd830d127 *tests/data/fate/cover-art-mp3-id3v2-remux.mp3
+94946f0efd5f9bb0061ac1fbff7d731f *tests/data/fate/cover-art-mp3-id3v2-remux.mp3
 399346 tests/data/fate/cover-art-mp3-id3v2-remux.mp3
 #tb 0: 1/14112000
 #media_type 0: audio
@@ -7,22 +7,22 @@ c1b55a9a92226cd72d3f53ccd830d127 *tests/data/fate/cover-art-mp3-id3v2-remux.mp3
 #channel_layout_name 0: stereo
 #tb 1: 1/90000
 #media_type 1: video
-#codec_id 1: mjpeg
+#codec_id 1: bmp
 #dimensions 1: 263x263
-#sar 1: 96/96
+#sar 1: 0/1
 #tb 2: 1/90000
 #media_type 2: video
-#codec_id 2: bmp
+#codec_id 2: mjpeg
 #dimensions 2: 263x263
-#sar 2: 0/1
+#sar 2: 96/96
 #tb 3: 1/90000
 #media_type 3: video
 #codec_id 3: png
 #dimensions 3: 263x263
 #sar 3: 1/1
 0,    -353590,    -353590,   368640,      417, 0x15848290, S=1,       10
-1,          0,          0,        0,    15760, 0x71d5c418
-2,          0,          0,        0,   208350, 0x291b44d1
+1,          0,          0,        0,   208350, 0x291b44d1
+2,          0,          0,        0,    15760, 0x71d5c418
 3,          0,          0,        0,   165671, 0x7c1c8070
 0,      15050,      15050,   368640,      418, 0x46f684a4
 0,     383690,     383690,   368640,      418, 0x46f684a4
@@ -36,15 +36,15 @@ TAG:encoder=Lavf
 [/STREAM]
 [STREAM]
 index=1
-codec_name=mjpeg
+codec_name=bmp
 DISPOSITION:attached_pic=1
-TAG:comment=Other
+TAG:comment=Band/Orchestra
 [/STREAM]
 [STREAM]
 index=2
-codec_name=bmp
+codec_name=mjpeg
 DISPOSITION:attached_pic=1
-TAG:comment=Band/Orchestra
+TAG:comment=Other
 [/STREAM]
 [STREAM]
 index=3
-- 
2.39.2

_______________________________________________
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".