[FFmpeg-devel] [PATCH 02/14] libavcodec: Implementation of AAC_fixed_decoder (LC-module) [2/5]

[Nedeljko Babic]

Thanks on your review,

>On Fri, Aug 01, 2014 at 03:53:08PM +0200, Nedeljko Babic wrote:
>> +#if !defined(_AAC_FLOAT_EMU_)
>> +#define _AAC_FLOAT_EMU_
>
>That is not out usual style
>1) We use #ifndef
>2) Identifiers starting with _ + upper case letter are reserved by POSIX
>and never should be used
>3) The naming convention is like e.g. AVCODEC_AAC_FLOAT_EMU_H
>

This file will be removed. 

Instead float_emu_tab.c and float_emu.h will be used.

>> +static const uint8_t ff_log2_tab[256] = {
>
>One with the same name exists in libavutil, I am not sure that is a good
>idea.
>

The one from the libavutil will be used instead of this one.

>> +static int divTable[128] = {
>
>These and the following ones seem to lack "const".
>

The versions from float_emu_tab.c will be used and they do have "const"

>> +    Q30(0.9999999995), Q30(0.9922480620), Q30(0.9846153846), Q30(0.9770992366),
>
>I'm a bit unsure btw. if this makes more sense than coding the converted
>numbers.
>It feels like it combines the disadvantages, since it neither avoids
>code duplication with the float tables nor does it compiler/architecture
>dependence completely (I think).
>

I am not sure what disadvantages are combined (there is no code duplication for example), 
but since only advantage is in that it can be seen what fixed point representation is used, 
the values will be hard coded in hex format.

>> +static av_always_inline av_const int av_log2_c_emu(unsigned int v)
>
>Since this duplicates existing code it definitely should have a comment.
>Also if performance is the reason there is a question whether we should
>at least internally have libavutil provide it instead, to at least avoid
>duplication.
>Though to be honest on most architectures the solution to it being slow
>should be to provide a ASM version.
>

The version from intmath.h will be used so no code duplication will occur.

>> +static av_always_inline aac_float_t int2float(const int x, const int exp)
>
>Those "const" are kind of pointless IMHO.
>

You are correct and they will be removed.

>> +    aac_float_t ret;
>> +    int nz;
>> +
>> +    if (x == 0)
>> +    {
>> +        ret.mant = 0;
>> +        ret.expo = 0;
>> +    }
>> +    else
>> +    {
>> +        ret.expo = exp;
>> +        ret.mant = x;
>> +        nz = 29 - av_log2_c_emu(FFABS(ret.mant));
>> +        ret.mant <<= nz;
>> +        ret.expo -= nz;
>
>nz is only used here, so it should be declared here.
>Also
>int nz = 29 - av_log2_c_emu(FFABS(x));

Since ISO C90 forbids mixing declarations and code, warnings will be generated if nz is declared here.
On the other hand, it can (and will) be declared at the start of the else block.

>ret.expo = exp - nz;
>ret.mant = x << nz;
>seems more readable to me.
>

Ok. It will be changed.

>> +static av_always_inline aac_float_t float_add(aac_float_t a, aac_float_t b)
>> +{
>> +    int diff, nz;
>> +    int expa = a.expo;
>> +    int expb = b.expo;
>> +    int manta = a.mant;
>> +    int mantb = b.mant;
>
>I don't think those local copies are a good idea, at least for
>readability.
>

Ok. This will be changed.

>> +    if (manta == 0)
>> +        return b;
>> +
>> +    if (mantb == 0)
>> +        return a;
>> +
>> +    diff = expa - expb;
>> +    if (diff < 0)  // expa < expb
>> +    {
>> +        diff = -diff;
>> +        if (diff >= 31)
>> +        manta = 0;
>> +        else if (diff != 0)
>> +        manta >>= diff;
>> +        expa = expb;
>> +    }
>> +    else  // expa >= expb
>> +    {
>> +        if (diff >= 31)
>> +        mantb = 0;
>> +        else if (diff != 0)
>> +        mantb >>= diff;
>> +    }
>> +
>> +    manta = manta + mantb;
>> +    if (manta == 0)
>> +        expa = 0;
>> +    else
>> +    {
>> +        nz = 30 - av_log2_c_emu(FFABS(manta));
>> +        manta <<= nz;
>> +        manta >>= 1;
>> +        expa -= (nz-1);
>> +    }
>
>I think this needs a good thinking over.
>If diff was calculated first, the >= 31/ <= -31 checks
>could be combined with the checks for 0.

diff is calculated right after checking if mantissas are zero.
Mantissas are checked first because if they are zero, 
it is a waste of time to do other calculations.

On the other hand, I am not sure what would be gain in combining
>= 31/ <= -31 checks with the checks for 0 (for diff).

>In the upper if, diff can obviously not be 0,

You are correct and != 0 check will be removed here.

>also the != 0 check can certainly only make performance worse.

I agree with this in if part, since it is not needed as you pointed out,
but this check is needed in the else part.

>For all functions, I think there should be a look at what rounding
>mode they end up using in the end and document that.
>This one looks like round to 0.
>

This is rounding to 0. It can be changed to rounding to nearest without loss in precision.

Do you suggest that we should make comment like:
    /* Rounding to zero used */
for every function?

>> +static av_always_inline aac_float_t float_sub(aac_float_t a, aac_float_t b)
>
>Why would this need a whole separate implementation instead
>of changing the sign of b and calling add?
>

Because there could be overhead because of function call if function is not inlined.
On the other hand I guess that gain is insignificant (or there is no gain if function 
is inlined) and this will be done by calling add as you suggested.

>> +static av_always_inline aac_float_t float_mul(aac_float_t a, aac_float_t b)
>> +{
>> +    aac_float_t res;
>> +    int mant;
>> +    int expa = a.expo;
>> +    int expb = b.expo;
>> +    long long accu;
>> +
>> +    expa = expa + expb;
>> +    accu = (long long)a.mant * b.mant;
>
>I'd suggest using types like int64_t.
>

Will be done.

>> +    mant = (int)((accu + 0x20000000) >> 30);
>
>This suddenly seems to use round-to-nearest-away as rounding mode/
>

Yes it is.

>> +    if (mant == 0)
>> +        expa = 0;
>> +    else if (mant < 536870912 && mant > -536870912)
>> +    {
>
>These constants would be vastly more readable in hex.
>

Will be converted.

>> +        mant <<= 1;
>> +        expa = expa - 1;
>
>expa--;
>

Will be changed.

>> +    if (mantb != 0)
>> +    {
>> +        iB = float_recip(b);
>> +        // newton iteration to double precision
>> +        tmp = float_sub(FLOAT_1, float_mul(b, iB));
>> +        iB = float_add(iB, float_mul(iB, tmp));
>> +        res = float_mul(a, iB);
>> +    }
>> +    else
>> +    {
>> +        res.mant = 1;
>> +        res.expo = 2147483647;
>
>Hexadecimal would be nicer.
>Also, this seems to be the only function so far that handles
>infinity correctly.
>I have some doubts of the value of that, as long as it is the only one.
>If there's a particular reason why it only needs to be supported here,
>that should be documented.
>

Only division by zero can create infinity, so it is addressed only here.

>> +static av_always_inline int float_gt(aac_float_t a, aac_float_t b)
>> +{
>> +    int expa = a.expo;
>> +    int expb = b.expo;
>> +    int manta = a.mant;
>> +    int mantb = b.mant;
>> +
>> +    if (manta == 0)
>> +        expa = 0x80000000;
>> +
>> +    if (mantb == 0)
>> +        expb = 0x80000000;
>> +
>> +    if (expa > expb)
>> +        return 1;
>> +    else if (expa < expb)
>> +        return 0;
>> +    else // expa == expb
>> +    {
>> +        if (manta > mantb)
>> +        return 1;
>> +        else
>> +        return 0;
>> +    }
>
>Does this actually work?
>The expa/expb assignment seems to invoke undefined behaviour by itself,
>but then it also seems to just not handle negative numbers, at least
>not if the exponent differs?!?

Functions in this file are made for use in the implementation of AAC fixed point decoder. 
This particular function is used only for calculating gain in aacsbr, so it doesn't need 
to handle negative numbers. 

On the other hand, I realize that this should be documented and will be in the next patch..

>I decided to rather skip reviewing the rest.
>
>> +++ b/libavcodec/float_emu.h
>
>That seems like the same file once more.
>
>> +++ b/libavcodec/float_emu_tab.c
>
>And these tables seem familiar, too.>

- Nedeljko