[FFmpeg-devel] [PATCH] NellyMoser audio decoder v2

[Loïc Minier]

        Hi,

 First, thanks for your reviews.

 (Side note: most code I submitted initially was the verbatim code as
 found in the nelly2pcm download I mentionned; I'll try to adapt them to
 match ffmpeg standards as I would enjoy having NellyMoser in ffmpeg,
 but the original work isn't mine.)

 Changes since original submission (from git shortlog):
      Retab with tab size of 4
      Switch nelly_init_table from type short to int16_t
      Remote trailing whitespace
      Include stdint.h for int16_t
      Merge tables into nelly.c; make tables used only once static
      Prefix table names with ff_
      Use FFSWAP in center()
      Make the data tables const
      Drop superfluous parenthesis in unpack_coeffs()
      Only shift b in sum_bits() if it's non-zero as a shifted zero would be zer
      Use av_clip(b, 0, NELLY_BIT_CAP) instead of if/else
      Simplify sum_bits() even more when b is zero
      Use FFMIN() instead of av_clip() and pack sum_bits() even more
      Rewrite headroom() with av_log2()
      Cleanups in get_sample_bits(), use FFMIN/FFMAX and rename some vars
      Change headroom() to return l/shift instead of changing a short*
      Group for loops in get_sample_bits()
      Group while loops in get_sample_bits() by checking FFABS(diff) <= 16383
      Add a signed_shift() helper inline func to shift left or right and use it
      Use av_clip() instead of FF_MIN in sum_bits()
      Use av_clip() instead of custom logic in get_sample_bits()
      Minor readability tweak
      Post-increment pointer while writing them instead of separately
      Split zeroing of pows and buf as buf is an int array
      Use "bitwise and with 1" instead of "modulo 2"
      Add my copyright
      Drop unused audio format vars
      Adapt comment in nelly.h to be compatible with doxygen
      Rename ff_nelly_copy_count to ff_band_sizes_table
      Rename ff_nelly_huff_table to ff_dequantization_table

On Tue, Sep 11, 2007, Michael Niedermayer wrote:
> > +static void inverse_dft(float *audio)
> if this is a standard dft (discrete fourier transform) then please use
> the existing code from fft.c 
> if its not a dft then please elaborate on what it is

 I have no idea what it is; like you, I simply read its name.  I didn't
 change it yet.

> > +static void unpack_coeffs(float *buf, float *audio)
>      this function doesnt unpack anything, it outputs the same number
> of coeffs as are input, it looks more like some antialias like filter
> similar to what mp3 does

 I have no idea what it does here either.

> [...]
> > +static unsigned char get_bits(unsigned char block[NELLY_BLOCK_LEN], int *off, int n)
> please use the bitreader from bitstream.c/h

 This looks like a bigger task.

> > +void nelly_decode_block(NellyMoserDecodeContext *s, unsigned char block[NELLY_BLOCK_LEN], float audio[256])
> > +{
> should be static

 This is used by the actual ffmpeg decoder in nellymoserdec.c; do you
 want these files to be merged?  I followed another advice to merge the
 tables which were a standalone file, but the decoder is currently LGPL,
 so I would need to change license one way or the other (I can do both:
 either keep the original license documented but switch to effective
 LGPL or make the ffmpeg part I wrote public domain).

> [...]
> > +		unpack_coeffs(buf, aptr);
> 
> > +		center(aptr);
> 
> this reorders the coefficients for the fft, dont give functions nonsense
> names
[...]
> > +		apply_state(s->state, aptr);
> 
> this applies a window name it appropriately

 I wish I would know what these functions actually do so I would be able
 to name them properly.  :)

> > +void nelly_util_floats2shorts(float audio[256], short shorts[256])
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < 256; i++) {
> > +		if (audio[i] >= 32767.0)
> > +			shorts[i] = 32767;
> > +		else if (audio[i] <= -32768.0)
> > +			shorts[i] = -32768;
> > +		else
> > +			shorts[i] = (short)audio[i];
> > +	}
> > +}
> 
> duplicate of the respective code in dsputil

 Hmm replacing nelly_util_floats2shorts() with ff_float_to_int16_c()
 ended in some garbage.

> [...]
> > +static int decode_tag(AVCodecContext * avctx,
> > +                      void *data, int *data_size,
> > +                      uint8_t * buf, int buf_size) {
> > +    NellyMoserDecodeContext *s = avctx->priv_data;
> > +    int remaining_size = buf_size;
> > +    *data_size = 0;
> > +
> > +    while (remaining_size >= NELLY_BLOCK_LEN) {
> > +        nelly_decode_block(s, buf, s->float_buf);
> > +        nelly_util_floats2shorts(s->float_buf, data);
> > +        data += NELLY_SAMPLE_SIZE;
> > +        *data_size += NELLY_SAMPLE_SIZE;
> > +        buf += NELLY_BLOCK_LEN;
> > +        remaining_size -= NELLY_BLOCK_LEN;
> > +    }
> 
> there should be a parser which splits these so the decoder is only feeded
> with individual blocks

 I have no idea whether you have the guarantee or not that a tag will
 hold one block or multiple ones.

    Bye,
-- 
Lo?c Minier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nellymoser-v2.patch
Type: text/x-diff
Size: 29749 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070912/c2b42e69/attachment.patch>