From patchwork Wed Apr 26 09:46:04 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anton Khirnov X-Patchwork-Id: 41327 Delivered-To: ffmpegpatchwork2@gmail.com Received: by 2002:a05:6a20:dca6:b0:f3:34fa:f187 with SMTP id ky38csp342370pzb; Wed, 26 Apr 2023 02:46:28 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6lq1kVnGRj9WGg/Ko93BbeYu/sWzITfJzbE+Z7T/6p6r7FGl9gUor+CAY/eTjZ+PdQsKQb X-Received: by 2002:a17:906:198:b0:94e:cfd0:ed9f with SMTP id 24-20020a170906019800b0094ecfd0ed9fmr1659524ejb.26.1682502388212; Wed, 26 Apr 2023 02:46:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682502388; cv=none; d=google.com; s=arc-20160816; b=xEksJG3bVIYgd5vlqkrwKcayUBoeqk2Zv0ehqSwnQ2Kcsb3y/cFRHGn19yPJskn82B vBIYFRFlZ7lR2A3kwJJKwDd+JPXQiYgwF55sLmQI68mwFKqDN0fZQ/4MY5+bUs/uwmNl bL+yDN/NzWocQGGcS9eeyf9JpAVMXZ63v3siQcIV4gP+JUoEZ79vw3E3U5ohbKMC6R9d UmPt3FcvxHL0ALYl/zG39bnzIwXjcKx3yiv0RMwbShSCaIRJNw7bOeqpd2Dw4j+96BR5 5SUNYH5eFjbdAPJDtgsRFOF8o2sWcmS4kuKpnObyGvXQRjAVe+9RhJUEk5bdXe6LuLww r/Cw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:content-transfer-encoding:reply-to:list-subscribe :list-help:list-post:list-archive:list-unsubscribe:list-id :precedence:subject:mime-version:message-id:date:to:from :delivered-to; bh=iLgEjBBfEdRDlAzMsr5NDKVehHvT8+3ieVZyzwKMviY=; b=vlevvnDWyKU7Htzm3f2xvaEKYYsVUFanLfyosgetp35Eyy1fJZJBA6YgCvbyOetSGW ntBYFhzX1VAPW0htu4MdJ2u972c+UQJkRroAbp36q2KfBN4rU9ssRq91JsHN11xWitQG lBs8+cJZ6qXqhEpGnhqMGOX/Vx0qhHx4j2pYkWkV7IGBjAPsCi6y8nYcdXji12YuPaxk Z4QAi3L6gSXKlZZtBpWzRmEFiJ527bIqg3u+AkSMm1eZsEXbAHH+GELlPYL1IrOv1qYM 0Woi3NxMrVvKUIXAdN75dauIEuRB18CsyxgzdD+6i5UpEEGMELBn0wki3XNrnYMqbPte n89A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id lz9-20020a170906fb0900b0094f67c904ccsi11777843ejb.91.2023.04.26.02.46.27; Wed, 26 Apr 2023 02:46:28 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 069E968B36D; Wed, 26 Apr 2023 12:46:25 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9FCE668B274 for ; Wed, 26 Apr 2023 12:46:16 +0300 (EEST) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id F249C2404EE for ; Wed, 26 Apr 2023 11:46:15 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id OuIwJxgZwAoU for ; Wed, 26 Apr 2023 11:46:15 +0200 (CEST) Received: from libav.khirnov.net (libav.khirnov.net [IPv6:2a00:c500:561:201::7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "libav.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id 1B7AD2404EC for ; Wed, 26 Apr 2023 11:46:15 +0200 (CEST) Received: from libav.khirnov.net (libav.khirnov.net [IPv6:::1]) by libav.khirnov.net (Postfix) with ESMTP id C19443A0212 for ; Wed, 26 Apr 2023 11:46:08 +0200 (CEST) From: Anton Khirnov To: ffmpeg-devel@ffmpeg.org Date: Wed, 26 Apr 2023 11:46:04 +0200 Message-Id: <20230426094604.4715-1-anton@khirnov.net> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH] lavf/mp3enc: write attached pictures in the stream order 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" X-TUID: axx+2Smw80ML 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