diff mbox series

[FFmpeg-devel,10/16] avcodec/svq3: Avoid allocations for SVQ3Frames

Message ID 20200913025753.274772-10-andreas.rheinhardt@gmail.com
State Accepted
Commit 96061c5a4f690c3ab49e4458701bb013fd3dd57f
Headers show
Series [FFmpeg-devel,01/16] avcodec/snowdec: Use ff_snow_common_init() directly
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 13, 2020, 2:57 a.m. UTC
These frames can be made part of the SVQ3Context; notice that the pointers
to the frames have been retained in order to allow to just swap them as
the code already does.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/svq3.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

Comments

Andreas Rheinhardt Sept. 13, 2020, 5:28 p.m. UTC | #1
Andreas Rheinhardt:
> These frames can be made part of the SVQ3Context; notice that the pointers
> to the frames have been retained in order to allow to just swap them as
> the code already does.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/svq3.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c
> index c8db08a32f..8a67836827 100644
> --- a/libavcodec/svq3.c
> +++ b/libavcodec/svq3.c
> @@ -147,6 +147,7 @@ typedef struct SVQ3Context {
>      DECLARE_ALIGNED(8, uint8_t, non_zero_count_cache)[15 * 8];
>      uint32_t dequant4_coeff[QP_MAX_NUM + 1][16];
>      int block_offset[2 * (16 * 3)];
> +    SVQ3Frame frames[3];
>  } SVQ3Context;
>  
>  #define FULLPEL_MODE  1
> @@ -1135,13 +1136,9 @@ static av_cold int svq3_decode_init(AVCodecContext *avctx)
>      int marker_found = 0;
>      int ret;
>  
> -    s->cur_pic  = av_mallocz(sizeof(*s->cur_pic));
> -    s->last_pic = av_mallocz(sizeof(*s->last_pic));
> -    s->next_pic = av_mallocz(sizeof(*s->next_pic));
> -    if (!s->next_pic || !s->last_pic || !s->cur_pic) {
> -        ret = AVERROR(ENOMEM);
> -        goto fail;
> -    }
> +    s->cur_pic  = &s->frames[0];
> +    s->last_pic = &s->frames[1];
> +    s->next_pic = &s->frames[2];
>  
>      s->cur_pic->f  = av_frame_alloc();
>      s->last_pic->f = av_frame_alloc();
> @@ -1631,9 +1628,6 @@ static av_cold int svq3_decode_end(AVCodecContext *avctx)
>      av_frame_free(&s->cur_pic->f);
>      av_frame_free(&s->next_pic->f);
>      av_frame_free(&s->last_pic->f);
> -    av_freep(&s->cur_pic);
> -    av_freep(&s->next_pic);
> -    av_freep(&s->last_pic);
>      av_freep(&s->slice_buf);
>      av_freep(&s->intra4x4_pred_mode);
>      av_freep(&s->edge_emu_buffer);
> 
I have just realized that going to fail as the old code above does is
dangerous as the close function presumes the said frames to have been
successfully allocated. Therefore I have modified the commit message as
follows:

avcodec/svq3: Fix segfault on allocation error, avoid allocations

The very first thing the SVQ3 decoder currently does is allocating several
SVQ3Frames, a structure which contains members that need to be freed on
their own. If one of these allocations fails, the decoder calls its own
close function to not leak the already allocated SVQ3Frames. Yet said
function presumes that the SVQ3Frames have been successfully allocated
as there is no check before freeing the members that need to be freed.

This commit fixes this by making these frames part of the SVQ3Context,
thereby avoiding the allocations altogether. Notice that the pointers
to the frames have been retained in order to allow to just swap them as
the code already does.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c
index c8db08a32f..8a67836827 100644
--- a/libavcodec/svq3.c
+++ b/libavcodec/svq3.c
@@ -147,6 +147,7 @@  typedef struct SVQ3Context {
     DECLARE_ALIGNED(8, uint8_t, non_zero_count_cache)[15 * 8];
     uint32_t dequant4_coeff[QP_MAX_NUM + 1][16];
     int block_offset[2 * (16 * 3)];
+    SVQ3Frame frames[3];
 } SVQ3Context;
 
 #define FULLPEL_MODE  1
@@ -1135,13 +1136,9 @@  static av_cold int svq3_decode_init(AVCodecContext *avctx)
     int marker_found = 0;
     int ret;
 
-    s->cur_pic  = av_mallocz(sizeof(*s->cur_pic));
-    s->last_pic = av_mallocz(sizeof(*s->last_pic));
-    s->next_pic = av_mallocz(sizeof(*s->next_pic));
-    if (!s->next_pic || !s->last_pic || !s->cur_pic) {
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
+    s->cur_pic  = &s->frames[0];
+    s->last_pic = &s->frames[1];
+    s->next_pic = &s->frames[2];
 
     s->cur_pic->f  = av_frame_alloc();
     s->last_pic->f = av_frame_alloc();
@@ -1631,9 +1628,6 @@  static av_cold int svq3_decode_end(AVCodecContext *avctx)
     av_frame_free(&s->cur_pic->f);
     av_frame_free(&s->next_pic->f);
     av_frame_free(&s->last_pic->f);
-    av_freep(&s->cur_pic);
-    av_freep(&s->next_pic);
-    av_freep(&s->last_pic);
     av_freep(&s->slice_buf);
     av_freep(&s->intra4x4_pred_mode);
     av_freep(&s->edge_emu_buffer);