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 <andreas.cadhal...@googlemail.com>
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 <andreas.cadhal...@googlemail.com>
---
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
