From patchwork Thu Oct 27 22:43:05 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Cadhalpun X-Patchwork-Id: 1208 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.140.133 with SMTP id o127csp809105vsd; Thu, 27 Oct 2016 15:43:18 -0700 (PDT) X-Received: by 10.28.209.75 with SMTP id i72mr814219wmg.56.1477608198662; Thu, 27 Oct 2016 15:43:18 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id j20si6098183wmg.138.2016.10.27.15.43.16; Thu, 27 Oct 2016 15:43:18 -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; dkim=neutral (body hash did not verify) header.i=@googlemail.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; dmarc=fail (p=QUARANTINE dis=NONE) header.from=googlemail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id F3476689F44; Fri, 28 Oct 2016 01:43:10 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4DEFF689EBA for ; Fri, 28 Oct 2016 01:43:04 +0300 (EEST) Received: by mail-wm0-f67.google.com with SMTP id c17so4783698wmc.3 for ; Thu, 27 Oct 2016 15:43:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20120113; h=from:subject:to:references:message-id:date:user-agent:mime-version :in-reply-to; bh=DD9ABp0LS9fOkYIpRTMF7TNvHfvsZ4qclMITzsZ1TnE=; b=0SaY9D0hwASl3UqqHvseCHSEw1GKpTq3B8Ig+UW5MpFDe4lGv8p2bVQxGbgDslx/7O vr/GZgN11a53mRlwC9d2EUisHk0jG0iluiNU/YHADevFOtocGUPAyYD6joPuFFxxghRU JGhMC4UyZ+t1mzW8CcRJX1ZBnKdb6vI9BaKu9v7689y1vitQOEwjoJQI/DTdOA0jWsKp GaUuNtl4uerNF5dR0hHpZ8CK9VIl/F0IHOg2yG1GrKpbIXbGBI2V3ehoE61/8D11c/TY xonJXDMwWNFyIwhn5uXXGiiKFvYvtB+yGiy/FzImmxrwhQ0Kd62xgUZP9uhNgMQIhZpo Jiyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to; bh=DD9ABp0LS9fOkYIpRTMF7TNvHfvsZ4qclMITzsZ1TnE=; b=UQ6uYsqntQu8N8tgQu5dxoOaSty3kStY/JVllDCWD4qGX0/jtua4D/3Mz3UEW7aPcg BDP01TrLYsB+LrFkloW0nCWuaZ9LCFZLM6nmokCVC5I8tNzGMfKexsnaQ9h3+HNkXKrS LPvpvgwXyW7e4mkJyXlOow+/tEMOUpRMdvEG42mfJR/9HIgjCHj2KDLA49sIqxMjrh16 KWN2eOmGdF08NZDqXyvYpHMD3GcCTpmkPavZvQSmIjqaPQZ4ajF+0g90hP/svoaI9z1m lmpLJVGrjAGj1nU2KPtjq3bI3x2DNaFhjV6Oom6Ph36TmzI4GE4sehlZnkS6A/zKqONj Nr7Q== X-Gm-Message-State: ABUngvfIMwj5bYpT5SseVfdJDXebEBi4Adqf89WfBFsbXnfzirK8GOXBRe8T2dKX3Sy5NQ== X-Received: by 10.28.55.203 with SMTP id e194mr517679wma.97.1477608186782; Thu, 27 Oct 2016 15:43:06 -0700 (PDT) Received: from [192.168.2.21] (p5B095458.dip0.t-ipconnect.de. [91.9.84.88]) by smtp.googlemail.com with ESMTPSA id rv12sm10869013wjb.29.2016.10.27.15.43.05 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 27 Oct 2016 15:43:06 -0700 (PDT) From: Andreas Cadhalpun X-Google-Original-From: Andreas Cadhalpun To: ffmpeg-devel@ffmpeg.org References: <084583c6-4351-9553-97f6-cb6cd08ecbf0@gmail.com> <974ac6e6-f925-961f-a0d3-24bf3c158c8a@googlemail.com> <268bab71-0ff9-0804-48ff-29c73963415b@gmail.com> Message-ID: <1d9ffe35-7826-de96-b4c6-8b4990b5393d@googlemail.com> Date: Fri, 28 Oct 2016 00:43:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: <268bab71-0ff9-0804-48ff-29c73963415b@gmail.com> Subject: Re: [FFmpeg-devel] [PATCH] ffmpeg: copy the extradata from encoder to muxer X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 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" On 27.10.2016 23:39, James Almer wrote: > On 10/27/2016 5:38 PM, Andreas Cadhalpun wrote: >> libavcodec/pngenc.c | 4 ++++ >> libavformat/apngenc.c | 27 ++++++++++++++++++++++++--- >> 2 files changed, 28 insertions(+), 3 deletions(-) >> >> diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c >> index 00c830e..1a308f2 100644 >> --- a/libavcodec/pngenc.c >> +++ b/libavcodec/pngenc.c >> @@ -917,6 +917,10 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt, >> if (s->last_frame) { >> uint8_t* last_fctl_chunk_start = pkt->data; >> uint8_t buf[26]; >> + uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA, avctx->extradata_size); > > You could add a variable called extradata_updated or so to PNGEncContext and set it to > 1 here so the encoder doesn't add side data to every packet when it's only needed for > the first. OK. >> + if (!side_data) >> + return AVERROR(ENOMEM); >> + memcpy(side_data, avctx->extradata, avctx->extradata_size); >> >> AV_WB32(buf + 0, s->last_frame_fctl.sequence_number); >> AV_WB32(buf + 4, s->last_frame_fctl.width); >> diff --git a/libavformat/apngenc.c b/libavformat/apngenc.c >> index e25df2e..f702667 100644 >> --- a/libavformat/apngenc.c >> +++ b/libavformat/apngenc.c >> @@ -101,15 +101,29 @@ static int apng_write_header(AVFormatContext *format_context) >> return 0; >> } >> >> -static void flush_packet(AVFormatContext *format_context, AVPacket *packet) >> +static int flush_packet(AVFormatContext *format_context, AVPacket *packet) >> { >> APNGMuxContext *apng = format_context->priv_data; >> AVIOContext *io_context = format_context->pb; >> AVStream *codec_stream = format_context->streams[0]; >> AVCodecParameters *codec_par = codec_stream->codecpar; >> + uint8_t *side_data = NULL; >> + int side_data_size = 0; >> >> av_assert0(apng->prev_packet); >> >> + if (packet) >> + side_data = av_packet_get_side_data(packet, AV_PKT_DATA_NEW_EXTRADATA, &side_data_size); > > If the muxer gets only one frame, the code below (standard png mode fallback) will not > work. You can test this with "./ffmpeg -f lavfi -i testsrc=s=32x32 -vframes 1 apng.apng". > > Don't check for packet and use apng->prev_packet unconditionally instead. That should > do it. Indeed, fixed. > Ideally, you'd not use avctx->extradata* in the apng encoder or codecpar->extradata* > in the muxer. The former should only be written by the init() function, and the latter > should afaik not be modified by the muxer at all. > If you can store the pointers and sizes for the extradata in the encoder/muxer contexts > and use them instead of avctx and codecpar extradata then that'd be great. Fine for me, updated patch attached. To avoid confusion and for lack of a better name I called the variables in the encoder/muxer contexts extra_side_data*. Best regards, Andreas From 20ad7d6905ce2123fd8100b6fe6e092dbbdf3c06 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun Date: Thu, 27 Oct 2016 22:34:48 +0200 Subject: [PATCH] apng: use side data to pass extradata to muxer This fixes creating apng files, which is broken since commit 5ef19590802f000299e418143fc2301e3f43affe. Signed-off-by: Andreas Cadhalpun --- libavcodec/pngenc.c | 18 +++++++++++++++--- libavformat/apngenc.c | 45 +++++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c index 00c830e..99395dd 100644 --- a/libavcodec/pngenc.c +++ b/libavcodec/pngenc.c @@ -68,6 +68,9 @@ typedef struct PNGEncContext { // APNG uint32_t palette_checksum; // Used to ensure a single unique palette uint32_t sequence_number; + int extra_side_data_updated; + uint8_t *extra_side_data; + int extra_side_data_size; AVFrame *prev_frame; AVFrame *last_frame; @@ -870,15 +873,15 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt, if (!pict) return AVERROR(EINVAL); - s->bytestream = avctx->extradata = av_malloc(FF_MIN_BUFFER_SIZE); - if (!avctx->extradata) + s->bytestream = s->extra_side_data = av_malloc(FF_MIN_BUFFER_SIZE); + if (!s->extra_side_data) return AVERROR(ENOMEM); ret = encode_headers(avctx, pict); if (ret < 0) return ret; - avctx->extradata_size = s->bytestream - avctx->extradata; + s->extra_side_data_size = s->bytestream - s->extra_side_data; s->last_frame_packet = av_malloc(max_packet_size); if (!s->last_frame_packet) @@ -917,6 +920,13 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt, if (s->last_frame) { uint8_t* last_fctl_chunk_start = pkt->data; uint8_t buf[26]; + if (!s->extra_side_data_updated) { + uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA, s->extra_side_data_size); + if (!side_data) + return AVERROR(ENOMEM); + memcpy(side_data, s->extra_side_data, s->extra_side_data_size); + s->extra_side_data_updated = 1; + } AV_WB32(buf + 0, s->last_frame_fctl.sequence_number); AV_WB32(buf + 4, s->last_frame_fctl.width); @@ -1093,6 +1103,8 @@ static av_cold int png_enc_close(AVCodecContext *avctx) av_frame_free(&s->last_frame); av_frame_free(&s->prev_frame); av_freep(&s->last_frame_packet); + av_freep(&s->extra_side_data); + s->extra_side_data_size = 0; return 0; } diff --git a/libavformat/apngenc.c b/libavformat/apngenc.c index e25df2e..9acb9d2 100644 --- a/libavformat/apngenc.c +++ b/libavformat/apngenc.c @@ -44,6 +44,9 @@ typedef struct APNGMuxContext { AVRational prev_delay; int framerate_warned; + + uint8_t *extra_side_data; + int extra_side_data_size; } APNGMuxContext; static uint8_t *apng_find_chunk(uint32_t tag, uint8_t *buf, size_t length) @@ -101,15 +104,27 @@ static int apng_write_header(AVFormatContext *format_context) return 0; } -static void flush_packet(AVFormatContext *format_context, AVPacket *packet) +static int flush_packet(AVFormatContext *format_context, AVPacket *packet) { APNGMuxContext *apng = format_context->priv_data; AVIOContext *io_context = format_context->pb; AVStream *codec_stream = format_context->streams[0]; - AVCodecParameters *codec_par = codec_stream->codecpar; + uint8_t *side_data = NULL; + int side_data_size = 0; av_assert0(apng->prev_packet); + side_data = av_packet_get_side_data(apng->prev_packet, AV_PKT_DATA_NEW_EXTRADATA, &side_data_size); + + if (side_data_size) { + av_freep(&apng->extra_side_data); + apng->extra_side_data = av_mallocz(side_data_size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!apng->extra_side_data) + return AVERROR(ENOMEM); + apng->extra_side_data_size = side_data_size; + memcpy(apng->extra_side_data, side_data, apng->extra_side_data_size); + } + if (apng->frame_number == 0 && !packet) { uint8_t *existing_acTL_chunk; uint8_t *existing_fcTL_chunk; @@ -117,13 +132,13 @@ static void flush_packet(AVFormatContext *format_context, AVPacket *packet) av_log(format_context, AV_LOG_INFO, "Only a single frame so saving as a normal PNG.\n"); // Write normal PNG headers without acTL chunk - existing_acTL_chunk = apng_find_chunk(MKBETAG('a', 'c', 'T', 'L'), codec_par->extradata, codec_par->extradata_size); + existing_acTL_chunk = apng_find_chunk(MKBETAG('a', 'c', 'T', 'L'), apng->extra_side_data, apng->extra_side_data_size); if (existing_acTL_chunk) { uint8_t *chunk_after_acTL = existing_acTL_chunk + AV_RB32(existing_acTL_chunk) + 12; - avio_write(io_context, codec_par->extradata, existing_acTL_chunk - codec_par->extradata); - avio_write(io_context, chunk_after_acTL, codec_par->extradata + codec_par->extradata_size - chunk_after_acTL); + avio_write(io_context, apng->extra_side_data, existing_acTL_chunk - apng->extra_side_data); + avio_write(io_context, chunk_after_acTL, apng->extra_side_data + apng->extra_side_data_size - chunk_after_acTL); } else { - avio_write(io_context, codec_par->extradata, codec_par->extradata_size); + avio_write(io_context, apng->extra_side_data, apng->extra_side_data_size); } // Write frame data without fcTL chunk @@ -142,9 +157,9 @@ static void flush_packet(AVFormatContext *format_context, AVPacket *packet) uint8_t *existing_acTL_chunk; // Write normal PNG headers - avio_write(io_context, codec_par->extradata, codec_par->extradata_size); + avio_write(io_context, apng->extra_side_data, apng->extra_side_data_size); - existing_acTL_chunk = apng_find_chunk(MKBETAG('a', 'c', 'T', 'L'), codec_par->extradata, codec_par->extradata_size); + existing_acTL_chunk = apng_find_chunk(MKBETAG('a', 'c', 'T', 'L'), apng->extra_side_data, apng->extra_side_data_size); if (!existing_acTL_chunk) { uint8_t buf[8]; // Write animation control header @@ -195,11 +210,13 @@ static void flush_packet(AVFormatContext *format_context, AVPacket *packet) av_packet_unref(apng->prev_packet); if (packet) av_copy_packet(apng->prev_packet, packet); + return 0; } static int apng_write_packet(AVFormatContext *format_context, AVPacket *packet) { APNGMuxContext *apng = format_context->priv_data; + int ret; if (!apng->prev_packet) { apng->prev_packet = av_malloc(sizeof(*apng->prev_packet)); @@ -208,7 +225,9 @@ static int apng_write_packet(AVFormatContext *format_context, AVPacket *packet) av_copy_packet(apng->prev_packet, packet); } else { - flush_packet(format_context, packet); + ret = flush_packet(format_context, packet); + if (ret < 0) + return ret; } return 0; @@ -219,10 +238,13 @@ static int apng_write_trailer(AVFormatContext *format_context) APNGMuxContext *apng = format_context->priv_data; AVIOContext *io_context = format_context->pb; uint8_t buf[8]; + int ret; if (apng->prev_packet) { - flush_packet(format_context, NULL); + ret = flush_packet(format_context, NULL); av_freep(&apng->prev_packet); + if (ret < 0) + return ret; } apng_write_chunk(io_context, MKBETAG('I', 'E', 'N', 'D'), NULL, 0); @@ -235,6 +257,9 @@ static int apng_write_trailer(AVFormatContext *format_context) apng_write_chunk(io_context, MKBETAG('a', 'c', 'T', 'L'), buf, 8); } + av_freep(&apng->extra_side_data); + apng->extra_side_data = 0; + return 0; } -- 2.9.3