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

[Ganesh Ajjanagadde]

On Tue, Nov 3, 2015 at 7:56 PM, Rostislav Pehlivanov
<atomnuker at gmail.com> wrote:
> The binary increase is 16 kbytes per file * 2 (since it's a template for
> the fixed and float decoders) = 32 kbytes. This is not very significant at
> all, even for the most memory and storage-space constrained (with an MMU)
> device. (not to mention that this is using the default compiler
> optimizations, GCC is probably smart enough not to inline the functions if
> optimization for size has been enabled).
> Performance wise this is probably worth it since this is an init function
> and you absolutely want to spend the least amount of time doing this.
> I'd normally be against not using standard libc functions but since qsort
> there is probably as optimized as it can be without inlining and we can
> beat it speed wise, it's nice and I'm for it.

Ok, pushed. Thanks.

>
> On 3 November 2015 at 21:13, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>
>> 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
>> _______________________________________________
>> 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