[FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO

[Michael Niedermayer]

On Thu, Feb 05, 2015 at 08:17:25AM +0100, Christophe Gisquet wrote:
> Hi,
> 
> 2015-02-05 7:29 GMT+01:00 Christophe Gisquet <christophe.gisquet at gmail.com>:
> > We were previously reference-counting the sao-buffer. Should we do
> > that for sao_pixel_buffer_[hv], then?
> 
> Something like the attached patch.
> 
> Note: I'm probably overallocating compared to previously, but that
> doesn't look to be a big deal.
> 
> On the other hand, those are buffers sized 2*block_height*image_width
> and 2*block_width*image_height.
> The storing is maybe overzealous, as I don't see why just more than 1
> or 2 lines/columns need to be stored.
> 
> -- 
> Christophe

>  hevc.c        |   50 +++++++++++++++++++++++++++++++++-----------------
>  hevc.h        |    4 ++--
>  hevc_filter.c |   18 ++++++++++--------
>  3 files changed, 45 insertions(+), 27 deletions(-)
> 139899fbe6e35afb746820042742bbae86da69f8  0001-hevc-sao-referenced-line-column-buffers.patch
> From 8ac4c9e5db63de26523e4677f397019c5eb6bd89 Mon Sep 17 00:00:00 2001
> From: Christophe Gisquet <christophe.gisquet at gmail.com>
> Date: Thu, 5 Feb 2015 08:11:28 +0100
> Subject: [PATCH] hevc/sao: referenced line/column buffers
> 
> Otherwise, a change in parameters may cause the buffers to be lost,
> causing memory leak.
> ---
>  libavcodec/hevc.c        | 50 ++++++++++++++++++++++++++++++++----------------
>  libavcodec/hevc.h        |  4 ++--
>  libavcodec/hevc_filter.c | 18 +++++++++--------
>  3 files changed, 45 insertions(+), 27 deletions(-)
> 
> diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
> index 0624cb0..9a6df3c 100644
> --- a/libavcodec/hevc.c
> +++ b/libavcodec/hevc.c
> @@ -280,6 +280,23 @@ static int decode_lt_rps(HEVCContext *s, LongTermRPS *rps, GetBitContext *gb)
>      return 0;
>  }
>  
> +static int get_buffer_sao(HEVCContext *s, const HEVCSPS *sps)
> +{
> +    int ret;
> +    AVFrame *frame = s->tmp_frame[0];

> +    frame->width  = FFALIGN(4 * sps->width, FF_INPUT_BUFFER_PADDING_SIZE);

FF_INPUT_BUFFER_PADDING_SIZE is not guranteed to be a power of 2 but
FFALIGN needs a power of 2


> +    frame->height = sps->ctb_height;
> +    if ((ret = ff_get_buffer(s->avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0)
> +        return ret;
> +
> +    frame = s->tmp_frame[1];
> +    frame->width  = FFALIGN(4 * sps->height, FF_INPUT_BUFFER_PADDING_SIZE);
> +    frame->height = sps->ctb_width;
> +    if ((ret = ff_get_buffer(s->avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0)
> +        return ret;
> +    return 0;
> +}
> +
>  static int set_sps(HEVCContext *s, const HEVCSPS *sps)
>  {
>      #define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL)
> @@ -335,19 +352,14 @@ static int set_sps(HEVCContext *s, const HEVCSPS *sps)
>      ff_videodsp_init (&s->vdsp,    sps->bit_depth);
>  
>      if (sps->sao_enabled && !s->avctx->hwaccel) {
> -        int c_count = (sps->chroma_format_idc != 0) ? 3 : 1;
> -        int c_idx;
> -
> -        for(c_idx = 0; c_idx < c_count; c_idx++) {
> -            int w = sps->width >> sps->hshift[c_idx];
> -            int h = sps->height >> sps->vshift[c_idx];
> -            s->sao_pixel_buffer_h[c_idx] =
> -                av_malloc((w * 2 * sps->ctb_height) <<
> -                          sps->pixel_shift);
> -            s->sao_pixel_buffer_v[c_idx] =
> -                av_malloc((h * 2 * sps->ctb_width) <<
> -                          sps->pixel_shift);
> -        }
> +        av_frame_unref(s->tmp_frame[0]);
> +        av_frame_unref(s->tmp_frame[1]);
> +        s->sao_frame[0] = NULL;
> +        s->sao_frame[1] = NULL;
> +        if ( (ret = get_buffer_sao(s, sps)) < 0 )
> +            goto fail;
> +        s->sao_frame[0] = s->tmp_frame[0];
> +        s->sao_frame[1] = s->tmp_frame[1];

I dont understand this code
tmp_frame is allocated then the pointer copied into sao_frame
later then tmp_frame is unreferenced which makes sao_frame a stale
pointer if its still pointing to the same memory
it seems the code works though with make fate even under valgrind,
so maybe iam missing something but wouldnt a simply av_freep() with
the existing code achive the same ?

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150205/c3a05f9d/attachment.asc>