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

Nedeljko Babic Nedeljko.Babic at imgtec.com
Fri Aug 8 16:36:07 CEST 2014


I agree with most of your suggestions and they will be incorporated in the next patch.

Thanks again on your review.

- Nedeljko
________________________________________
Od: ffmpeg-devel-bounces at ffmpeg.org [ffmpeg-devel-bounces at ffmpeg.org] u ime korisnika Reimar Döffinger [Reimar.Doeffinger at gmx.de]
Poslato: 7. avgust 2014 22:12
Za: FFmpeg development discussions and patches
Tema: Re: [FFmpeg-devel] [PATCH 02/14] libavcodec: Implementation of AAC_fixed_decoder (LC-module) [2/5]

On Thu, Aug 07, 2014 at 09:51:36AM +0000, Nedeljko Babic wrote:
> >> +    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),

I assumed the same tables might already exist somewhere, but I might
just misunderstand.

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

To be honest, I flagged this more as a point of discussion.
In some ways, this variant is more "readable/verifiable", but
on the other hand bit-exact decoding between compilers/platforms
would be even nicer. Yet again, it might be a purely theoretical issue.


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

Yes, I meant at the start of the else block by "here", though with my proposed change that will
also be the first use :)

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

As far as I can tell, if the difference is <= 31 we will always
return either a or b, i.e. whether one is at least 31 exponents
smaller or it is 0 is the same thing.
Assuming I understood the code right, I guess you can also just replace
> >> +        if (diff >= 31)
> >> +        manta = 0;

by

> >> +        if (diff >= 31)
> >> +        return b;

Though the question is whether it's more important performance wise
to optimize the case where one operand is 0 or to reduce the number of
branches. Or it might not matter at all.
But I admit I haven't looked at it closely enough to be 100% sure about
any of this, thus the statement that I believe someone needs to think
through this code once more, because I strongly suspect it's needlessly
confusing and slow.

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

If the statement is
if (s != 0) a >>= s;
how can the if _ever_ be _needed_ for correctness?
a >>= 0 is a NOP, so no need to explicitly skip it for s == 0.

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

Well, if every function used round to zero that, it should be written once e.g.
at the top of the file.
However that does not seem to be the case.
Ideally, I think there should be a statement that all functions unless
otherwise stated use round to 0 for simplicity.
In addition, for those using a different rounding this should be
documented.

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

The functions are av_always_inline, so I think we may assume that they
will be inlined and when they are not tell people to live with the
performance or find a better compiler.
Or possibly do the brute-force method of using a macro.

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

I pointed it out because switching rounding mode without documenting
the reason makes the code a bit hard to read, and even harder to
optimize because nobody knows whether there is a specific reason
why round to nearest is used or if it could just be removed for slightly
better performance.

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

Sure, only it can create infinity from non-infinity inputs.
But what happens then? If you now use the result of a division with an
add it won't quite work right in case it was an infinity as far as I can
tell. At least not IEEE-style if it's (-1/0)+(1/0).
So either you have to assume that a division will always be the last
operation ever done or supporting infinity here is kind of pointless
unless you support it anywhere as input at least.
Maybe inf will actually work in a kind of reasonable (but not
IEEE-compliant) way in the other functions, but it's not clear to
me anyone tested that.

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

Ok, but then I still don't understand why the exp redirection.

if (manta == 0) return 0;
(if a is 0, then it can't be greater)

if (mantb == 0) return 1;
(if a is not 0, but b is 0, then a is greater)

And the rest can also be simplified to e.g. just these 3 lines:
if (expa > expb) return 1;
if (expa < expb) return 0;
return manta > mantb;
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list