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

[Michael Niedermayer]

Hi

On Wed, Sep 12, 2007 at 10:58:29PM +0200, Lo?c Minier wrote:
>         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.)

Side note: the person posting a patch will always be flamed and made
responsible for any errors in the patch, misunderstandings and the
current weather even if he didnt create any of the previous :)


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

well try our fft please :)
or feed the inverse_dft with an array of all 0 and just the first element 1
then only the second element 1, then only the third, then only the last and
post the 4 outputs here and we will maybe be able to tell what it is


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

then call it antialias for the moment, maybe its correct maybe not but
unpack_coeffs() is definitly wrong


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

well i think you overestimate it


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

change the whole to LGPL, and merge the ffmpeg wraper with the actual
decoder (or if you prefer chnage your code to BSD, but having it split in
decoder and ffmpeg wraper is not clean)


> 
> > [...]
> > > +		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.  :)

try apply_window() for now


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

read the fine manual (
 /* C version: convert floats from the range [384.0,386.0] to ints in [-32768,32767]
     * simd versions: convert floats from [-32768.0,32767.0] without rescaling and arrays are 16byte aligned */
    void (*float_to_int16)(int16_t *dst, const float *src, int len);
)
it clearly says [384.0,386.0] -> [-32768,32767] for C


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

remove the while loop, if it doesnt work look at AVParser / parser.c
it shouldnt be hard to split the blocks in NELLY_BLOCK_LEN units

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070913/0eee3649/attachment.pgp>