| Message ID | 20210408084654.20863-1-anton@khirnov.net |
|---|---|
| State | Accepted |
| Commit | b593abda6c642cb0c3959752dd235c2faf66837f |
| Headers | show |
| Series | [FFmpeg-devel] lavc/pngdec: always create a copy for APNG_DISPOSE_OP_BACKGROUND | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure | warning | Failed to apply patch |
Anton Khirnov: > Calling av_frame_make_writable() from decoders is tricky, especially > when frame threading is used. It is much simpler and safer to just make > a private copy of the frame. > This is not expected to have a major performance impact, since > APNG_DISPOSE_OP_BACKGROUND is not used often and > av_frame_make_writable() would typically make a copy anyway. > > Found-by: James Almer <jamrial@gmail.com> > --- > libavcodec/pngdec.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > index 562c5ffea4..3e509e8ae0 100644 > --- a/libavcodec/pngdec.c > +++ b/libavcodec/pngdec.c > @@ -89,6 +89,8 @@ 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; > @@ -1079,19 +1081,20 @@ 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: > - // 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; > + av_fast_malloc(&s->background_buf, &s->background_buf_allocated, > + src_stride * p->height); No check for whether the frame is already writable? > + if (!s->background_buf) > + return AVERROR(ENOMEM); > > - src = s->last_picture.f->data[0]; > - src_stride = s->last_picture.f->linesize[0]; > + 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->last_picture.f->data[0] + src_stride * y + > + memset(s->background_buf + src_stride * y + > s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); > } > + > + src = s->background_buf; > } > > // copy unchanged rectangles from the last frame > @@ -1716,6 +1719,7 @@ 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); >
Quoting Andreas Rheinhardt (2021-04-08 10:56:42) > Anton Khirnov: > > Calling av_frame_make_writable() from decoders is tricky, especially > > when frame threading is used. It is much simpler and safer to just make > > a private copy of the frame. > > This is not expected to have a major performance impact, since > > APNG_DISPOSE_OP_BACKGROUND is not used often and > > av_frame_make_writable() would typically make a copy anyway. > > > > Found-by: James Almer <jamrial@gmail.com> > > --- > > libavcodec/pngdec.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c > > index 562c5ffea4..3e509e8ae0 100644 > > --- a/libavcodec/pngdec.c > > +++ b/libavcodec/pngdec.c > > @@ -89,6 +89,8 @@ 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; > > @@ -1079,19 +1081,20 @@ 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: > > - // 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; > > + av_fast_malloc(&s->background_buf, &s->background_buf_allocated, > > + src_stride * p->height); > > No check for whether the frame is already writable? Yes, it doesn't seem worth the extra code. Frankly I've had enough of pngdec bugs and would just prefer to be done with it.
On Thu, Apr 08, 2021 at 10:46:54AM +0200, Anton Khirnov wrote: > Calling av_frame_make_writable() from decoders is tricky, especially > when frame threading is used. It is much simpler and safer to just make > a private copy of the frame. > This is not expected to have a major performance impact, since > APNG_DISPOSE_OP_BACKGROUND is not used often and > av_frame_make_writable() would typically make a copy anyway. > > Found-by: James Almer <jamrial@gmail.com> > --- > libavcodec/pngdec.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) LGTM thx [...]
On Thu, Apr 08, 2021 at 05:34:30PM +0200, Michael Niedermayer wrote: > On Thu, Apr 08, 2021 at 10:46:54AM +0200, Anton Khirnov wrote: > > Calling av_frame_make_writable() from decoders is tricky, especially > > when frame threading is used. It is much simpler and safer to just make > > a private copy of the frame. > > This is not expected to have a major performance impact, since > > APNG_DISPOSE_OP_BACKGROUND is not used often and > > av_frame_make_writable() would typically make a copy anyway. > > > > Found-by: James Almer <jamrial@gmail.com> > > --- > > libavcodec/pngdec.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > LGTM i will apply and backport this if it has not been applied when i make the 4.4 release thx [...]
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index 562c5ffea4..3e509e8ae0 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -89,6 +89,8 @@ 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; @@ -1079,19 +1081,20 @@ 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: - // 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; + av_fast_malloc(&s->background_buf, &s->background_buf_allocated, + src_stride * p->height); + if (!s->background_buf) + return AVERROR(ENOMEM); - src = s->last_picture.f->data[0]; - src_stride = s->last_picture.f->linesize[0]; + 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->last_picture.f->data[0] + src_stride * y + + memset(s->background_buf + src_stride * y + s->bpp * s->last_x_offset, 0, s->bpp * s->last_w); } + + src = s->background_buf; } // copy unchanged rectangles from the last frame @@ -1716,6 +1719,7 @@ 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);