Discussion:
[PATCH] Fixed: large memory-leaks when the encoder is closed.
(too old to reply)
Boris Nagels
2018-01-02 22:31:08 UTC
Permalink
Calling x264_encoder_close() will also call x264_analyse_free_costs() to free up the allocated resources. The current implementation checks the wrong pointers if they're valid. This is because the pointers that are stored in the x264_t structure are shifted (see implementation for x264_analyse_init_costs()). This patch also stores the original pointers that were allocated, to be able to free the memory when the codec is closed. In the original implementation, this leads to memory-leaks and/or memory corruption when the encoder is opened/closed and reopened.
---
common/common.h | 2 ++
encoder/analyse.c | 21 ++++++++++++---------
2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/common/common.h b/common/common.h
index 867b2073..b292fcdc 100644
--- a/common/common.h
+++ b/common/common.h
@@ -563,6 +563,8 @@ struct x264_t
/* mv/ref cost arrays. */
uint16_t *cost_mv[QP_MAX+1];
uint16_t *cost_mv_fpel[QP_MAX+1][4];
+ uint16_t *p_cost_mv[QP_MAX+1];
+ uint16_t *p_cost_mv_fpel[QP_MAX+1][4];

const uint8_t *chroma_qp_table; /* includes both the nonlinear luma->chroma mapping and chroma_qp_offset */

