| Message ID | GV1P250MB07376C098B41808892A2E8CD8FEB2@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 42e0e05834ba156f91d84e940fbfc8e2756954fb |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avfilter/af_atempo: Properly check av_tx_init() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On Wed, May 22, 2024, 02:59 Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > The earlier code distinguished between a partial reset > (yae_clear()) and a complete reset (yae_release_buffers() > which also releases the buffers); this separation existed > to avoid allocations, as buffers were reallocated on reconfigs. > > Yet it is pointless since a5704659e3e41b7698812b89f67d9a7467a74d20, > so simply use yae_release_buffers() everywhere. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavfilter/af_atempo.c | 69 +++++++++++++++++++++-------------------- > 1 file changed, 35 insertions(+), 34 deletions(-) > > diff --git a/libavfilter/af_atempo.c b/libavfilter/af_atempo.c > index 1aedb82060..110f792eec 100644 > --- a/libavfilter/af_atempo.c > +++ b/libavfilter/af_atempo.c > @@ -244,18 +244,6 @@ static void yae_release_buffers(ATempoContext *atempo) > av_tx_uninit(&atempo->complex_to_real); > } > > -/* av_realloc is not aligned enough; fortunately, the data does not need > to > - * be preserved */ > -#define RE_MALLOC_OR_FAIL(field, field_size, element_size) \ > - do { \ > - av_freep(&field); \ > - field = av_calloc(field_size, element_size); \ > - if (!field) { \ > - yae_release_buffers(atempo); \ > - return AVERROR(ENOMEM); \ > - } \ > - } while (0) > - > /** > * Prepare filter for processing audio data of given format, > * sample rate and number of channels. > @@ -289,40 +277,51 @@ static int yae_reset(ATempoContext *atempo, > nlevels++; > } > > + /* av_realloc is not aligned enough, so simply discard all the old > buffers > + * (fortunately, their data does not need to be preserved) */ > + yae_release_buffers(atempo); > + > // initialize audio fragment buffers: > - RE_MALLOC_OR_FAIL(atempo->frag[0].data, atempo->window, > atempo->stride); > - RE_MALLOC_OR_FAIL(atempo->frag[1].data, atempo->window, > atempo->stride); > - RE_MALLOC_OR_FAIL(atempo->frag[0].xdat_in, (atempo->window + 1), > sizeof(AVComplexFloat)); > - RE_MALLOC_OR_FAIL(atempo->frag[1].xdat_in, (atempo->window + 1), > sizeof(AVComplexFloat)); > - RE_MALLOC_OR_FAIL(atempo->frag[0].xdat, (atempo->window + 1), > sizeof(AVComplexFloat)); > - RE_MALLOC_OR_FAIL(atempo->frag[1].xdat, (atempo->window + 1), > sizeof(AVComplexFloat)); > + if (!(atempo->frag[0].data = av_calloc(atempo->window, > atempo->stride)) || > + !(atempo->frag[1].data = av_calloc(atempo->window, > atempo->stride)) || > + !(atempo->frag[0].xdat_in = av_calloc(atempo->window + 1, > sizeof(AVComplexFloat))) || > + !(atempo->frag[1].xdat_in = av_calloc(atempo->window + 1, > sizeof(AVComplexFloat))) || > + !(atempo->frag[0].xdat = av_calloc(atempo->window + 1, > sizeof(AVComplexFloat))) || > + !(atempo->frag[1].xdat = av_calloc(atempo->window + 1, > sizeof(AVComplexFloat)))) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > > // initialize rDFT contexts: > - av_tx_uninit(&atempo->real_to_complex); > - av_tx_uninit(&atempo->complex_to_real); > - > ret = av_tx_init(&atempo->real_to_complex, &atempo->r2c_fn, > AV_TX_FLOAT_RDFT, 0, 1 << (nlevels + 1), &scale, 0); > - if (ret < 0) { > - yae_release_buffers(atempo); > - return ret; > - } > + if (ret < 0) > + goto fail; > > ret = av_tx_init(&atempo->complex_to_real, &atempo->c2r_fn, > AV_TX_FLOAT_RDFT, 1, 1 << (nlevels + 1), &iscale, 0); > - if (ret < 0) { > - yae_release_buffers(atempo); > - return ret; > - } > + if (ret < 0) > + goto fail; > > - RE_MALLOC_OR_FAIL(atempo->correlation_in, (atempo->window + 1), > sizeof(AVComplexFloat)); > - RE_MALLOC_OR_FAIL(atempo->correlation, atempo->window, > sizeof(AVComplexFloat)); > + if (!(atempo->correlation_in = av_calloc(atempo->window + 1, > sizeof(AVComplexFloat))) || > + !(atempo->correlation = av_calloc(atempo->window, > sizeof(AVComplexFloat)))) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > > atempo->ring = atempo->window * 3; > - RE_MALLOC_OR_FAIL(atempo->buffer, atempo->ring, atempo->stride); > + atempo->buffer = av_calloc(atempo->ring, atempo->stride); > + if (!atempo->buffer) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > > // initialize the Hann window function: > - RE_MALLOC_OR_FAIL(atempo->hann, atempo->window, sizeof(float)); > + atempo->hann = av_malloc_array(atempo->window, sizeof(float)); > + if (!atempo->hann) { > + ret = AVERROR(ENOMEM); > + goto fail; > + } > > for (i = 0; i < atempo->window; i++) { > double t = (double)i / (double)(atempo->window - 1); > @@ -330,8 +329,10 @@ static int yae_reset(ATempoContext *atempo, > atempo->hann[i] = (float)h; > } > > - yae_clear(atempo); > return 0; > +fail: > + yae_release_buffers(atempo); > + return ret; > } > > static int yae_update(AVFilterContext *ctx) > -- > 2.40.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". > This doesn't feel like a necessary change (how frequently is the affected code actually traversed, realistically?)... but since you've already spent the effort to make this change I have no objection. Pavel >
Pavel Koshevoy: > On Wed, May 22, 2024, 02:59 Andreas Rheinhardt < > andreas.rheinhardt@outlook.com> wrote: > >> The earlier code distinguished between a partial reset >> (yae_clear()) and a complete reset (yae_release_buffers() >> which also releases the buffers); this separation existed >> to avoid allocations, as buffers were reallocated on reconfigs. >> >> Yet it is pointless since a5704659e3e41b7698812b89f67d9a7467a74d20, >> so simply use yae_release_buffers() everywhere. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavfilter/af_atempo.c | 69 +++++++++++++++++++++-------------------- >> 1 file changed, 35 insertions(+), 34 deletions(-) >> >> diff --git a/libavfilter/af_atempo.c b/libavfilter/af_atempo.c >> index 1aedb82060..110f792eec 100644 >> --- a/libavfilter/af_atempo.c >> +++ b/libavfilter/af_atempo.c >> @@ -244,18 +244,6 @@ static void yae_release_buffers(ATempoContext *atempo) >> av_tx_uninit(&atempo->complex_to_real); >> } >> >> -/* av_realloc is not aligned enough; fortunately, the data does not need >> to >> - * be preserved */ >> -#define RE_MALLOC_OR_FAIL(field, field_size, element_size) \ >> - do { \ >> - av_freep(&field); \ >> - field = av_calloc(field_size, element_size); \ >> - if (!field) { \ >> - yae_release_buffers(atempo); \ >> - return AVERROR(ENOMEM); \ >> - } \ >> - } while (0) >> - >> /** >> * Prepare filter for processing audio data of given format, >> * sample rate and number of channels. >> @@ -289,40 +277,51 @@ static int yae_reset(ATempoContext *atempo, >> nlevels++; >> } >> >> + /* av_realloc is not aligned enough, so simply discard all the old >> buffers >> + * (fortunately, their data does not need to be preserved) */ >> + yae_release_buffers(atempo); >> + >> // initialize audio fragment buffers: >> - RE_MALLOC_OR_FAIL(atempo->frag[0].data, atempo->window, >> atempo->stride); >> - RE_MALLOC_OR_FAIL(atempo->frag[1].data, atempo->window, >> atempo->stride); >> - RE_MALLOC_OR_FAIL(atempo->frag[0].xdat_in, (atempo->window + 1), >> sizeof(AVComplexFloat)); >> - RE_MALLOC_OR_FAIL(atempo->frag[1].xdat_in, (atempo->window + 1), >> sizeof(AVComplexFloat)); >> - RE_MALLOC_OR_FAIL(atempo->frag[0].xdat, (atempo->window + 1), >> sizeof(AVComplexFloat)); >> - RE_MALLOC_OR_FAIL(atempo->frag[1].xdat, (atempo->window + 1), >> sizeof(AVComplexFloat)); >> + if (!(atempo->frag[0].data = av_calloc(atempo->window, >> atempo->stride)) || >> + !(atempo->frag[1].data = av_calloc(atempo->window, >> atempo->stride)) || >> + !(atempo->frag[0].xdat_in = av_calloc(atempo->window + 1, >> sizeof(AVComplexFloat))) || >> + !(atempo->frag[1].xdat_in = av_calloc(atempo->window + 1, >> sizeof(AVComplexFloat))) || >> + !(atempo->frag[0].xdat = av_calloc(atempo->window + 1, >> sizeof(AVComplexFloat))) || >> + !(atempo->frag[1].xdat = av_calloc(atempo->window + 1, >> sizeof(AVComplexFloat)))) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> >> // initialize rDFT contexts: >> - av_tx_uninit(&atempo->real_to_complex); >> - av_tx_uninit(&atempo->complex_to_real); >> - >> ret = av_tx_init(&atempo->real_to_complex, &atempo->r2c_fn, >> AV_TX_FLOAT_RDFT, 0, 1 << (nlevels + 1), &scale, 0); >> - if (ret < 0) { >> - yae_release_buffers(atempo); >> - return ret; >> - } >> + if (ret < 0) >> + goto fail; >> >> ret = av_tx_init(&atempo->complex_to_real, &atempo->c2r_fn, >> AV_TX_FLOAT_RDFT, 1, 1 << (nlevels + 1), &iscale, 0); >> - if (ret < 0) { >> - yae_release_buffers(atempo); >> - return ret; >> - } >> + if (ret < 0) >> + goto fail; >> >> - RE_MALLOC_OR_FAIL(atempo->correlation_in, (atempo->window + 1), >> sizeof(AVComplexFloat)); >> - RE_MALLOC_OR_FAIL(atempo->correlation, atempo->window, >> sizeof(AVComplexFloat)); >> + if (!(atempo->correlation_in = av_calloc(atempo->window + 1, >> sizeof(AVComplexFloat))) || >> + !(atempo->correlation = av_calloc(atempo->window, >> sizeof(AVComplexFloat)))) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> >> atempo->ring = atempo->window * 3; >> - RE_MALLOC_OR_FAIL(atempo->buffer, atempo->ring, atempo->stride); >> + atempo->buffer = av_calloc(atempo->ring, atempo->stride); >> + if (!atempo->buffer) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> >> // initialize the Hann window function: >> - RE_MALLOC_OR_FAIL(atempo->hann, atempo->window, sizeof(float)); >> + atempo->hann = av_malloc_array(atempo->window, sizeof(float)); >> + if (!atempo->hann) { >> + ret = AVERROR(ENOMEM); >> + goto fail; >> + } >> >> for (i = 0; i < atempo->window; i++) { >> double t = (double)i / (double)(atempo->window - 1); >> @@ -330,8 +329,10 @@ static int yae_reset(ATempoContext *atempo, >> atempo->hann[i] = (float)h; >> } >> >> - yae_clear(atempo); >> return 0; >> +fail: >> + yae_release_buffers(atempo); >> + return ret; >> } >> >> static int yae_update(AVFilterContext *ctx) >> -- >> 2.40.1 >> > > > This doesn't feel like a necessary change (how frequently is the affected > code actually traversed, realistically?)... but since you've already spent > the effort to make this change I have no objection. 1. Looking at ff_filter_config_links(), it seems like the answer is at most once; yet IIRC it is intended for filtergraph reconfigurations to become a thing one day and given that the old code already supported it, I kept supporting it. 2. I disagree with your point that code that is executed only infrequently need not be optimized. Optimizations are good if they enhance clarity or reduce codesize; IMO this patch does both. - Andreas
diff --git a/libavfilter/af_atempo.c b/libavfilter/af_atempo.c index 1aedb82060..110f792eec 100644 --- a/libavfilter/af_atempo.c +++ b/libavfilter/af_atempo.c @@ -244,18 +244,6 @@ static void yae_release_buffers(ATempoContext *atempo) av_tx_uninit(&atempo->complex_to_real); } -/* av_realloc is not aligned enough; fortunately, the data does not need to - * be preserved */ -#define RE_MALLOC_OR_FAIL(field, field_size, element_size) \ - do { \ - av_freep(&field); \ - field = av_calloc(field_size, element_size); \ - if (!field) { \ - yae_release_buffers(atempo); \ - return AVERROR(ENOMEM); \ - } \ - } while (0) - /** * Prepare filter for processing audio data of given format, * sample rate and number of channels. @@ -289,40 +277,51 @@ static int yae_reset(ATempoContext *atempo, nlevels++; } + /* av_realloc is not aligned enough, so simply discard all the old buffers + * (fortunately, their data does not need to be preserved) */ + yae_release_buffers(atempo); + // initialize audio fragment buffers: - RE_MALLOC_OR_FAIL(atempo->frag[0].data, atempo->window, atempo->stride); - RE_MALLOC_OR_FAIL(atempo->frag[1].data, atempo->window, atempo->stride); - RE_MALLOC_OR_FAIL(atempo->frag[0].xdat_in, (atempo->window + 1), sizeof(AVComplexFloat)); - RE_MALLOC_OR_FAIL(atempo->frag[1].xdat_in, (atempo->window + 1), sizeof(AVComplexFloat)); - RE_MALLOC_OR_FAIL(atempo->frag[0].xdat, (atempo->window + 1), sizeof(AVComplexFloat)); - RE_MALLOC_OR_FAIL(atempo->frag[1].xdat, (atempo->window + 1), sizeof(AVComplexFloat)); + if (!(atempo->frag[0].data = av_calloc(atempo->window, atempo->stride)) || + !(atempo->frag[1].data = av_calloc(atempo->window, atempo->stride)) || + !(atempo->frag[0].xdat_in = av_calloc(atempo->window + 1, sizeof(AVComplexFloat))) || + !(atempo->frag[1].xdat_in = av_calloc(atempo->window + 1, sizeof(AVComplexFloat))) || + !(atempo->frag[0].xdat = av_calloc(atempo->window + 1, sizeof(AVComplexFloat))) || + !(atempo->frag[1].xdat = av_calloc(atempo->window + 1, sizeof(AVComplexFloat)))) { + ret = AVERROR(ENOMEM); + goto fail; + } // initialize rDFT contexts: - av_tx_uninit(&atempo->real_to_complex); - av_tx_uninit(&atempo->complex_to_real); - ret = av_tx_init(&atempo->real_to_complex, &atempo->r2c_fn, AV_TX_FLOAT_RDFT, 0, 1 << (nlevels + 1), &scale, 0); - if (ret < 0) { - yae_release_buffers(atempo); - return ret; - } + if (ret < 0) + goto fail; ret = av_tx_init(&atempo->complex_to_real, &atempo->c2r_fn, AV_TX_FLOAT_RDFT, 1, 1 << (nlevels + 1), &iscale, 0); - if (ret < 0) { - yae_release_buffers(atempo); - return ret; - } + if (ret < 0) + goto fail; - RE_MALLOC_OR_FAIL(atempo->correlation_in, (atempo->window + 1), sizeof(AVComplexFloat)); - RE_MALLOC_OR_FAIL(atempo->correlation, atempo->window, sizeof(AVComplexFloat)); + if (!(atempo->correlation_in = av_calloc(atempo->window + 1, sizeof(AVComplexFloat))) || + !(atempo->correlation = av_calloc(atempo->window, sizeof(AVComplexFloat)))) { + ret = AVERROR(ENOMEM); + goto fail; + } atempo->ring = atempo->window * 3; - RE_MALLOC_OR_FAIL(atempo->buffer, atempo->ring, atempo->stride); + atempo->buffer = av_calloc(atempo->ring, atempo->stride); + if (!atempo->buffer) { + ret = AVERROR(ENOMEM); + goto fail; + } // initialize the Hann window function: - RE_MALLOC_OR_FAIL(atempo->hann, atempo->window, sizeof(float)); + atempo->hann = av_malloc_array(atempo->window, sizeof(float)); + if (!atempo->hann) { + ret = AVERROR(ENOMEM); + goto fail; + } for (i = 0; i < atempo->window; i++) { double t = (double)i / (double)(atempo->window - 1); @@ -330,8 +329,10 @@ static int yae_reset(ATempoContext *atempo, atempo->hann[i] = (float)h; } - yae_clear(atempo); return 0; +fail: + yae_release_buffers(atempo); + return ret; } static int yae_update(AVFilterContext *ctx)
The earlier code distinguished between a partial reset (yae_clear()) and a complete reset (yae_release_buffers() which also releases the buffers); this separation existed to avoid allocations, as buffers were reallocated on reconfigs. Yet it is pointless since a5704659e3e41b7698812b89f67d9a7467a74d20, so simply use yae_release_buffers() everywhere. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavfilter/af_atempo.c | 69 +++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 34 deletions(-)