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

[Vitor Sessak]

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]?

>>> +    for (k = 0; k < 32; k++) {
>>> +        float post = M_PI * (k + 0.5) / 128;
>>> +        analysis_cossin_post[k][0] =  2.0 * cos(post);
>>> +        analysis_cossin_post[k][1] = -2.0 * sin(post);
>>> +    }
>> Also here if possible
>>
>>> +static void sbr_extension(SpectralBandReplication *sbr, GetBitContext
>>> *gb,
>>> +                          int bs_extension_id, int *num_bits_left)
>>> +{
>>> +/* TODO - implement ps_data for parametric stereo parsing
>>> +    switch (bs_extension_id) {
>>> +    case EXTENSION_ID_PS:
>>> +        num_bits_left -= ps_data(sbr, gb);
>>> +        break;
>>> +    default:
>>> +*/
>>> +        skip_bits(gb, *num_bits_left); // bs_fill_bits
>>> +        *num_bits_left = 0;
>>> +/*
>>> +        break;
>>> +    }
>>> +*/
>>> +}
>> Isn't it missing a call to av_log_missing_feature()?
> 
> People complained constantly about av_log_missing_feature() spamming
> the console on finding SBR.
> 
>>> +        if (div) {
>>> +            memset(X[0][l]+32, 0, 32*sizeof(float));
>>> +            memset(X[1][l]+32, 0, 32*sizeof(float));
>>> +        }
>>> +        ff_imdct_half(mdct, mdct_buf[0], X[0][l]);
>>> +        ff_imdct_half(mdct, mdct_buf[1], X[1][l]);
>>> +        for (n = 0; n < 64 >> div; n++) {
>>> +            v[               n] = -mdct_buf[0][64-1-(n<<div)    ] +
>>> mdct_buf[1][ n<<div     ];
>>> +            v[(128>>div)-1 - n] =  mdct_buf[0][64-1-(n<<div)-div] +
>>> mdct_buf[1][(n<<div)+div];
>>> +        }
>> I still think you can avoid the temporary scratch buffer...
> 
> I will fix
> 
>>> +        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().

>>> --- /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?

-Vitor