Re: [FFmpeg-devel] [PATCH 2/5] avfilter/af_atempo: Simplify resetting

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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

_______________________________________________
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".