diff --git a/encoder/analyse.c b/encoder/analyse.c
index 036d6c15..07668a63 100644
--- a/encoder/analyse.c
+++ b/encoder/analyse.c
@@ -270,8 +270,8 @@ static int init_costs( x264_t *h, float *logs, int qp )
int mv_range = h->param.analyse.i_mv_range;
int lambda = x264_lambda_tab[qp];
/* factor of 4 from qpel, 2 from sign, and 2 because mv can be opposite from mvp */
- CHECKED_MALLOC( h->cost_mv[qp], (4*4*mv_range + 1) * sizeof(uint16_t) );
- h->cost_mv[qp] += 2*4*mv_range;
+ CHECKED_MALLOC( h->p_cost_mv[qp], (4*4*mv_range + 1) * sizeof(uint16_t) );
+ h->cost_mv[qp] = h->p_cost_mv[qp] + 2*4*mv_range;
for( int i = 0; i <= 2*4*mv_range; i++ )
{
h->cost_mv[qp][-i] =
@@ -286,8 +286,8 @@ static int init_costs( x264_t *h, float *logs, int qp )
{
for( int j = 0; j < 4; j++ )
{
- CHECKED_MALLOC( h->cost_mv_fpel[qp][j], (4*mv_range + 1) * sizeof(uint16_t) );
- h->cost_mv_fpel[qp][j] += 2*mv_range;
+ CHECKED_MALLOC( h->p_cost_mv_fpel[qp][j], (4*mv_range + 1) * sizeof(uint16_t) );
+ h->cost_mv_fpel[qp][j] = h->p_cost_mv_fpel[qp][j] + 2*mv_range;
for( int i = -2*mv_range; i < 2*mv_range; i++ )
h->cost_mv_fpel[qp][j][i] = h->cost_mv[qp][i*4+j];
}
@@ -330,11 +330,14 @@ void x264_analyse_free_costs( x264_t *h )
int mv_range = h->param.analyse.i_mv_range;
for( int i = 0; i < QP_MAX+1; i++ )
{
- if( h->cost_mv[i] )
- x264_free( h->cost_mv[i] - 2*4*mv_range );
- if( h->cost_mv_fpel[i][0] )
- for( int j = 0; j < 4; j++ )
- x264_free( h->cost_mv_fpel[i][j] - 2*mv_range );
+ if (h->p_cost_mv[i]) {
+ x264_free(h->p_cost_mv[i]);
+ }
+ if(h->p_cost_mv_fpel[i][0]) {
+ for( int j = 0; j < 4; j++ ) {
+ x264_free(h->p_cost_mv_fpel[i][j]);
+ }
+ }
}
}
--
2.14.3 (Apple Git-98)
Henrik Gramner
2018-01-02 23:26:23 UTC
Permalink
Post by Boris Nagels
Calling x264_encoder_close() will also call x264_analyse_free_costs() to free up the allocated resources. The current implementation checks the wrong pointers if they're valid. This is because the pointers that are stored in the x264_t structure are shifted (see implementation for x264_analyse_init_costs()). This patch also stores the original pointers that were allocated, to be able to free the memory when the codec is closed. In the original implementation, this leads to memory-leaks and/or memory corruption when the encoder is opened/closed and reopened.
The pointers should only be wrong if h->param.analyse.i_mv_range
changes between encoder_open() and encoder_close() and I don't believe
mv-range can be reconfigured after the encoder has been opened.

valgrind does not indicate any errors or memory leaks for me.
Boris Nagels
2018-01-03 22:22:56 UTC
Permalink
Some more info: I didn’t use the x264 library directly, instead the calls are made from the within the ffmpeg library that I’m using (x264 encoder part).
Probably the leak occurs because of a combination of the 2 libraries. I’ll look in more detail at the series of calls when the memory leaks occur.

The leaks were reported by the “leak instrument” that is part of Xcode.
Post by Henrik Gramner
Post by Boris Nagels
Calling x264_encoder_close() will also call x264_analyse_free_costs() to free up the allocated resources. The current implementation checks the wrong pointers if they're valid. This is because the pointers that are stored in the x264_t structure are shifted (see implementation for x264_analyse_init_costs()). This patch also stores the original pointers that were allocated, to be able to free the memory when the codec is closed. In the original implementation, this leads to memory-leaks and/or memory corruption when the encoder is opened/closed and reopened.
The pointers should only be wrong if h->param.analyse.i_mv_range
changes between encoder_open() and encoder_close() and I don't believe
mv-range can be reconfigured after the encoder has been opened.
valgrind does not indicate any errors or memory leaks for me.
_______________________________________________
x264-devel mailing list
https://mailman.videolan.org/listinfo/x264-devel
Henrik Gramner
2018-01-04 19:16:11 UTC
Permalink
Post by Boris Nagels
Some more info: I didn’t use the x264 library directly, instead the calls are made from the within the ffmpeg library that I’m using (x264 encoder part).
Probably the leak occurs because of a combination of the 2 libraries. I’ll look in more detail at the series of calls when the memory leaks occur.
The leaks were reported by the “leak instrument” that is part of Xcode.
I tested valgrind with libx264 through ffmpeg CLI (latest git master
of both ffmpeg and x264) and it does not detect any leaks or errors.
Boris Nagels
2018-01-05 09:08:52 UTC
Permalink
The problem occurs when I use at least two encoders, opened at the same time, from 2 separate threads.
Each thread is responsible for its own x264 encoder (opening and closing).
Post by Henrik Gramner
Post by Boris Nagels
Some more info: I didn’t use the x264 library directly, instead the calls are made from the within the ffmpeg library that I’m using (x264 encoder part).
Probably the leak occurs because of a combination of the 2 libraries. I’ll look in more detail at the series of calls when the memory leaks occur.
The leaks were reported by the “leak instrument” that is part of Xcode.
I tested valgrind with libx264 through ffmpeg CLI (latest git master
of both ffmpeg and x264) and it does not detect any leaks or errors.
_______________________________________________
x264-devel mailing list
https://mailman.videolan.org/listinfo/x264-devel
BugMaster
2018-01-05 09:49:50 UTC
Permalink
Post by Boris Nagels
The problem occurs when I use at least two encoders, opened at the
same time, from 2 separate threads.
Each thread is responsible for its own x264 encoder (opening and closing).
Hi.

Are you using the latest version of source code (i.e. b00bcafe53a166b63a179a2f41470cd13b59f927)?
Are you sure that your leak is in x264_encoder_close and not due failed x264_encoder_open
which can leak during fail.
Also you can try with attached patch but it fix not leak but
theoretical but not possible situation when h->cost_mv_fpel[i][0] was
allocated but one of h->cost_mv_fpel[i][1..3] allocation failed. This
situation is impossible because in this case we wouldn't call
x264_analyse_free_costs (at least currently).

Loading...