[FFmpeg-devel] [PATCH][RFC] avcodec/aacsbr_template: replace qsort with AV_QSORT

[Ganesh Ajjanagadde]

On Tue, Nov 3, 2015 at 3:24 PM, Rostislav Pehlivanov
<atomnuker at gmail.com> wrote:
> LGTM.
> Tested the patch, doesn't change the output.

I am personally fine with this, but was hoping for an opinion on
whether the perf increase here is important enough to justify the
binary size increase. Hence my ping to the AAC people here.

>
> On 28 October 2015 at 01:38, Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> wrote:
>
>> When sbr->reset is set in encode_frame, a bunch of qsort calls might get
>> made.
>> Thus, there is the potential of calling qsort whenever the spectral
>> contents change.
>>
>> AV_QSORT is substantially faster due to the inlining of the comparison
>> callback.
>> Thus, the increase in performance should be worth the increase in binary
>> size.
>>
>> Tested with FATE.
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>>
>>
>> --------------------------------------------------------------------------------
>> Please note that I am not not knowledgable about AAC, and above analysis
>> was based on a quick glance at the code. I could not get benchmarks with
>> runs > 1 easily (even with stream_loop), hence they are omitted.
>> Furthermore, it seems like AAC is under active work, so I defer
>> meaningful benchmarks to the maintainers.
>> ---
>>  libavcodec/aacsbr_template.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
>> index d31b71e..9561ba0 100644
>> --- a/libavcodec/aacsbr_template.c
>> +++ b/libavcodec/aacsbr_template.c
>> @@ -32,6 +32,8 @@
>>   * @author Zoran Basaric ( zoran.basaric at imgtec.com )
>>   */
>>
>> +#include "libavutil/qsort.h"
>> +
>>  av_cold void AAC_RENAME(ff_aac_sbr_init)(void)
>>  {
>>      static const struct {
>> @@ -138,8 +140,8 @@ static void
>> sbr_make_f_tablelim(SpectralBandReplication *sbr)
>>              memcpy(sbr->f_tablelim + sbr->n[0] + 1, patch_borders + 1,
>>                     (sbr->num_patches - 1) * sizeof(patch_borders[0]));
>>
>> -        qsort(sbr->f_tablelim, sbr->num_patches + sbr->n[0],
>> -              sizeof(sbr->f_tablelim[0]),
>> +        AV_QSORT(sbr->f_tablelim, sbr->num_patches + sbr->n[0],
>> +              uint16_t,
>>                qsort_comparison_function_int16);
>>
>>          sbr->n_lim = sbr->n[0] + sbr->num_patches - 1;
>> @@ -296,7 +298,7 @@ static int sbr_make_f_master(AACContext *ac,
>> SpectralBandReplication *sbr,
>>      if (spectrum->bs_stop_freq < 14) {
>>          sbr->k[2] = stop_min;
>>          make_bands(stop_dk, stop_min, 64, 13);
>> -        qsort(stop_dk, 13, sizeof(stop_dk[0]),
>> qsort_comparison_function_int16);
>> +        AV_QSORT(stop_dk, 13, int16_t, qsort_comparison_function_int16);
>>          for (k = 0; k < spectrum->bs_stop_freq; k++)
>>              sbr->k[2] += stop_dk[k];
>>      } else if (spectrum->bs_stop_freq == 14) {
>> @@ -389,7 +391,7 @@ static int sbr_make_f_master(AACContext *ac,
>> SpectralBandReplication *sbr,
>>
>>          make_bands(vk0+1, sbr->k[0], sbr->k[1], num_bands_0);
>>
>> -        qsort(vk0 + 1, num_bands_0, sizeof(vk0[1]),
>> qsort_comparison_function_int16);
>> +        AV_QSORT(vk0 + 1, num_bands_0, int16_t,
>> qsort_comparison_function_int16);
>>          vdk0_max = vk0[num_bands_0];
>>
>>          vk0[0] = sbr->k[0];
>> @@ -430,13 +432,13 @@ static int sbr_make_f_master(AACContext *ac,
>> SpectralBandReplication *sbr,
>>
>>              if (vdk1_min < vdk0_max) {
>>                  int change;
>> -                qsort(vk1 + 1, num_bands_1, sizeof(vk1[1]),
>> qsort_comparison_function_int16);
>> +                AV_QSORT(vk1 + 1, num_bands_1, int16_t,
>> qsort_comparison_function_int16);
>>                  change = FFMIN(vdk0_max - vk1[1], (vk1[num_bands_1] -
>> vk1[1]) >> 1);
>>                  vk1[1]           += change;
>>                  vk1[num_bands_1] -= change;
>>              }
>>
>> -            qsort(vk1 + 1, num_bands_1, sizeof(vk1[1]),
>> qsort_comparison_function_int16);
>> +            AV_QSORT(vk1 + 1, num_bands_1, int16_t,
>> qsort_comparison_function_int16);
>>
>>              vk1[0] = sbr->k[1];
>>              for (k = 1; k <= num_bands_1; k++) {
>> --
>> 2.6.2
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel