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

Rostislav Pehlivanov atomnuker at gmail.com
Wed Nov 4 01:56:48 CET 2015


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.

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
>


More information about the ffmpeg-devel mailing list