[FFmpeg-devel] [PATCH] lavc/pngdec: always create a copy for APNG_DISPOSE_OP_BACKGROUND

[Anton Khirnov]

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 at 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.

-- 
Anton Khirnov