[FFmpeg-devel] [PATCH]HE-AACv1 try 3 (all missing functionality added)

Alex Converse alex.converse
Mon Feb 15 20:20:19 CET 2010


On Mon, Feb 15, 2010 at 1:52 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> Alex Converse wrote:
>>
>> On Sat, Feb 13, 2010 at 11:31 AM, Vitor Sessak <vitor1001 at gmail.com>
>> wrote:
>>>
>>> Alex Converse wrote:
>>>>
>>>> Notes:
>>>> *A filterbank that supports float_to_int16_c as been added
>>>> *All the computation time is spent in ff_sbr_apply() and it's
>>>> children. If it isn't called from ff_sbr_apply() making it 100% faster
>>>> isn't going to buy us anything.
>>>> *No calls to lrintf depend on rounding behavior at (2*n+1)*0.5
>>>> *Some sample SIMD placeholders are attached as a second patch.
>>>> *Right now the synthesis filterbank is written on top on an MDCT. With
>>>> appropriate SIMD functions it may make sense to move it to an FFT.
>>>> Right now the MDCT version is much faster.
>>>> *The analysis filterbank has been switched from an FFT to an RDFT.
>>>> + ? ?for (n = 0; n < 64; n++) {
>>>> + ? ? ? ?float pre = M_PI * n / 64;
>>>> + ? ? ? ?analysis_cos_pre[n] = cos(pre);
>>>> + ? ? ? ?analysis_sin_pre[n] = sin(pre);
>>>
>>> Please do not reinvent ff_cos_tabs[]. There is no point polluting the
>>> cache
>>> with several different copies of the same tables.
>>
>> Why is ff_cos_tabs positive in quadrant II?
>
> Are you using init_ff_cos_tabs(7); and ff_cos_tabs[7]?
>

yes.

  64 av_cold void ff_init_ff_cos_tabs(int index)
  65 {
  66 #if !CONFIG_HARDCODED_TABLES
  67     int i;
  68     int m = 1<<index;
  69     double freq = 2*M_PI/m;
  70     FFTSample *tab = ff_cos_tabs[index];
Quadrant I goes from theta (=i*freq) = 0 to pi/2
cos(theta) is positive
the table is positive
  71     for(i=0; i<=m/4; i++)
  72         tab[i] = cos(i*freq);
Quadrant II goes from theta (=i*freq) = pi/2 to pi
cos(theta) is negative
the table is positive
  73     for(i=1; i<m/4; i++)
  74         tab[m/2-i] = tab[i];
  75 #endif
  76 }

[...]
>>>> + ? ? ? ?for (n = 0; n < 64 >> div; n++)
>>>> + ? ? ? ? ? ?out[n] = out[n] * scale + bias;
>>>
>>> I suppose scale == 1 and bias == 0 is a common case, no? If yes, I'd put
>>> it
>>> inside an if(scale != 1 || bias != 0)
>>>
>>
>> If I do that then someone is going to whine all the favorites about
>> floating point equality tests.
>
> It is not really pretty, but doing useless loops for the great majority of
> supported platforms is neither.
>
>> * that on their favorite architecture that is not 100% 754 complaint
>> the result isn't quite equal
>
> It will work in 95% of the platforms and just skip an optimization in the
> remaining 5%. There is no way it could lead to wrong output.
>
>> * gcc spitting out really bad code for what should be a trivial check
>
> One could evaluate it on init().

OK

>
>>>> --- /dev/null
>>>> +++ b/libavcodec/aacsbr.h
>>>> @@ -0,0 +1,186 @@
>
> [...]
>
>>>> +++ b/libavcodec/aacsbr_internal.h
>>>> @@ -0,0 +1,51 @@
>>>> +/*
>>>> + * AAC Spectral Band Replication function declarations
>>>> + * Copyright (c) 2008-2009 Robert Swain ( rob opendot cl )
>>>> + * Copyright (c) 2010 ? ? ?Alex Converse <alex.converse at gmail.com>
>>>> + *
>>>> + * This file is part of FFmpeg.
>>>> + *
>>>> + * FFmpeg is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>> + *
>>>> + * FFmpeg is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with FFmpeg; if not, write to the Free Software
>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>>> 02110-1301 USA
>>>> + */
>>>> +
>>>> +/**
>>>> + * @file libavcodec/aacsbr_internal.h
>>>> + * AAC Spectral Band Replication function declarations
>>>> + * @author Robert Swain ( rob opendot cl )
>>>> + */
>>>> +
>>>> +#ifndef AVCODEC_AACSBR_INTERNAL_H
>>>> +#define AVCODEC_AACSBR_INTERNAL_H
>>>> +
>>>> +#include "get_bits.h"
>>>> +#include "aac.h"
>>>> +#include "aacsbr.h"
>>>> +
>>>> +/** Initialize SBR. */
>>>> +av_cold void ff_aac_sbr_init(void);
>>>> +/** Initialize one SBR context. */
>>>> +av_cold void ff_aac_sbr_ctx_init(SpectralBandReplication *sbr);
>>>> +/** Close one SBR context. */
>>>> +av_cold void ff_aac_sbr_ctx_close(SpectralBandReplication *sbr);
>>>> +/** Decode one SBR element. */
>>>> +int ff_decode_sbr_extension(AACContext *ac, SpectralBandReplication
>>>> *sbr,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?GetBitContext *gb, int crc, int cnt, int
>>>> id_aac);
>>>> +/** Dequantized all channels in one SBR element. */
>>>> +void ff_sbr_dequant(AACContext *ac, SpectralBandReplication *sbr, int
>>>> id_aac);
>>>> +/** Apply dequantized SBR to a single AAC channel. */
>>>> +void ff_sbr_apply(AACContext *ac, SpectralBandReplication *sbr, int ch,
>>>> + ? ? ? ? ? ? ? ? ?const float* in, float* out);
>>>> +
>>>> +#endif /* AVCODEC_AACSBR_INTERNAL_H */
>>>
>>> I don't see how this header is more internal than aacsbr.h...
>>
>> Can you propose better names?
>
> My question is why do you need two headers (aacsbr.h and aacsbr_internal.h)?
> How do you decide what goes to one and what goes to the other?
>

aac.h declares AACContext. AACContext has an SBRContext contained inside it.
aacsbr.h declares the SBRContext.
aacsbr_internal.h has function declarations from the SBR decoder for
use by the AAC decoder. These functions require both the AACContext
and the SBRContext as arguments.

Other options are:
*Dump the aacsbr_internal.h functions into aac.h but I'd like to keep
SBR as separate as possible from AAC. Demangling the two further is
probably required for MPEG Surround, BSAC+SBR, and USAC
*Forward declare AACContext in aacsbr.h



More information about the ffmpeg-devel mailing list