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

[Vitor Sessak]

Alex Converse wrote:
> 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?

Good question. I think I use only the first quadrant. Any one of the FFT 
authors could clarify it?

[...]


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

Ok, so aacsbr.h is for general SBR code, not necessarily AAC specific 
and aacsbr_internal.h is AAC specific. Since you say it will be 
necessary to demangle the two further to reuse for other SBR code, I 
think just splitting the headers again will be the least of the trouble. 
So what I suggest is:

1- By now merge aacsbr.h and aacsbr_internal.h

2- When this code is used in another codec, do something like
    sbr.{c,h} - declaration for functions and structures used by all SBR 
codecs. do not include "aac.h"
    aacsbr.{c,h} - do a "#include aac.h" and have AAC specific code

-Vitor