Message ID | 20210216202416.12116-5-anton@khirnov.net |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel,1/6] tests: add a test for LSCR | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | fail | Make fate failed |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | warning | Make fate failed |
On Tue, Feb 16, 2021 at 9:26 PM Anton Khirnov <anton@khirnov.net> wrote: > Current code is very confused and confusing. It uses two different > reference frames - "previous" and "last" - when only one is really > necessary. It also confuses the two, leading to incorrect output with > APNG_DISPOSE_OP_PREVIOUS mode. > > Fixes #9017. > --- > libavcodec/pngdec.c | 93 ++++++++++++++++++++------------------------- > 1 file changed, 42 insertions(+), 51 deletions(-) > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > index d02c45f1a7..cece08ebca 100644 > --- a/libavcodec/pngdec.c > +++ b/libavcodec/pngdec.c > @@ -54,7 +54,6 @@ typedef struct PNGDecContext { > AVCodecContext *avctx; > > GetByteContext gb; > - ThreadFrame previous_picture; > ThreadFrame last_picture; > ThreadFrame picture; > > @@ -711,13 +710,10 @@ static int decode_idat_chunk(AVCodecContext *avctx, > PNGDecContext *s, > s->bpp += byte_depth; > } > > + ff_thread_release_buffer(avctx, &s->picture); > if ((ret = ff_thread_get_buffer(avctx, &s->picture, > AV_GET_BUFFER_FLAG_REF)) < 0) > return ret; > - if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op != > APNG_DISPOSE_OP_PREVIOUS) { > - ff_thread_release_buffer(avctx, &s->previous_picture); > - if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture, > AV_GET_BUFFER_FLAG_REF)) < 0) > - return ret; > - } > + > p->pict_type = AV_PICTURE_TYPE_I; > p->key_frame = 1; > p->interlaced_frame = !!s->interlace_type; > @@ -1020,7 +1016,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, > PNGDecContext *s, > return AVERROR_INVALIDDATA; > } > > - if ((sequence_number == 0 || !s->previous_picture.f->data[0]) && > + if ((sequence_number == 0 || !s->last_picture.f->data[0]) && > dispose_op == APNG_DISPOSE_OP_PREVIOUS) { > // No previous frame to revert to for the first frame > // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext > *avctx, PNGDecContext *s, > if (!buffer) > return AVERROR(ENOMEM); > > + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > - // Do the disposal operation specified by the last frame on the frame > - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { > - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > s->height); > - > - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) > - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; > ++y) > - memset(buffer + s->image_linesize * y + s->bpp * > s->last_x_offset, 0, s->bpp * s->last_w); > + // need to reset a rectangle to background: > + // create a new writable copy > + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { > + int ret = av_frame_make_writable(s->last_picture.f); > + if (ret < 0) > + return ret; > > - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize > * s->height); > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > - } else { > - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); > - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize > * s->height); > + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) > { > + memset(s->last_picture.f->data[0] + s->image_linesize * y + > + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > + } > } > > + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > s->height); > + > // Perform blending > if (s->blend_op == APNG_BLEND_OP_SOURCE) { > for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) { > @@ -1448,22 +1444,17 @@ exit_loop: > if (CONFIG_PNG_DECODER && avctx->codec_id != AV_CODEC_ID_APNG) > handle_p_frame_png(s, p); > else if (CONFIG_APNG_DECODER && > - s->previous_picture.f->width == p->width && > - s->previous_picture.f->height== p->height && > - s->previous_picture.f->format== p->format && > No explanation provided why those line are removed. > avctx->codec_id == AV_CODEC_ID_APNG && > (ret = handle_p_frame_apng(avctx, s, p)) < 0) > goto fail; > } > } > ff_thread_report_progress(&s->picture, INT_MAX, 0); > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > > return 0; > > fail: > ff_thread_report_progress(&s->picture, INT_MAX, 0); > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > return ret; > } > > @@ -1475,14 +1466,10 @@ static int decode_frame_png(AVCodecContext *avctx, > PNGDecContext *const s = avctx->priv_data; > const uint8_t *buf = avpkt->data; > int buf_size = avpkt->size; > - AVFrame *p; > + AVFrame *p = s->picture.f; > int64_t sig; > int ret; > > - ff_thread_release_buffer(avctx, &s->last_picture); > - FFSWAP(ThreadFrame, s->picture, s->last_picture); > - p = s->picture.f; > - > bytestream2_init(&s->gb, buf, buf_size); > > /* check signature */ > @@ -1519,6 +1506,11 @@ static int decode_frame_png(AVCodecContext *avctx, > if ((ret = av_frame_ref(data, s->picture.f)) < 0) > goto the_end; > > + if (!(avctx->active_thread_type & FF_THREAD_FRAME)) { > + ff_thread_release_buffer(avctx, &s->last_picture); > + FFSWAP(ThreadFrame, s->picture, s->last_picture); > + } > + > *got_frame = 1; > > ret = bytestream2_tell(&s->gb); > @@ -1536,11 +1528,7 @@ static int decode_frame_apng(AVCodecContext *avctx, > { > PNGDecContext *const s = avctx->priv_data; > int ret; > - AVFrame *p; > - > - ff_thread_release_buffer(avctx, &s->last_picture); > - FFSWAP(ThreadFrame, s->picture, s->last_picture); > - p = s->picture.f; > + AVFrame *p = s->picture.f; > > if (!(s->hdr_state & PNG_IHDR)) { > if (!avctx->extradata_size) > @@ -1576,6 +1564,15 @@ static int decode_frame_apng(AVCodecContext *avctx, > if ((ret = av_frame_ref(data, s->picture.f)) < 0) > goto end; > > + if (!(avctx->active_thread_type & FF_THREAD_FRAME)) { > + if (s->dispose_op == APNG_DISPOSE_OP_PREVIOUS) { > + ff_thread_release_buffer(avctx, &s->picture); > + } else if (s->dispose_op == APNG_DISPOSE_OP_NONE) { > + ff_thread_release_buffer(avctx, &s->last_picture); > + FFSWAP(ThreadFrame, s->picture, s->last_picture); > + } > + } > + > *got_frame = 1; > ret = bytestream2_tell(&s->gb); > > @@ -1590,16 +1587,14 @@ static int update_thread_context(AVCodecContext > *dst, const AVCodecContext *src) > { > PNGDecContext *psrc = src->priv_data; > PNGDecContext *pdst = dst->priv_data; > + ThreadFrame *src_frame = NULL; > int ret; > > if (dst == src) > return 0; > > - ff_thread_release_buffer(dst, &pdst->picture); > - if (psrc->picture.f->data[0] && > - (ret = ff_thread_ref_frame(&pdst->picture, &psrc->picture)) < 0) > - return ret; > if (CONFIG_APNG_DECODER && dst->codec_id == AV_CODEC_ID_APNG) { > + > pdst->width = psrc->width; > pdst->height = psrc->height; > pdst->bit_depth = psrc->bit_depth; > @@ -1619,15 +1614,15 @@ static int update_thread_context(AVCodecContext > *dst, const AVCodecContext *src) > memcpy(pdst->palette, psrc->palette, sizeof(pdst->palette)); > > pdst->hdr_state |= psrc->hdr_state; > + } > > - ff_thread_release_buffer(dst, &pdst->last_picture); > - if (psrc->last_picture.f->data[0] && > - (ret = ff_thread_ref_frame(&pdst->last_picture, > &psrc->last_picture)) < 0) > - return ret; > + src_frame = psrc->dispose_op == APNG_DISPOSE_OP_NONE ? > + &psrc->picture : &psrc->last_picture; > > - ff_thread_release_buffer(dst, &pdst->previous_picture); > - if (psrc->previous_picture.f->data[0] && > - (ret = ff_thread_ref_frame(&pdst->previous_picture, > &psrc->previous_picture)) < 0) > + ff_thread_release_buffer(dst, &pdst->last_picture); > + if (src_frame && src_frame->f->data[0]) { > + ret = ff_thread_ref_frame(&pdst->last_picture, src_frame); > + if (ret < 0) > return ret; > } > > @@ -1642,11 +1637,9 @@ static av_cold int png_dec_init(AVCodecContext > *avctx) > avctx->color_range = AVCOL_RANGE_JPEG; > > s->avctx = avctx; > - s->previous_picture.f = av_frame_alloc(); > s->last_picture.f = av_frame_alloc(); > s->picture.f = av_frame_alloc(); > - if (!s->previous_picture.f || !s->last_picture.f || !s->picture.f) { > - av_frame_free(&s->previous_picture.f); > + if (!s->last_picture.f || !s->picture.f) { > av_frame_free(&s->last_picture.f); > av_frame_free(&s->picture.f); > return AVERROR(ENOMEM); > @@ -1661,8 +1654,6 @@ static av_cold int png_dec_end(AVCodecContext *avctx) > { > PNGDecContext *s = avctx->priv_data; > > - ff_thread_release_buffer(avctx, &s->previous_picture); > - av_frame_free(&s->previous_picture.f); > ff_thread_release_buffer(avctx, &s->last_picture); > av_frame_free(&s->last_picture.f); > ff_thread_release_buffer(avctx, &s->picture); > -- > 2.28.0 > > _______________________________________________ > 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".
Quoting Paul B Mahol (2021-02-17 11:52:31) > On Tue, Feb 16, 2021 at 9:26 PM Anton Khirnov <anton@khirnov.net> wrote: > > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext > > *avctx, PNGDecContext *s, > > if (!buffer) > > return AVERROR(ENOMEM); > > > > + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > > - // Do the disposal operation specified by the last frame on the frame > > - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { > > - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > > s->height); > > - > > - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) > > - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; > > ++y) > > - memset(buffer + s->image_linesize * y + s->bpp * > > s->last_x_offset, 0, s->bpp * s->last_w); > > + // need to reset a rectangle to background: > > + // create a new writable copy > > + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { > > + int ret = av_frame_make_writable(s->last_picture.f); > > + if (ret < 0) > > + return ret; > > > > - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize > > * s->height); > > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > > - } else { > > - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); > > - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize > > * s->height); > > + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) > > { > > + memset(s->last_picture.f->data[0] + s->image_linesize * y + > > + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > > + } > > } > > > > + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > > s->height); > > + > > // Perform blending > > if (s->blend_op == APNG_BLEND_OP_SOURCE) { > > for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) { > > @@ -1448,22 +1444,17 @@ exit_loop: > > if (CONFIG_PNG_DECODER && avctx->codec_id != AV_CODEC_ID_APNG) > > handle_p_frame_png(s, p); > > else if (CONFIG_APNG_DECODER && > > - s->previous_picture.f->width == p->width && > > - s->previous_picture.f->height== p->height && > > - s->previous_picture.f->format== p->format && > > > > No explanation provided why those line are removed. > They are removed because previous_picture is removed. As is actually explained in the commit message.
On Wed, Feb 17, 2021 at 12:33 PM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Paul B Mahol (2021-02-17 11:52:31) > > On Tue, Feb 16, 2021 at 9:26 PM Anton Khirnov <anton@khirnov.net> wrote: > > > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext > > > *avctx, PNGDecContext *s, > > > if (!buffer) > > > return AVERROR(ENOMEM); > > > > > > + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > > > > - // Do the disposal operation specified by the last frame on the > frame > > > - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { > > > - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > > > s->height); > > > - > > > - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) > > > - for (y = s->last_y_offset; y < s->last_y_offset + > s->last_h; > > > ++y) > > > - memset(buffer + s->image_linesize * y + s->bpp * > > > s->last_x_offset, 0, s->bpp * s->last_w); > > > + // need to reset a rectangle to background: > > > + // create a new writable copy > > > + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { > > > + int ret = av_frame_make_writable(s->last_picture.f); > > > + if (ret < 0) > > > + return ret; > > > > > > - memcpy(s->previous_picture.f->data[0], buffer, > s->image_linesize > > > * s->height); > > > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > > > - } else { > > > - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); > > > - memcpy(buffer, s->previous_picture.f->data[0], > s->image_linesize > > > * s->height); > > > + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; > y++) > > > { > > > + memset(s->last_picture.f->data[0] + s->image_linesize * y > + > > > + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > > > + } > > > } > > > > > > + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > > > s->height); > > > + > > > // Perform blending > > > if (s->blend_op == APNG_BLEND_OP_SOURCE) { > > > for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) { > > > @@ -1448,22 +1444,17 @@ exit_loop: > > > if (CONFIG_PNG_DECODER && avctx->codec_id != > AV_CODEC_ID_APNG) > > > handle_p_frame_png(s, p); > > > else if (CONFIG_APNG_DECODER && > > > - s->previous_picture.f->width == p->width && > > > - s->previous_picture.f->height== p->height && > > > - s->previous_picture.f->format== p->format && > > > > > > > No explanation provided why those line are removed. > > > > They are removed because previous_picture is removed. As is actually > explained in the commit message. > > But you just replaced previous with last? > -- > Anton Khirnov > _______________________________________________ > 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".
Quoting Paul B Mahol (2021-02-17 14:34:18) > On Wed, Feb 17, 2021 at 12:33 PM Anton Khirnov <anton@khirnov.net> wrote: > > > Quoting Paul B Mahol (2021-02-17 11:52:31) > > > On Tue, Feb 16, 2021 at 9:26 PM Anton Khirnov <anton@khirnov.net> wrote: > > > > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext > > > > *avctx, PNGDecContext *s, > > > > if (!buffer) > > > > return AVERROR(ENOMEM); > > > > > > > > + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > > > > > > - // Do the disposal operation specified by the last frame on the > > frame > > > > - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { > > > > - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > > - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > > > > s->height); > > > > - > > > > - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) > > > > - for (y = s->last_y_offset; y < s->last_y_offset + > > s->last_h; > > > > ++y) > > > > - memset(buffer + s->image_linesize * y + s->bpp * > > > > s->last_x_offset, 0, s->bpp * s->last_w); > > > > + // need to reset a rectangle to background: > > > > + // create a new writable copy > > > > + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { > > > > + int ret = av_frame_make_writable(s->last_picture.f); > > > > + if (ret < 0) > > > > + return ret; > > > > > > > > - memcpy(s->previous_picture.f->data[0], buffer, > > s->image_linesize > > > > * s->height); > > > > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > > > > - } else { > > > > - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); > > > > - memcpy(buffer, s->previous_picture.f->data[0], > > s->image_linesize > > > > * s->height); > > > > + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; > > y++) > > > > { > > > > + memset(s->last_picture.f->data[0] + s->image_linesize * y > > + > > > > + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > > > > + } > > > > } > > > > > > > > + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * > > > > s->height); > > > > + > > > > // Perform blending > > > > if (s->blend_op == APNG_BLEND_OP_SOURCE) { > > > > for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) { > > > > @@ -1448,22 +1444,17 @@ exit_loop: > > > > if (CONFIG_PNG_DECODER && avctx->codec_id != > > AV_CODEC_ID_APNG) > > > > handle_p_frame_png(s, p); > > > > else if (CONFIG_APNG_DECODER && > > > > - s->previous_picture.f->width == p->width && > > > > - s->previous_picture.f->height== p->height && > > > > - s->previous_picture.f->format== p->format && > > > > > > > > > > No explanation provided why those line are removed. > > > > > > > They are removed because previous_picture is removed. As is actually > > explained in the commit message. > > > > > But you just replaced previous with last? The same check is already done for last_picture a few lines above.
On Tue, Feb 16, 2021 at 09:24:15PM +0100, Anton Khirnov wrote: > Current code is very confused and confusing. It uses two different > reference frames - "previous" and "last" - when only one is really > necessary. It also confuses the two, leading to incorrect output with > APNG_DISPOSE_OP_PREVIOUS mode. > > Fixes #9017. > --- > libavcodec/pngdec.c | 93 ++++++++++++++++++++------------------------- > 1 file changed, 42 insertions(+), 51 deletions(-) [...] > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s, > if (!buffer) > return AVERROR(ENOMEM); > > + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > - // Do the disposal operation specified by the last frame on the frame > - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { > - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); > - > - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) > - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y) > - memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > + // need to reset a rectangle to background: > + // create a new writable copy > + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { > + int ret = av_frame_make_writable(s->last_picture.f); > + if (ret < 0) > + return ret; > > - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height); > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > - } else { > - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); > - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height); > + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) { > + memset(s->last_picture.f->data[0] + s->image_linesize * y + > + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > + } > } > > + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); > + This results in out of array reads av_frame_make_writable() decreases linesize [0] but the memcpy() now av_memdup() assumes all frames have the same linesize Thanks [...]
On Tue, Mar 30, 2021 at 10:56:13AM +0200, Michael Niedermayer wrote: > On Tue, Feb 16, 2021 at 09:24:15PM +0100, Anton Khirnov wrote: > > Current code is very confused and confusing. It uses two different > > reference frames - "previous" and "last" - when only one is really > > necessary. It also confuses the two, leading to incorrect output with > > APNG_DISPOSE_OP_PREVIOUS mode. > > > > Fixes #9017. > > --- > > libavcodec/pngdec.c | 93 ++++++++++++++++++++------------------------- > > 1 file changed, 42 insertions(+), 51 deletions(-) > > > [...] > > > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s, > > if (!buffer) > > return AVERROR(ENOMEM); > > > > + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > > - // Do the disposal operation specified by the last frame on the frame > > - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { > > - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); > > - > > - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) > > - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y) > > - memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > > + // need to reset a rectangle to background: > > + // create a new writable copy > > + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { > > + int ret = av_frame_make_writable(s->last_picture.f); > > + if (ret < 0) > > + return ret; > > > > - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height); > > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > > - } else { > > - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); > > - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height); > > + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) { > > + memset(s->last_picture.f->data[0] + s->image_linesize * y + > > + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > > + } > > } > > > > + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); > > + > > This results in out of array reads > > av_frame_make_writable() decreases linesize [0] but the memcpy() now av_memdup() > assumes all frames have the same linesize CCing anton, to make sure this is not missed in its old thread A fix to this also should be backported to release/4.4 Thanks [...]
On 3/30/2021 5:56 AM, Michael Niedermayer wrote: > On Tue, Feb 16, 2021 at 09:24:15PM +0100, Anton Khirnov wrote: >> Current code is very confused and confusing. It uses two different >> reference frames - "previous" and "last" - when only one is really >> necessary. It also confuses the two, leading to incorrect output with >> APNG_DISPOSE_OP_PREVIOUS mode. >> >> Fixes #9017. >> --- >> libavcodec/pngdec.c | 93 ++++++++++++++++++++------------------------- >> 1 file changed, 42 insertions(+), 51 deletions(-) > > > [...] > >> @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s, >> if (!buffer) >> return AVERROR(ENOMEM); >> >> + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); >> >> - // Do the disposal operation specified by the last frame on the frame >> - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { >> - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); >> - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); >> - >> - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) >> - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y) >> - memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); >> + // need to reset a rectangle to background: >> + // create a new writable copy >> + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { >> + int ret = av_frame_make_writable(s->last_picture.f); >> + if (ret < 0) >> + return ret; >> >> - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height); >> - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); >> - } else { >> - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); >> - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height); >> + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) { >> + memset(s->last_picture.f->data[0] + s->image_linesize * y + >> + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); >> + } >> } >> >> + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); >> + > > This results in out of array reads > > av_frame_make_writable() decreases linesize [0] but the memcpy() now av_memdup() > assumes all frames have the same linesize FATE didn't detect this? What sample triggers these out of array reads? Also, instead of av_frame_make_writable(), this should probably call ff_thread_get_buffer() to get a new buffer, then copy the old data if needed. The difference in linesize is because avcodec_default_get_buffer2() allocates buffers in a different way than av_frame_get_buffer() as invoked by av_frame_make_writable(). > > Thanks > > [...] > > > _______________________________________________ > 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". >
On Tue, Mar 30, 2021 at 02:30:08PM -0300, James Almer wrote: > On 3/30/2021 5:56 AM, Michael Niedermayer wrote: > > On Tue, Feb 16, 2021 at 09:24:15PM +0100, Anton Khirnov wrote: > > > Current code is very confused and confusing. It uses two different > > > reference frames - "previous" and "last" - when only one is really > > > necessary. It also confuses the two, leading to incorrect output with > > > APNG_DISPOSE_OP_PREVIOUS mode. > > > > > > Fixes #9017. > > > --- > > > libavcodec/pngdec.c | 93 ++++++++++++++++++++------------------------- > > > 1 file changed, 42 insertions(+), 51 deletions(-) > > > > > > [...] > > > > > @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s, > > > if (!buffer) > > > return AVERROR(ENOMEM); > > > + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > - // Do the disposal operation specified by the last frame on the frame > > > - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { > > > - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); > > > - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); > > > - > > > - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) > > > - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y) > > > - memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > > > + // need to reset a rectangle to background: > > > + // create a new writable copy > > > + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { > > > + int ret = av_frame_make_writable(s->last_picture.f); > > > + if (ret < 0) > > > + return ret; > > > - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height); > > > - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); > > > - } else { > > > - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); > > > - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height); > > > + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) { > > > + memset(s->last_picture.f->data[0] + s->image_linesize * y + > > > + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > > > + } > > > } > > > + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); > > > + > > > > This results in out of array reads > > > > av_frame_make_writable() decreases linesize [0] but the memcpy() now av_memdup() > > assumes all frames have the same linesize > > FATE didn't detect this? What sample triggers these out of array reads? if iam reading my notes correctly 31405/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-6572805215879168 did if someone wants to work on this and wants it i can send it privatly i dont know why fate didnt catch this ... > > Also, instead of av_frame_make_writable(), this should probably call > ff_thread_get_buffer() to get a new buffer, then copy the old data if > needed. The difference in linesize is because avcodec_default_get_buffer2() > allocates buffers in a different way than av_frame_get_buffer() as invoked > by av_frame_make_writable(). yes thx [...]
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index d02c45f1a7..cece08ebca 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -54,7 +54,6 @@ typedef struct PNGDecContext { AVCodecContext *avctx; GetByteContext gb; - ThreadFrame previous_picture; ThreadFrame last_picture; ThreadFrame picture; @@ -711,13 +710,10 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, s->bpp += byte_depth; } + ff_thread_release_buffer(avctx, &s->picture); if ((ret = ff_thread_get_buffer(avctx, &s->picture, AV_GET_BUFFER_FLAG_REF)) < 0) return ret; - if (avctx->codec_id == AV_CODEC_ID_APNG && s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { - ff_thread_release_buffer(avctx, &s->previous_picture); - if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0) - return ret; - } + p->pict_type = AV_PICTURE_TYPE_I; p->key_frame = 1; p->interlaced_frame = !!s->interlace_type; @@ -1020,7 +1016,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, return AVERROR_INVALIDDATA; } - if ((sequence_number == 0 || !s->previous_picture.f->data[0]) && + if ((sequence_number == 0 || !s->last_picture.f->data[0]) && dispose_op == APNG_DISPOSE_OP_PREVIOUS) { // No previous frame to revert to for the first frame // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND @@ -1088,23 +1084,23 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s, if (!buffer) return AVERROR(ENOMEM); + ff_thread_await_progress(&s->last_picture, INT_MAX, 0); - // Do the disposal operation specified by the last frame on the frame - if (s->last_dispose_op != APNG_DISPOSE_OP_PREVIOUS) { - ff_thread_await_progress(&s->last_picture, INT_MAX, 0); - memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); - - if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) - for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; ++y) - memset(buffer + s->image_linesize * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); + // need to reset a rectangle to background: + // create a new writable copy + if (s->last_dispose_op == APNG_DISPOSE_OP_BACKGROUND) { + int ret = av_frame_make_writable(s->last_picture.f); + if (ret < 0) + return ret; - memcpy(s->previous_picture.f->data[0], buffer, s->image_linesize * s->height); - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); - } else { - ff_thread_await_progress(&s->previous_picture, INT_MAX, 0); - memcpy(buffer, s->previous_picture.f->data[0], s->image_linesize * s->height); + for (y = s->last_y_offset; y < s->last_y_offset + s->last_h; y++) { + memset(s->last_picture.f->data[0] + s->image_linesize * y + + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); + } } + memcpy(buffer, s->last_picture.f->data[0], s->image_linesize * s->height); + // Perform blending if (s->blend_op == APNG_BLEND_OP_SOURCE) { for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) { @@ -1448,22 +1444,17 @@ exit_loop: if (CONFIG_PNG_DECODER && avctx->codec_id != AV_CODEC_ID_APNG) handle_p_frame_png(s, p); else if (CONFIG_APNG_DECODER && - s->previous_picture.f->width == p->width && - s->previous_picture.f->height== p->height && - s->previous_picture.f->format== p->format && avctx->codec_id == AV_CODEC_ID_APNG && (ret = handle_p_frame_apng(avctx, s, p)) < 0) goto fail; } } ff_thread_report_progress(&s->picture, INT_MAX, 0); - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); return 0; fail: ff_thread_report_progress(&s->picture, INT_MAX, 0); - ff_thread_report_progress(&s->previous_picture, INT_MAX, 0); return ret; } @@ -1475,14 +1466,10 @@ static int decode_frame_png(AVCodecContext *avctx, PNGDecContext *const s = avctx->priv_data; const uint8_t *buf = avpkt->data; int buf_size = avpkt->size; - AVFrame *p; + AVFrame *p = s->picture.f; int64_t sig; int ret; - ff_thread_release_buffer(avctx, &s->last_picture); - FFSWAP(ThreadFrame, s->picture, s->last_picture); - p = s->picture.f; - bytestream2_init(&s->gb, buf, buf_size); /* check signature */ @@ -1519,6 +1506,11 @@ static int decode_frame_png(AVCodecContext *avctx, if ((ret = av_frame_ref(data, s->picture.f)) < 0) goto the_end; + if (!(avctx->active_thread_type & FF_THREAD_FRAME)) { + ff_thread_release_buffer(avctx, &s->last_picture); + FFSWAP(ThreadFrame, s->picture, s->last_picture); + } + *got_frame = 1; ret = bytestream2_tell(&s->gb); @@ -1536,11 +1528,7 @@ static int decode_frame_apng(AVCodecContext *avctx, { PNGDecContext *const s = avctx->priv_data; int ret; - AVFrame *p; - - ff_thread_release_buffer(avctx, &s->last_picture); - FFSWAP(ThreadFrame, s->picture, s->last_picture); - p = s->picture.f; + AVFrame *p = s->picture.f; if (!(s->hdr_state & PNG_IHDR)) { if (!avctx->extradata_size) @@ -1576,6 +1564,15 @@ static int decode_frame_apng(AVCodecContext *avctx, if ((ret = av_frame_ref(data, s->picture.f)) < 0) goto end; + if (!(avctx->active_thread_type & FF_THREAD_FRAME)) { + if (s->dispose_op == APNG_DISPOSE_OP_PREVIOUS) { + ff_thread_release_buffer(avctx, &s->picture); + } else if (s->dispose_op == APNG_DISPOSE_OP_NONE) { + ff_thread_release_buffer(avctx, &s->last_picture); + FFSWAP(ThreadFrame, s->picture, s->last_picture); + } + } + *got_frame = 1; ret = bytestream2_tell(&s->gb); @@ -1590,16 +1587,14 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) { PNGDecContext *psrc = src->priv_data; PNGDecContext *pdst = dst->priv_data; + ThreadFrame *src_frame = NULL; int ret; if (dst == src) return 0; - ff_thread_release_buffer(dst, &pdst->picture); - if (psrc->picture.f->data[0] && - (ret = ff_thread_ref_frame(&pdst->picture, &psrc->picture)) < 0) - return ret; if (CONFIG_APNG_DECODER && dst->codec_id == AV_CODEC_ID_APNG) { + pdst->width = psrc->width; pdst->height = psrc->height; pdst->bit_depth = psrc->bit_depth; @@ -1619,15 +1614,15 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) memcpy(pdst->palette, psrc->palette, sizeof(pdst->palette)); pdst->hdr_state |= psrc->hdr_state; + } - ff_thread_release_buffer(dst, &pdst->last_picture); - if (psrc->last_picture.f->data[0] && - (ret = ff_thread_ref_frame(&pdst->last_picture, &psrc->last_picture)) < 0) - return ret; + src_frame = psrc->dispose_op == APNG_DISPOSE_OP_NONE ? + &psrc->picture : &psrc->last_picture; - ff_thread_release_buffer(dst, &pdst->previous_picture); - if (psrc->previous_picture.f->data[0] && - (ret = ff_thread_ref_frame(&pdst->previous_picture, &psrc->previous_picture)) < 0) + ff_thread_release_buffer(dst, &pdst->last_picture); + if (src_frame && src_frame->f->data[0]) { + ret = ff_thread_ref_frame(&pdst->last_picture, src_frame); + if (ret < 0) return ret; } @@ -1642,11 +1637,9 @@ static av_cold int png_dec_init(AVCodecContext *avctx) avctx->color_range = AVCOL_RANGE_JPEG; s->avctx = avctx; - s->previous_picture.f = av_frame_alloc(); s->last_picture.f = av_frame_alloc(); s->picture.f = av_frame_alloc(); - if (!s->previous_picture.f || !s->last_picture.f || !s->picture.f) { - av_frame_free(&s->previous_picture.f); + if (!s->last_picture.f || !s->picture.f) { av_frame_free(&s->last_picture.f); av_frame_free(&s->picture.f); return AVERROR(ENOMEM); @@ -1661,8 +1654,6 @@ static av_cold int png_dec_end(AVCodecContext *avctx) { PNGDecContext *s = avctx->priv_data; - ff_thread_release_buffer(avctx, &s->previous_picture); - av_frame_free(&s->previous_picture.f); ff_thread_release_buffer(avctx, &s->last_picture); av_frame_free(&s->last_picture.f); ff_thread_release_buffer(avctx, &s->picture);