diff mbox series

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

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
Related show

Checks

Context Check Description
andriy/configure warning Failed to apply patch
andriy/configure warning Failed to apply patch

Commit Message

Anton Khirnov April 8, 2021, 8:46 a.m. UTC
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(-)

Comments

Andreas Rheinhardt April 8, 2021, 8:56 a.m. UTC | #1
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);
>
Anton Khirnov April 8, 2021, 9:32 a.m. UTC | #2
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.
Michael Niedermayer April 8, 2021, 3:34 p.m. UTC | #3
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

[...]
Michael Niedermayer April 8, 2021, 7:59 p.m. UTC | #4
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 mbox series

Patch

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);