[FFmpeg-soc] [soc]: r3941 - in wmapro: checkout.sh mdct.diff wma3.h wma3data.h wma3dec.c wmapro_ffmpeg.patch

[Sascha Sommer]

Hi,

Thanks for the fast review.

On Samstag, 10. Januar 2009, Diego Biurrun wrote:
> On Sat, Jan 10, 2009 at 08:24:34PM +0100, faust3 wrote:
> > Log:
> > added a bit of code
> >
> > --- /dev/null	00:00:00 1970	(empty, because file is newly added)
> > +++ wmapro/wma3.h	Sat Jan 10 20:24:33 2009	(r3941)
> > @@ -0,0 +1,166 @@
> > +
> > +#ifndef WMA3_H
> > +#define WMA3_H 1
>
> Drop the '1', AVCODEC_ prefix is missing.
>

Done.

> > +/* size of blocks defines taken from wma.h*/
>
> /* size of block defines taken from wma.h */
>
> > +    uint16_t channel_len;                 //< channel len in samples
> > +    uint16_t decoded_samples;                   //< number of samples
> > that have been processed already
>
> Could be aligned.
>
> > +typedef struct {
> > +    int nb_channels;
> > +    int no_rotation; //< controls the type of the transform
> > +    int transform; //< also controls the type of the transform
> > +    char transform_band[MAX_BANDS]; //< controls if the transform is
> > enabled for a certain band +    char rotation_offset[MAX_CHANNELS *
> > MAX_CHANNELS];
> > +    char positive[MAX_CHANNELS * MAX_CHANNELS]; //< fixme for what are
> > these numbers used?
>
> ditto
>

Both not done yet. Layout of these will change a bit. I'll fix afterwards. I 
also need to add more comments.

> > +#endif
>
> missing comment
>

Done

> > --- /dev/null	00:00:00 1970	(empty, because file is newly added)
> > +++ wmapro/wma3data.h	Sat Jan 10 20:24:33 2009	(r3941)
> > @@ -0,0 +1,502 @@
> > +
> > +#define FF_WMA3_SCALE_SIZE 121
> > +#define FF_WMA3_SCALE_MAXBITS 19
> > +static const uint32_t ff_wma3_scale_huffcodes[FF_WMA3_SCALE_SIZE] = {
>
> The second #define seems unused.
>
> > +};
> > +static const uint8_t ff_wma3_scale_huffbits[FF_WMA3_SCALE_SIZE] = {
>
> Here and in other places: It would be more readable if you left an empty
> line between tables.
>
> > +#define FF_WMA3_COEF0_SIZE 244
> > +#define FF_WMA3_COEF0_MAXBITS 22
>
> unused again?  more instances below..
>

Will be fixed otherwise.


> > +static const short ff_wma3_run_0[] = {
> > +0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
> > +20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37,
> > 38, 39, +40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55,
> > 56, 57, 58, 59, +60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73,
> > 74, 75, 76, 77, 78, 79, +80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91,
> > 92, 93, 94, 95, 96, 97, 98, 99, +0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11,
> > 12, 13, 14, 15, 16, 17, 18, 19, +20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
> > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, +0, 1, 2, 3, 4, 5, 0, 1, 2, 0, 1, 2, 0, 1,
> > 2, 0, 1, 0, 1, 0,
> > +1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0,
> > +1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0,
> > +1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0,
> > +1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0,
> > +0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > +};
>
> This one and other tables would be more readable if you aligned the values.
>

Done

> > --- wmapro/wma3dec.c	Sat Jan 10 03:31:27 2009	(r3940)
> > +++ wmapro/wma3dec.c	Sat Jan 10 20:24:33 2009	(r3941)
> > +static av_cold int wma3_decode_end(AVCodecContext *avctx);
>
> bleh, forward declaration..
>

Fixed

> > @@ -200,14 +160,187 @@ static av_cold int wma3_decode_init(AVCo
> > +static int wma_decode_channel_transform(WMA3DecodeContext* s,
> > GetBitContext* gb){
>
> You use a mix of K&R style function declarations and this style in the
> file.  Please use K&R.
>

Fixed

> > @@ -400,6 +824,497 @@ static int wma_decode_tilehdr(WMA3Decode
> >
> > +/**
> > + * Apply MDCT window and add into output.
> > + *
> > + * We ensure that when the windows overlap their squared sum
> > + * is always 1 (MDCT reconstruction rule).
> > + */
> > +
> > +
> > +
> > +
> > +
> > +static int wma_decode_subframe(WMA3DecodeContext *s,GetBitContext* gb){
>
> Stray empty lines?
>

Fixed

> >  /**
> >   *@brief WMA9 decoder
> >   */
> > -AVCodec wmav3pro_decoder =
> > +AVCodec wmapro_decoder =
> >  {
> >      "wmav3pro",
>
> I think this should be one or the other name, but not a mix...
>

Not now.

> > --- wmapro/wmapro_ffmpeg.patch	Sat Jan 10 03:31:27 2009	(r3940)
> > +++ wmapro/wmapro_ffmpeg.patch	Sat Jan 10 20:24:33 2009	(r3941)
> > @@ -1,24 +1,24 @@
> > ---- libavcodec/Makefile	(revision 13558)
> > +--- libavcodec/Makefile	(revision 16517)
> >  +++ libavcodec/Makefile	(working copy)
> > +@@ -233,6 +233,7 @@
> > + OBJS-$(CONFIG_WMAV1_ENCODER)           += wmaenc.o wma.o
> > + OBJS-$(CONFIG_WMAV2_DECODER)           += wmadec.o wma.o
> > + OBJS-$(CONFIG_WMAV2_ENCODER)           += wmaenc.o wma.o
> > ++OBJS-$(CONFIG_WMAPRO_DECODER)          += wma3dec.o
>
> This does not depend on wma.o?
>

No.

Please note that I want to fix a few other things before I go on with the 
consistency fixes in case you have more points to add to this list.

Regards

Sascha