[FFmpeg-devel] [PATCH] Add float support to wavpack decoder

[Reimar Döffinger]

On Tue, May 05, 2009 at 12:02:58AM +0200, Laurent Aimar wrote:
> I submit them to have your comments about them as I still dislike one part
> of it: I suppose IEEE 754 float storage to convert from sign, mantissa and
> exponent to a float value. I am open to any ideas to avoid that unless it
> is acceptable.

Of course it is possible to avoid, you can e.g. use ldexp. But it is
slow, so e.g. aac does not use it.
Or you can use av_int2flt which uses it but is even slower (though
possibly could be optimized to just use the union{} stuff where
possible, making it inline and then also using it for AAC).

> +    if(S){
> +        S <<= s->float_shift;

Why S ? That is such a useless variable name, doubly here where it seems
to be only the mantissa.

> +        }else if(exp){
> +            int shift = 23 - av_log2(S);
> +            exp = s->float_max_exp;
> +            if(exp <= shift){
> +                shift = --exp;
> +            }
> +            exp -= shift;
> +
> +            if(shift){
> +                S <<= shift;
> +                if((s->float_flag & WV_FLT_SHIFT_ONES) ||
> +                   (s->got_extra_bits && (s->float_flag & WV_FLT_SHIFT_SAME) && get_bits1(&s->gb_extra_bits)) ){
> +                    S |= (1 << shift) - 1;
> +                } else if(s->got_extra_bits && (s->float_flag & WV_FLT_SHIFT_SENT)){
> +                    S |= get_bits(&s->gb_extra_bits, shift);
> +                }
> +            }
> +            S &= 0x7fffff;
> +        }else{
> +            S &= 0x7fffff;
> +            exp = s->float_max_exp;
> +        }

The "S &= 0x7fffff;" could be factored out.

> +    (*crc) = (*crc) * 27 + S * 9 + exp * 3 + sign;

Useless ()

> +    value.u = (sign << 31) | (exp<<23) | S;

Spaces are inconsistent.

> +    if(!got_float && avctx->sample_fmt == SAMPLE_FMT_FLT){
> +        av_log(avctx, AV_LOG_ERROR, "Float informations not found\n");

"information" is not countable, the word "informations" does not exist.