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

Reimar Döffinger Reimar.Doeffinger at gmx.de
Fri Aug 1 21:08:50 CEST 2014


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

> +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.

> +static int divTable[128] = {

These and the following ones seem to lack "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).

> +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.

> +static av_always_inline aac_float_t int2float(const int x, const int exp)

Those "const" are kind of pointless IMHO.

> +    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));
ret.expo = exp - nz;
ret.mant = x << nz;
seems more readable to me.

> +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.

> +    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.
In the upper if, diff can obviously not be 0,
also the != 0 check can certainly only make performance worse.
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.

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

> +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.

> +    mant = (int)((accu + 0x20000000) >> 30);

This suddenly seems to use round-to-nearest-away as rounding mode/

> +    if (mant == 0)
> +        expa = 0;
> +    else if (mant < 536870912 && mant > -536870912)
> +    {

These constants would be vastly more readable in hex.

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

expa--;

> +    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.

> +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?!?
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.


More information about the ffmpeg-devel mailing list