Hi,
2015-02-05 19:19 GMT+01:00 Michael Niedermayer <michae...@gmx.at>:
>> + 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
I don't think the padding is actually needed because it's just plain C
accessing this. There's a dsp restore function but that is unlikely to
get simd.
So, maybe that is sufficient: width = 4 * sps->width +
FF_INPUT_BUFFER_PADDING_SIZE.
Same for the other allocation.
>> 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 ?
At first I thought this was a threading bug, eg a thread accessing
something no longer valid.
That would have been the reason why there was a reference-counted
frame. But the sequence having an issue is one where there are
multiple parameter sets, so this code gets executed twice. Because the
code didn't free the buffers, there was a leak
So, you're right, it's just a fancy way of freeing the buffers. As you
said, one can probably add the needed av_freep, something like the
attached patch.
It is untested, though, as I can't test it right now.
--
Christophe
From 7945866d0b24fc3c97f98635736e553ba541422d Mon Sep 17 00:00:00 2001
From: Christophe Gisquet <christophe.gisq...@gmail.com>
Date: Thu, 5 Feb 2015 19:51:22 +0100
Subject: [PATCH] hevc: free sao buffers when receiving a new SPS
The buffer pointers would be otherwise overwritten, causing a
leak on e.g. PERSIST_RPARAM_A_RExt_Sony_1.
---
libavcodec/hevc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
index 0624cb0..afbfda1 100644
--- a/libavcodec/hevc.c
+++ b/libavcodec/hevc.c
@@ -284,7 +284,7 @@ static int set_sps(HEVCContext *s, const HEVCSPS *sps)
{
#define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL)
enum AVPixelFormat pix_fmts[HWACCEL_MAX + 2], *fmt = pix_fmts;
- int ret;
+ int ret, i;
unsigned int num = 0, den = 0;
pic_arrays_free(s);
@@ -334,6 +334,13 @@ static int set_sps(HEVCContext *s, const HEVCSPS *sps)
ff_hevc_dsp_init (&s->hevcdsp, sps->bit_depth);
ff_videodsp_init (&s->vdsp, sps->bit_depth);
+ for (i = 0; i < 3; i++) {
+ if (s->sao_pixel_buffer_h[i])
+ av_freep(&s->sao_pixel_buffer_h[i]);
+ if (s->sao_pixel_buffer_v[i])
+ av_freep(&s->sao_pixel_buffer_v[i]);
+ }
+
if (sps->sao_enabled && !s->avctx->hwaccel) {
int c_count = (sps->chroma_format_idc != 0) ? 3 : 1;
int c_idx;
--
1.9.2.msysgit.0
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
