| Message ID | 20200127060048.5229-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/asrc_sinc: Don't allocate arrays separately | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
LGTM On 1/27/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Besides the obvious advantages this also fixes a potential memleak: > If only one of the arrays had been successfully allocated, the other > would leak. This also fixes Coverity issues #1440386 and #1440387. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > During testing with -f lavfi -i sinc all I got is one output frame. Is > this intended or did I do something wrong? Consult documentation. > > libavfilter/asrc_sinc.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/libavfilter/asrc_sinc.c b/libavfilter/asrc_sinc.c > index 90f043d680..24aeab6e7b 100644 > --- a/libavfilter/asrc_sinc.c > +++ b/libavfilter/asrc_sinc.c > @@ -227,10 +227,11 @@ static int fir_to_phase(SincContext *s, float **h, int > *len, int *post_len, floa > > for (i = *len, work_len = 2 * 2 * 8; i > 1; work_len <<= 1, i >>= 1); > > - work = av_calloc(work_len + 2, sizeof(*work)); /* +2: (UN)PACK */ > - pi_wraps = av_calloc(((work_len + 2) / 2), sizeof(*pi_wraps)); > - if (!work || !pi_wraps) > + /* The first part is for work (+2 for (UN)PACK), the latter for > pi_wraps. */ > + work = av_calloc((work_len + 2) + (work_len / 2 + 1), sizeof(float)); > + if (!work) > return AVERROR(ENOMEM); > + pi_wraps = &work[work_len + 2]; > > memcpy(work, *h, *len * sizeof(*work)); > > @@ -240,7 +241,6 @@ static int fir_to_phase(SincContext *s, float **h, int > *len, int *post_len, floa > s->rdft = av_rdft_init(av_log2(work_len), DFT_R2C); > s->irdft = av_rdft_init(av_log2(work_len), IDFT_C2R); > if (!s->rdft || !s->irdft) { > - av_free(pi_wraps); > av_free(work); > return AVERROR(ENOMEM); > } > @@ -323,7 +323,6 @@ static int fir_to_phase(SincContext *s, float **h, int > *len, int *post_len, floa > *len = end - begin; > *h = av_realloc_f(*h, *len, sizeof(**h)); > if (!*h) { > - av_free(pi_wraps); > av_free(work); > return AVERROR(ENOMEM); > } > @@ -338,7 +337,6 @@ static int fir_to_phase(SincContext *s, float **h, int > *len, int *post_len, floa > work_len, pi_wraps[work_len >> 1] / M_PI, peak, peak_imp_sum, > imp_peak, > work[imp_peak], *len, *post_len, 100.f - 100.f * *post_len / > (*len - 1)); > > - av_free(pi_wraps); > av_free(work); > > return 0; > -- > 2.20.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Mon, Jan 27, 2020 at 09:36:51AM +0100, Paul B Mahol wrote:
> LGTM
will apply
[...]
diff --git a/libavfilter/asrc_sinc.c b/libavfilter/asrc_sinc.c index 90f043d680..24aeab6e7b 100644 --- a/libavfilter/asrc_sinc.c +++ b/libavfilter/asrc_sinc.c @@ -227,10 +227,11 @@ static int fir_to_phase(SincContext *s, float **h, int *len, int *post_len, floa for (i = *len, work_len = 2 * 2 * 8; i > 1; work_len <<= 1, i >>= 1); - work = av_calloc(work_len + 2, sizeof(*work)); /* +2: (UN)PACK */ - pi_wraps = av_calloc(((work_len + 2) / 2), sizeof(*pi_wraps)); - if (!work || !pi_wraps) + /* The first part is for work (+2 for (UN)PACK), the latter for pi_wraps. */ + work = av_calloc((work_len + 2) + (work_len / 2 + 1), sizeof(float)); + if (!work) return AVERROR(ENOMEM); + pi_wraps = &work[work_len + 2]; memcpy(work, *h, *len * sizeof(*work)); @@ -240,7 +241,6 @@ static int fir_to_phase(SincContext *s, float **h, int *len, int *post_len, floa s->rdft = av_rdft_init(av_log2(work_len), DFT_R2C); s->irdft = av_rdft_init(av_log2(work_len), IDFT_C2R); if (!s->rdft || !s->irdft) { - av_free(pi_wraps); av_free(work); return AVERROR(ENOMEM); } @@ -323,7 +323,6 @@ static int fir_to_phase(SincContext *s, float **h, int *len, int *post_len, floa *len = end - begin; *h = av_realloc_f(*h, *len, sizeof(**h)); if (!*h) { - av_free(pi_wraps); av_free(work); return AVERROR(ENOMEM); } @@ -338,7 +337,6 @@ static int fir_to_phase(SincContext *s, float **h, int *len, int *post_len, floa work_len, pi_wraps[work_len >> 1] / M_PI, peak, peak_imp_sum, imp_peak, work[imp_peak], *len, *post_len, 100.f - 100.f * *post_len / (*len - 1)); - av_free(pi_wraps); av_free(work); return 0;
Besides the obvious advantages this also fixes a potential memleak: If only one of the arrays had been successfully allocated, the other would leak. This also fixes Coverity issues #1440386 and #1440387. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- During testing with -f lavfi -i sinc all I got is one output frame. Is this intended or did I do something wrong? libavfilter/asrc_sinc.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)