Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
Hi,
2015-02-05 19:19 GMT+01:00 Michael Niedermayer :
>> +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
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
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
On Thu, Feb 05, 2015 at 08:17:25AM +0100, Christophe Gisquet wrote:
> Hi,
>
> 2015-02-05 7:29 GMT+01:00 Christophe Gisquet :
> > 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
> 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
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
On Thu, Feb 05, 2015 at 06:47:08PM +0100, Michael Niedermayer wrote: > On Thu, Feb 05, 2015 at 01:57:48PM +0100, Mickaël Raulet wrote: > > PPS_A_qualcomm_7, yes I have sometimes an issue with this sequence. > > > > Patch LGTM otherwise, it does not change the behaviour of the previous > > implementation. > > applied not pushed yet, ive a few questions about the code [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
On Thu, Feb 05, 2015 at 01:57:48PM +0100, Mickaël Raulet wrote: > PPS_A_qualcomm_7, yes I have sometimes an issue with this sequence. > > Patch LGTM otherwise, it does not change the behaviour of the previous > implementation. applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
Hi, 2015-02-05 11:13 GMT+01:00 Christophe Gisquet : > 2015-02-05 10:13 GMT+01:00 Christophe Gisquet : >> The patch breaks make fate-hevc THREADS=3, so needs more thought. > > Compilation issue, running make clean first passes "fate-hevc > THREADS=3" and "fate-hevc THREADS=3 THREAD_TYPE=slice" Ran with "--enable-gpl --enable-memory-poisoning --enable-avresample --valgrind=valgrind --disable-stripping" (still strip binaries and asm object files!?) on a CentOS system. Seems to pass with the different fate parameters above. -- Christophe ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
PPS_A_qualcomm_7, yes I have sometimes an issue with this sequence. Patch LGTM otherwise, it does not change the behaviour of the previous implementation. Mickaël 2015-02-05 13:36 GMT+01:00 Christophe Gisquet : > Hi, > > 2015-02-05 13:17 GMT+01:00 Mickaël Raulet : > > on one sequence? PPS7_xxx ? Right? > > I think we're not speaking about the same thing, maybe. I had an > issue, starting right from the first fate-hevc-conformance sequence, > where ffmpeg wouldn't even parse correctly the SPS. > > Rebuilding from scratch fixed the issue, and the patch runs fine under > fate with THREADS=3 and with or without THREAD_TYPE=slice. Though, I > have no idea if it actually fixes the fate failure on the > aforementioned instance. > > I haven't seen any issue with PPS7_whatever (PPS_A_qualcomm_7?). > > -- > Christophe > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
Hi, 2015-02-05 13:17 GMT+01:00 Mickaël Raulet : > on one sequence? PPS7_xxx ? Right? I think we're not speaking about the same thing, maybe. I had an issue, starting right from the first fate-hevc-conformance sequence, where ffmpeg wouldn't even parse correctly the SPS. Rebuilding from scratch fixed the issue, and the patch runs fine under fate with THREADS=3 and with or without THREAD_TYPE=slice. Though, I have no idea if it actually fixes the fate failure on the aforementioned instance. I haven't seen any issue with PPS7_whatever (PPS_A_qualcomm_7?). -- Christophe ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
on one sequence? PPS7_xxx ? Right? Mickaël 2015-02-05 11:13 GMT+01:00 Christophe Gisquet : > 2015-02-05 10:13 GMT+01:00 Christophe Gisquet < > christophe.gisq...@gmail.com>: > > The patch breaks make fate-hevc THREADS=3, so needs more thought. > > Compilation issue, running make clean first passes "fate-hevc > THREADS=3" and "fate-hevc THREADS=3 THREAD_TYPE=slice" > > -- > Christophe > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
2015-02-05 10:13 GMT+01:00 Christophe Gisquet : > The patch breaks make fate-hevc THREADS=3, so needs more thought. Compilation issue, running make clean first passes "fate-hevc THREADS=3" and "fate-hevc THREADS=3 THREAD_TYPE=slice" -- Christophe ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
Hi, 2015-02-05 9:48 GMT+01:00 Mickaël Raulet : > WPP try it out with thread_type=slice. Does it mean these buffer should rather be per thread? If having 2 ctb lines of buffer fixes this, does this mean having 2 instances of a single line/column, one per ctb line number parity, would equally fix things? The patch breaks make fate-hevc THREADS=3, so needs more thought. -- Christophe ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
2015-02-05 8:17 GMT+01:00 Christophe Gisquet : > Hi, > > 2015-02-05 7:29 GMT+01:00 Christophe Gisquet >: > > 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. > WPP try it out with thread_type=slice. Mickaël ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
Hi,
2015-02-05 7:29 GMT+01:00 Christophe Gisquet :
> 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
From 8ac4c9e5db63de26523e4677f397019c5eb6bd89 Mon Sep 17 00:00:00 2001
From: Christophe Gisquet
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);
+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];
}
s->sps = sps;
@@ -3178,10 +3190,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
av_freep(&s->cabac_state);
-for (i = 0; i < 3; i++) {
-av_freep(&s->sao_pixel_buffer_h[i]);
-av_freep(&s->sao_pixel_buffer_v[i]);
-}
+av_frame_free(&s->tmp_frame[0]);
+av_frame_free(&s->tmp_frame[1]);
av_frame_free(&s->output_frame);
for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
@@ -3241,6 +3251,12 @@ static av_cold int hevc_init_context(AVCodecContext *avctx)
if (!s->cabac_state)
goto fail;
+for (i = 0; i < 2; i++) {
+s->tmp_frame[i] = av_frame_alloc();
+if (!s->tmp_frame[i])
+goto fail;
+}
+
s->output_frame = av_frame_alloc();
if (!s->output_frame)
goto fail;
diff --git a/libavcodec/hevc.h b/libavcodec/hevc.h
index ae9a32a..5ec7a23 100644
--- a/libavcodec/hevc.h
+++ b/libavcodec/hevc.h
@@ -809,8 +809,8 @@ typedef struct HEVCContext {
AVFrame *frame;
AVFrame *output_frame;
-uint8_t *sao_pixel_buffer_h[3];
-uint8_t *sao_pixel_buffer_v[3];
+AVFrame *tmp_frame[2];
+AVFrame *sao_frame[2];
const HEVCVPS *vps;
const HEVCSPS *sps;
diff --git a/libavcodec/hevc_filter.c b/libavcodec/hevc_filter.c
index 1172282..63d83c1 100644
--- a/libavcodec/hevc_filter.c
+++ b/libavcodec/hevc_filter.c
@@ -196,17 +196,19 @@ static void copy_CTB_to_hv(HEVCContext *s, const uint8_t *src,
int sh = s->sps->pixel_shift;
int w = s->sps->width >> s->sps->hshift[c_idx];
int h = s->sps->height >> s->sps->vshift[c_idx];
+uint8_t *buffer_h = s->sao_frame[0]->data[c_idx];
+uint8_t *buffer_v = s->sao_frame[1]->data[c_idx];
/* copy horizontal edges */
-memcpy(s->sao_pixel_buffer_h[c_idx] + (((2 * y_ctb) * w + x) << sh),
+memcpy(buffer_h + (((2 * y_ctb) * w + x) << sh),
src, width << sh);
-memcpy(s->sao_pixel_buffer_h[c_idx] + (((2 * y_ctb + 1) * w + x) <<
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
Hi,
2015-02-04 2:05 GMT+01:00 Michael Niedermayer :
> applied the cherry picked code and the update
James mentioned this issue:
http://fate.ffmpeg.org/report.cgi?time=20150205015545&slot=x86_64-archlinux-gcc-valgrindundef
This looks like a missing free:
==15442==at 0x4C2C526: memalign (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15442==by 0x4C2C641: posix_memalign (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15442==by 0xD21B86: av_malloc (mem.c:95)
==15442==by 0x7B8E9E: set_sps (hevc.c:345)
for sao_pixel_buffer_[hv].
We do have the cleaning code in hevc_decode_free:
for (i = 0; i < 3; i++) {
av_freep(&s->sao_pixel_buffer_h[i]);
av_freep(&s->sao_pixel_buffer_v[i]);
still there is that issue.
We were previously reference-counting the sao-buffer. Should we do
that for sao_pixel_buffer_[hv], then?
--
Christophe
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
On Mon, Feb 02, 2015 at 04:02:41PM +0100, Michael Niedermayer wrote: > On Mon, Feb 02, 2015 at 03:31:54PM +0100, Michael Niedermayer wrote: > > On Mon, Feb 02, 2015 at 02:22:36PM +0100, Christophe Gisquet wrote: > > > Hi, > > > > > > 2015-02-02 13:32 GMT+01:00 Michael Niedermayer : > > > > On Mon, Feb 02, 2015 at 07:41:54AM +0100, Christophe Gisquet wrote: > > > > hmm, is there a reason not to take the original commit unchanged ? > > > > I was hoping to reduce the difference to openhevc so that we also > > > > are able to merge future changes from openhevc with few confilcts > > > > but maybe iam missing something > > > > > > Because there are alignment requirements in our dsp, and technically, > > > adding another buffer (which isn't aligned) while there are already > > > perfectly good ones is not the best solution (memory-wise and > > > bookkeeping-wise). > > > > > > > > I'd go as far as suggest openhevc to align to this version. But maybe > > > they have diverged too much. > > > > iam happy with either openhevc picking this version or us picking > > the one from openhevc with minimal changes needed to make it work for > > us.. > > > Though in the first case we should pick the openhevc version and > > comit our changes in a seperate commit on top so openhevc can > > more easily pick the changes we made if they want to. > > For reference attached the difference between the 2 implementations > that is diff between this version and the one cherry picked from > openhevc (actually cherry picked yesterday and rebased to HEAD) applied the cherry picked code and the update Thanks to all [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Let us carefully observe those good qualities wherein our enemies excel us and endeavor to excel them, by avoiding what is faulty, and imitating what is excellent in them. -- Plutarch signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
On Mon, Feb 02, 2015 at 03:31:54PM +0100, Michael Niedermayer wrote:
> On Mon, Feb 02, 2015 at 02:22:36PM +0100, Christophe Gisquet wrote:
> > Hi,
> >
> > 2015-02-02 13:32 GMT+01:00 Michael Niedermayer :
> > > On Mon, Feb 02, 2015 at 07:41:54AM +0100, Christophe Gisquet wrote:
> > > hmm, is there a reason not to take the original commit unchanged ?
> > > I was hoping to reduce the difference to openhevc so that we also
> > > are able to merge future changes from openhevc with few confilcts
> > > but maybe iam missing something
> >
> > Because there are alignment requirements in our dsp, and technically,
> > adding another buffer (which isn't aligned) while there are already
> > perfectly good ones is not the best solution (memory-wise and
> > bookkeeping-wise).
> >
>
> > I'd go as far as suggest openhevc to align to this version. But maybe
> > they have diverged too much.
>
> iam happy with either openhevc picking this version or us picking
> the one from openhevc with minimal changes needed to make it work for
> us..
> Though in the first case we should pick the openhevc version and
> comit our changes in a seperate commit on top so openhevc can
> more easily pick the changes we made if they want to.
For reference attached the difference between the 2 implementations
that is diff between this version and the one cherry picked from
openhevc (actually cherry picked yesterday and rebased to HEAD)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No snowflake in an avalanche ever feels responsible. -- Voltaire
From d6c12c28f8a03edebab03f18631fa30ade01d0da Mon Sep 17 00:00:00 2001
From: Christophe Gisquet
Date: Mon, 2 Feb 2015 15:51:45 +0100
Subject: [PATCH 2/2] Changes on top of openhevc
Signed-off-by: Michael Niedermayer
---
libavcodec/hevc.c| 65 +++---
libavcodec/hevc.h| 11 +---
libavcodec/hevc_filter.c | 56 ++-
3 files changed, 18 insertions(+), 114 deletions(-)
diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
index 1b526a0..7699297 100644
--- a/libavcodec/hevc.c
+++ b/libavcodec/hevc.c
@@ -104,8 +104,7 @@ static int pic_arrays_init(HEVCContext *s, const HEVCSPS *sps)
s->cbf_luma = av_malloc_array(sps->min_tb_width, sps->min_tb_height);
s->tab_ipm = av_mallocz(min_pu_size);
-s->is_pcm = av_mallocz_array(sps->min_pu_width + 1, sps->min_pu_height + 1);
-
+s->is_pcm = av_malloc_array(sps->min_pu_width + 1, sps->min_pu_height + 1);
if (!s->tab_ipm || !s->cbf_luma || !s->is_pcm)
goto fail;
@@ -281,24 +280,6 @@ static int decode_lt_rps(HEVCContext *s, LongTermRPS *rps, GetBitContext *gb)
return 0;
}
-static int get_buffer_sao(HEVCContext *s, AVFrame *frame, const HEVCSPS *sps)
-{
-int ret, i;
-
-frame->width = FFALIGN(s->avctx->coded_width + 2, FF_INPUT_BUFFER_PADDING_SIZE);
-frame->height = s->avctx->coded_height + 3;
-if ((ret = ff_get_buffer(s->avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0)
-return ret;
-for (i = 0; frame->data[i]; i++) {
-int offset = frame->linesize[i] + FF_INPUT_BUFFER_PADDING_SIZE;
-frame->data[i] += offset;
-}
-frame->width = s->avctx->coded_width;
-frame->height = s->avctx->coded_height;
-
-return 0;
-}
-
static int set_sps(HEVCContext *s, const HEVCSPS *sps)
{
#define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL)
@@ -354,34 +335,19 @@ static int set_sps(HEVCContext *s, const HEVCSPS *sps)
ff_videodsp_init (&s->vdsp,sps->bit_depth);
if (sps->sao_enabled && !s->avctx->hwaccel) {
-#ifdef USE_SAO_SMALL_BUFFER
-{
-int ctb_size = 1 << sps->log2_ctb_size;
-int c_count = (sps->chroma_format_idc != 0) ? 3 : 1;
-int c_idx, i;
-
-for (i = 0; i < s->threads_number ; i++) {
-HEVCLocalContext*lc = s->HEVClcList[i];
-lc->sao_pixel_buffer =
-av_malloc(((ctb_size + 2) * (ctb_size + 2)) <<
- sps->pixel_shift);
-}
-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] =
+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] =
+s->sao_pixel_buffer_v[c_idx] =
av_malloc((h * 2 * sps->ctb_width) <<
sps->pixel_shift);
-}
}
-#else
-av_frame_unref(s->tmp_frame);
-
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
On Mon, Feb 02, 2015 at 02:22:36PM +0100, Christophe Gisquet wrote: > Hi, > > 2015-02-02 13:32 GMT+01:00 Michael Niedermayer : > > On Mon, Feb 02, 2015 at 07:41:54AM +0100, Christophe Gisquet wrote: > > hmm, is there a reason not to take the original commit unchanged ? > > I was hoping to reduce the difference to openhevc so that we also > > are able to merge future changes from openhevc with few confilcts > > but maybe iam missing something > > Because there are alignment requirements in our dsp, and technically, > adding another buffer (which isn't aligned) while there are already > perfectly good ones is not the best solution (memory-wise and > bookkeeping-wise). > > I'd go as far as suggest openhevc to align to this version. But maybe > they have diverged too much. iam happy with either openhevc picking this version or us picking the one from openhevc with minimal changes needed to make it work for us.. Though in the first case we should pick the openhevc version and comit our changes in a seperate commit on top so openhevc can more easily pick the changes we made if they want to. either way i hope very much that openhevcs and our implementation could stay similar as divergence would mean duplicated maintaince effort and both projects would have to do extra work in picking changes from the other [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
Hi,
2015-02-02 13:32 GMT+01:00 Michael Niedermayer :
> On Mon, Feb 02, 2015 at 07:41:54AM +0100, Christophe Gisquet wrote:
> hmm, is there a reason not to take the original commit unchanged ?
> I was hoping to reduce the difference to openhevc so that we also
> are able to merge future changes from openhevc with few confilcts
> but maybe iam missing something
Because there are alignment requirements in our dsp, and technically,
adding another buffer (which isn't aligned) while there are already
perfectly good ones is not the best solution (memory-wise and
bookkeeping-wise).
I'd go as far as suggest openhevc to align to this version. But maybe
they have diverged too much.
Also, I don't really like merge for the sake of merging in that case,
because those are matters difficult to follow, and I prefer something
that is more logical and makes sense.
> also it seems this does not apply cleanly
OK, I took a shortcut thinking my branch wouldn't cause such an issue.
Please try the attached patch.
--
Christophe
From c691861e730f941412749b764a15d66c26d9a216 Mon Sep 17 00:00:00 2001
From: Fabrice Bellard
Date: Mon, 12 Jan 2015 23:35:25 +0100
Subject: [PATCH] avcodec/hevc: reduce memory used by the SAO
SAO edge filter uses pre-SAO pixel data on the left and top of the ctb, so
this data must be kept available. This was done previously by having 2
copies of the frame, one before and one after SAO.
This patch reduces the storage to just that, instead of the previous whole
frame. A slight adaptation from Fabrice's version is to match our alignment
requirements, and abuse the edge emu buffers instead of adding a new
buffer.
Decicycles: 26772->26220 (BO32), 83803->80942 (BO64)
Signed-off-by: Christophe Gisquet
---
libavcodec/hevc.c| 43 +--
libavcodec/hevc.h| 5 +-
libavcodec/hevc_filter.c | 192 ++-
3 files changed, 177 insertions(+), 63 deletions(-)
diff --git a/libavcodec/hevc.c b/libavcodec/hevc.c
index f24cd8f..7699297 100644
--- a/libavcodec/hevc.c
+++ b/libavcodec/hevc.c
@@ -280,24 +280,6 @@ static int decode_lt_rps(HEVCContext *s, LongTermRPS *rps, GetBitContext *gb)
return 0;
}
-static int get_buffer_sao(HEVCContext *s, AVFrame *frame, const HEVCSPS *sps)
-{
-int ret, i;
-
-frame->width = FFALIGN(s->avctx->coded_width + 2, FF_INPUT_BUFFER_PADDING_SIZE);
-frame->height = s->avctx->coded_height + 3;
-if ((ret = ff_get_buffer(s->avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0)
-return ret;
-for (i = 0; frame->data[i]; i++) {
-int offset = frame->linesize[i] + FF_INPUT_BUFFER_PADDING_SIZE;
-frame->data[i] += offset;
-}
-frame->width = s->avctx->coded_width;
-frame->height = s->avctx->coded_height;
-
-return 0;
-}
-
static int set_sps(HEVCContext *s, const HEVCSPS *sps)
{
#define HWACCEL_MAX (CONFIG_HEVC_DXVA2_HWACCEL)
@@ -353,9 +335,19 @@ static int set_sps(HEVCContext *s, const HEVCSPS *sps)
ff_videodsp_init (&s->vdsp,sps->bit_depth);
if (sps->sao_enabled && !s->avctx->hwaccel) {
-av_frame_unref(s->tmp_frame);
-ret = get_buffer_sao(s, s->tmp_frame, sps);
-s->sao_frame = s->tmp_frame;
+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);
+}
}
s->sps = sps;
@@ -3175,7 +3167,10 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
av_freep(&s->cabac_state);
-av_frame_free(&s->tmp_frame);
+for (i = 0; i < 3; i++) {
+av_freep(&s->sao_pixel_buffer_h[i]);
+av_freep(&s->sao_pixel_buffer_v[i]);
+}
av_frame_free(&s->output_frame);
for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
@@ -3235,10 +3230,6 @@ static av_cold int hevc_init_context(AVCodecContext *avctx)
if (!s->cabac_state)
goto fail;
-s->tmp_frame = av_frame_alloc();
-if (!s->tmp_frame)
-goto fail;
-
s->output_frame = av_frame_alloc();
if (!s->output_frame)
goto fail;
diff --git a/libavcodec/hevc.h b/libavcodec/hevc.h
index 1727b60..ae9a32a 100644
--- a/libavcodec/hevc.h
+++ b/libavcodec/hevc.h
@@ -769,6 +769,7 @@ typedef struct HEVCLocalContext {
int end_of_tiles_y;
/* +7 is for subpixel interpolation, *2 for high bit depths */
DECLARE_ALIGNED(32, uint8_t, edge_emu_buffer)[(MAX_PB_SIZE + 7) * EDGE_EMU_BUFFER_STRIDE * 2];
+/* The extended size between the new edge emu buffer is abused by SAO */
DECLARE_ALIGNED(32, uint8_t, edge_emu_buffer2)[(MAX_PB
Re: [FFmpeg-devel] [PATCH] avcodec/hevc: reduce memory used by the SAO
On Mon, Feb 02, 2015 at 07:41:54AM +0100, Christophe Gisquet wrote: > Hi, > > the attached patch is somewhat of a hack job, as the commit I used may > already have been edited from its original version, and I have added > some stuff on top of it (eg the commit message). hmm, is there a reason not to take the original commit unchanged ? I was hoping to reduce the difference to openhevc so that we also are able to merge future changes from openhevc with few confilcts but maybe iam missing something also it seems this does not apply cleanly Applying: avcodec/hevc: reduce memory used by the SAO fatal: sha1 information is lacking or useless (libavcodec/hevc.c). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 avcodec/hevc: reduce memory used by the SAO When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
