[FFmpeg-devel] NellyMoser transform bug

[Benjamin Larsson]

Fabrice Bellard wrote:
> Benjamin Larsson wrote:
>> Fabrice Bellard wrote:
>>> Hi,
>>>
>>> While looking at the NellyMoser audio decoder, I found a very 
>>> suspect code which makes the transform incorrect in the sense that 
>>> there is information loss. So I suggest the attached patch.
>>>
>>> Can someone confirm that my code corresponds to what the "reference" 
>>> decoder does ? If it does, then I confirm that it is possible to 
>>> convert the code to use the ffmpeg IMDCT code...
>>>
>>> Fabrice.
>>>
>>>   
>> This code should be from the unpack_coeffs function in nelly.c from 
>> http://nelly2pcm.googlecode.com/files/nelly2pcm.tar.bz2. And it looks 
>> like a typo from my cleanup work. Thanks for catching this and feel 
>> free to rewrite it to use the regular mdct or tell me how to do it.
>>  [...]
>
> In the attached patch you can find the modified code to use the IMDCT. 
> I also made a few simplications. What is missing is that the windowing 
> and overlapping could be simplified too by directly using the output 
> from the IMDCT, but I don't think it is critical.
>
> The difference between the samples from the new code and the corrected 
> old one doesn't exceed one LSB for http://ffmpeg.pl.devwap.com/input.flv.
>
> Regards,
>
> Fabrice.
> ------------------------------------------------------------------------
>
> Index: nellymoserdec.c
> ===================================================================
> --- nellymoserdec.c	(revision 10857)
> +++ nellymoserdec.c	(working copy)
> @@ -46,7 +46,7 @@
>  #define NELLY_BIT_CAP     6
>  #define NELLY_BASE_OFF    4228
>  #define NELLY_BASE_SHIFT  19
> -#define NELLY_SAMPLES     256
> +#define NELLY_SAMPLES     (2 * NELLY_BUF_LEN)
>  
>  static const float dequantization_table[127] = {
>  0.0000000000,-0.8472560048, 0.7224709988, -1.5247479677, -0.4531480074, 0.3753609955, 1.4717899561,
> @@ -98,14 +98,12 @@
>      int             add_bias;
>      int             scale_bias;
>      DSPContext      dsp;
> -    FFTContext      fftc;
> +    MDCTContext     imdct_ctx;
> +    DECLARE_ALIGNED_16(float,imdct_tmp[NELLY_BUF_LEN]);
> +    DECLARE_ALIGNED_16(float,imdct_out[NELLY_BUF_LEN * 2]);
>  } NellyMoserDecodeContext;
>  
> -
>   

Cosmetics.

> [...]
>
> +        a = 1.0 / 8.0;
> +        for(j = 0; j < NELLY_BUF_LEN / 2; j++) {
> +            aptr[j] = s->imdct_out[j + NELLY_BUF_LEN + NELLY_BUF_LEN / 2] * a;
> +            aptr[j + NELLY_BUF_LEN / 2] = -s->imdct_out[j] * a;
> +        }
>   
And can this be moved to the overlap_and_window function ?
>          overlap_and_window(s, s->state, aptr);
>   

Other then that feel free to commit with the small simplifications as a 
separate commit.

MvH
Benjamin Larsson