[FFmpeg-devel] [PATCH] HE-AAC v1 decoder

[Vitor Sessak]

Alex Converse wrote:
> On Wed, Jan 27, 2010 at 11:34 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> On Wed, Jan 27, 2010 at 11:15:23AM -0500, Alex Converse wrote:
>>> On Wed, Jan 27, 2010 at 8:53 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>> On Wed, Jan 27, 2010 at 05:53:24AM -0500, Alexander Strange wrote:
>>>>> On Jan 26, 2010, at 10:10 PM, Alex Converse wrote:
>>>> [...]
>>>>>> +    for (k = 0; k < num_bands-1; k++) {
>>>>>> +        prod *= base;
>>>>>> +        present  = lroundf(start * prod);
>>>>>> +        bands[k] = present - previous;
>>>>>> +        previous = present;
>>>>>> +    }
>>>>> You use lroundf() a lot, which is a library call for me. Can't you set the rounding direction (or just ignore it) and cast instead?
>>>> whats wrong with lrintf() ?
>>> *round*() family functions round their argument to the nearest integer
>>> value, rounding away from zero, regardless of the current rounding
>>> direction. *rint*() uses the current rounding direction. There are a
>>> few cases where things can end up at exactly 0.5 and it makes a big
>>> difference.
>> then the code is broken, floats are not exact, especially not floats in
>> C. floats in ASM might be useable like that but in C assuming that a
>> calculation like lround(a/b) will have a 0.5 result rounded the way
>> lround should is russian roullet.
>> you only need a lround(c/b) close by and many compilers includig gcc
>> will calculate x=1/b and do lround(a*x) and lround(c*x) also
>> the compiler can choose to keep values in registers that are more
>> precisse than doubles or store as floats in intermediate memory loctions
>> Code that depends on the rounding direction of 0.5 definitly is a
>> time bomb.
>>
> 
> Numbers like 0.f, 2.f, 4.f, 0.5f, 1.5f are all exactly representable
> with IEEE 754.

While it might even work on all our platforms, it surely is less robust 
than integers. Also I really think it makes the code much more 
complicated and less readable. How many people can tell without testing 
or reading the doc the output of

printf("%i %i %i\n", (int) (1./2 + 0.5), lroundf(.5), lrintf(.5));
?

There is also the risk someone decides in the future to "optimize" or 
"cleanup" the lround() to lrint().

>> Besides that a quick grep for round indicates that its used for many
>> calculations with pure integer input and pure integer output
>> like lroundf((float)a / (float)b);
>> such calculations should be done purely with integers, that way
>> you get always the correct result ...
>>
> 
> According to the original thread
> <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/2009-November/008425.html>
> there are two lroundf()s that are important:
> 
> +            sbr->n_master =         lroundf((sbr->k[2] - sbr->k[0]) *
> 0.25f)  << 1;
> That one looks trivially fixable.
> 
> +        int temp = lroundf(abs_bord_trail / (float)ch_data->bs_num_env[1]);
> when bs_num_env[1] is 2 or 4.
> 
> In general it seems to be a good idea to follow the recommendation of
> the spec when possible.

This is just my opinion, but in this case I really don't think it is 
worth the downsides...

-Vitor