[FFmpeg-devel] [PATCH 1/4] avcodec/pngdec: Fix APNG_DISPOSE_OP_BACKGROUND
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Mon Aug 22 23:45:58 EEST 2022
Andreas Rheinhardt:
> APNG works with a single reference frame and an output frame.
> According to the spec, decoding APNG works by decoding
> the current IDAT/fdAT chunks (which decodes to a rectangular
> subregion of the whole image region), followed by either
> overwriting the region of the output frame with the newly
> decoded data or by blending the newly decoded data with
> the data from the reference frame onto the current subregion
> of the output frame. The remainder of the output frame
> is just copied from the reference frame.
> Then the reference frame might be left untouched
> (APNG_DISPOSE_OP_PREVIOUS), it might be replaced by the output
> frame (APNG_DISPOSE_OP_NONE) or the rectangular subregion
> corresponding to the just decoded frame has to be reset
> to black (APNG_DISPOSE_OP_BACKGROUND).
>
> The latter case is not handled correctly by our decoder:
> It only performs resetting the rectangle in the reference frame
> when decoding the next frame; and since commit
> b593abda6c642cb0c3959752dd235c2faf66837f it does not reset
> the reference frame permanently, but only temporarily (i.e.
> it only affects decoding the frame after the frame with
> APNG_DISPOSE_OP_BACKGROUND). This is a problem if the
> frame after the APNG_DISPOSE_OP_BACKGROUND frame uses
> APNG_DISPOSE_OP_PREVIOUS, because then the frame after
> the APNG_DISPOSE_OP_PREVIOUS frame has an incorrect reference
> frame. (If it is not followed by an APNG_DISPOSE_OP_PREVIOUS
> frame, the decoder only keeps a reference to the output frame,
> which is ok.)
>
> This commit fixes this by being much closer to the spec
> than the earlier code: Resetting the background is no longer
> postponed until the next frame; instead it is applied to
> the reference frame.
>
> Fixes ticket #9602.
>
> (For multithreaded decoding it was actually already broken
> since commit 5663301560d77486c7f7c03c1aa5f542fab23c24.)
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
> Btw: If we had a function that only references a frame's data
> (and leaves the dst frame's side data completely untouched),
> we could apply the side data directly to the output frame,
> making 8d74baccff59192d395735036cd40a131a140391 unnecessary.
>
> I also wonder whether the handle_p_frame stuff should be moved
> out of decode_frame_common() and in the codec-specific code.
>
> libavcodec/pngdec.c | 98 ++++++++++++++++++++++-----------------------
> 1 file changed, 48 insertions(+), 50 deletions(-)
>
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 3b888199d4..8a197d038d 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -78,11 +78,8 @@ typedef struct PNGDecContext {
> enum PNGImageState pic_state;
> int width, height;
> int cur_w, cur_h;
> - int last_w, last_h;
> int x_offset, y_offset;
> - int last_x_offset, last_y_offset;
> uint8_t dispose_op, blend_op;
> - uint8_t last_dispose_op;
> int bit_depth;
> int color_type;
> int compression_type;
> @@ -94,8 +91,6 @@ typedef struct PNGDecContext {
> int has_trns;
> uint8_t transparent_color_be[6];
>
> - uint8_t *background_buf;
> - unsigned background_buf_allocated;
> uint32_t palette[256];
> uint8_t *crow_buf;
> uint8_t *last_row;
> @@ -725,9 +720,30 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
> }
>
> ff_thread_release_ext_buffer(avctx, &s->picture);
> - if ((ret = ff_thread_get_ext_buffer(avctx, &s->picture,
> - AV_GET_BUFFER_FLAG_REF)) < 0)
> - return ret;
> + if (s->dispose_op == APNG_DISPOSE_OP_PREVIOUS) {
> + /* We only need a buffer for the current picture. */
> + ret = ff_thread_get_buffer(avctx, p, 0);
> + if (ret < 0)
> + return ret;
> + } else if (s->dispose_op == APNG_DISPOSE_OP_BACKGROUND) {
> + /* We need a buffer for the current picture as well as
> + * a buffer for the reference to retain. */
> + ret = ff_thread_get_ext_buffer(avctx, &s->picture,
> + AV_GET_BUFFER_FLAG_REF);
> + if (ret < 0)
> + return ret;
> + ret = ff_thread_get_buffer(avctx, p, 0);
> + if (ret < 0)
> + return ret;
> + } else {
> + /* The picture output this time and the reference to retain coincide. */
> + if ((ret = ff_thread_get_ext_buffer(avctx, &s->picture,
> + AV_GET_BUFFER_FLAG_REF)) < 0)
> + return ret;
> + ret = av_frame_ref(p, s->picture.f);
> + if (ret < 0)
> + return ret;
> + }
>
> p->pict_type = AV_PICTURE_TYPE_I;
> p->key_frame = 1;
> @@ -985,12 +1001,6 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s,
> return AVERROR_INVALIDDATA;
> }
>
> - s->last_w = s->cur_w;
> - s->last_h = s->cur_h;
> - s->last_x_offset = s->x_offset;
> - s->last_y_offset = s->y_offset;
> - s->last_dispose_op = s->dispose_op;
> -
> sequence_number = bytestream2_get_be32(gb);
> cur_w = bytestream2_get_be32(gb);
> cur_h = bytestream2_get_be32(gb);
> @@ -1086,23 +1096,6 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
>
> ff_thread_await_progress(&s->last_picture, INT_MAX, 0);
>
> - // need to reset a rectangle to background:
> - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) {
> - av_fast_malloc(&s->background_buf, &s->background_buf_allocated,
> - src_stride * p->height);
> - if (!s->background_buf)
> - return AVERROR(ENOMEM);
> -
> - memcpy(s->background_buf, src, src_stride * p->height);
> -
> - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) {
> - memset(s->background_buf + src_stride * y +
> - bpp * s->last_x_offset, 0, bpp * s->last_w);
> - }
> -
> - src = s->background_buf;
> - }
> -
> // copy unchanged rectangles from the last frame
> for (y = 0; y < s->y_offset; y++)
> memcpy(dst + y * dst_stride, src + y * src_stride, p->width * bpp);
> @@ -1171,6 +1164,22 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
> return 0;
> }
>
> +static void apng_reset_background(PNGDecContext *s, const AVFrame *p)
> +{
> + // need to reset a rectangle to black
> + av_unused int ret = av_frame_copy(s->picture.f, p);
> + const int bpp = s->color_type == PNG_COLOR_TYPE_PALETTE ? 4 : s->bpp;
> + const ptrdiff_t dst_stride = s->picture.f->linesize[0];
> + uint8_t *dst = s->picture.f->data[0] + s->y_offset * dst_stride + bpp * s->x_offset;
> +
> + av_assert1(ret >= 0);
> +
> + for (size_t y = 0; y < s->cur_h; y++) {
> + memset(dst, 0, bpp * s->cur_w);
> + dst += dst_stride;
> + }
> +}
> +
> static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
> AVFrame *p, const AVPacket *avpkt)
> {
> @@ -1434,6 +1443,9 @@ exit_loop:
> goto fail;
> }
> }
> + if (CONFIG_APNG_DECODER && s->dispose_op == APNG_DISPOSE_OP_BACKGROUND)
> + apng_reset_background(s, p);
> +
> ff_thread_report_progress(&s->picture, INT_MAX, 0);
>
> return 0;
> @@ -1456,15 +1468,10 @@ static void clear_frame_metadata(PNGDecContext *s)
> av_dict_free(&s->frame_metadata);
> }
>
> -static int output_frame(PNGDecContext *s, AVFrame *f,
> - const AVFrame *src)
> +static int output_frame(PNGDecContext *s, AVFrame *f)
> {
> int ret;
>
> - ret = av_frame_ref(f, src);
> - if (ret < 0)
> - return ret;
> -
> if (s->iccp_data) {
> AVFrameSideData *sd = av_frame_new_side_data(f, AV_FRAME_DATA_ICC_PROFILE, s->iccp_data_len);
> if (!sd) {
> @@ -1515,13 +1522,12 @@ fail:
> }
>
> #if CONFIG_PNG_DECODER
> -static int decode_frame_png(AVCodecContext *avctx, AVFrame *dst_frame,
> +static int decode_frame_png(AVCodecContext *avctx, AVFrame *p,
> int *got_frame, AVPacket *avpkt)
> {
> PNGDecContext *const s = avctx->priv_data;
> const uint8_t *buf = avpkt->data;
> int buf_size = avpkt->size;
> - AVFrame *p = s->picture.f;
> int64_t sig;
> int ret;
>
> @@ -1555,7 +1561,7 @@ static int decode_frame_png(AVCodecContext *avctx, AVFrame *dst_frame,
> goto the_end;
> }
>
> - ret = output_frame(s, dst_frame, s->picture.f);
> + ret = output_frame(s, p);
> if (ret < 0)
> goto the_end;
>
> @@ -1574,12 +1580,11 @@ the_end:
> #endif
>
> #if CONFIG_APNG_DECODER
> -static int decode_frame_apng(AVCodecContext *avctx, AVFrame *dst_frame,
> +static int decode_frame_apng(AVCodecContext *avctx, AVFrame *p,
> int *got_frame, AVPacket *avpkt)
> {
> PNGDecContext *const s = avctx->priv_data;
> int ret;
> - AVFrame *p = s->picture.f;
>
> clear_frame_metadata(s);
>
> @@ -1608,7 +1613,7 @@ static int decode_frame_apng(AVCodecContext *avctx, AVFrame *dst_frame,
> if (!(s->pic_state & (PNG_ALLIMAGE|PNG_IDAT)))
> return AVERROR_INVALIDDATA;
>
> - ret = output_frame(s, dst_frame, s->picture.f);
> + ret = output_frame(s, p);
> if (ret < 0)
> return ret;
>
> @@ -1646,15 +1651,9 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
> pdst->compression_type = psrc->compression_type;
> pdst->interlace_type = psrc->interlace_type;
> pdst->filter_type = psrc->filter_type;
> - pdst->cur_w = psrc->cur_w;
> - pdst->cur_h = psrc->cur_h;
> - pdst->x_offset = psrc->x_offset;
> - pdst->y_offset = psrc->y_offset;
> pdst->has_trns = psrc->has_trns;
> memcpy(pdst->transparent_color_be, psrc->transparent_color_be, sizeof(pdst->transparent_color_be));
>
> - pdst->dispose_op = psrc->dispose_op;
> -
> memcpy(pdst->palette, psrc->palette, sizeof(pdst->palette));
>
> pdst->hdr_state |= psrc->hdr_state;
> @@ -1705,7 +1704,6 @@ static av_cold int png_dec_end(AVCodecContext *avctx)
> s->last_row_size = 0;
> av_freep(&s->tmp_row);
> s->tmp_row_size = 0;
> - av_freep(&s->background_buf);
>
> av_freep(&s->iccp_data);
> av_dict_free(&s->frame_metadata);
Will apply this patchset tomorrow unless there are objections.
- Andreas
More information about the ffmpeg-devel
mailing list