[FFmpeg-devel] [PATCH] wmapro decoder

[Sascha Sommer]

Hi Vitor,

thanks for your comments.

> > The code still outputs shorts. Float produced clicks all around the
> > place. I'm not sure yet if the problem is in my code or in ffmpeg.
>
> It looks like it works for me, see attached.

So your code does not produce clicks at second 16-18 for you with the sample 
file you mentioned below? It does for me when I'm using ffmpeg to produce a 
16-bit wav file and play that back with MPlayer.
Also the scaling can be done in the imdct.

>
> > Float output can also be added after the code has been added to svn imho.
> >
> > New patch attached.
> >
> >
> > +/**
> > + * @brief main decoder context
> > + */
>
> The @brief here makes the plain C code harder to read without improving
> a lot doxy output, so it is better to remove it (happens a lot elsewhere).
>

Do others agree?
I don't think it makes the code any harder to read.

> > +    /** generic decoder variables */
> > +    AVCodecContext*  avctx;                         ///< codec context
> > for av_log +    DSPContext       dsp;                           ///<
> > accelerated dsp functions +    uint8_t          frame_data[MAX_FRAMESIZE
> > +
> > +                      FF_INPUT_BUFFER_PADDING_SIZE];///< compressed
> > frame data +    MDCTContext      mdct_ctx[WMAPRO_BLOCK_SIZES];  ///< MDCT
> > context per block size +    DECLARE_ALIGNED_16(float,
> > tmp[WMAPRO_BLOCK_MAX_SIZE]); ///< imdct output buffer +    float*        
> >   windows[WMAPRO_BLOCK_SIZES];   ///< window per block size +    int     
> >         coef_max[2];                   ///< max length of vlc codes
>
> The "/** generic decoder variables */" will be misinterpreted by doxygen
> as been only referent to "AVCodecContext*  avctx;". There is a doxygen
> syntax for grouping.
>

Yes and the grouping syntax leads to the same problem. Looks like a doxygen 
problem. Any other ideas?

> > +    s->num_sfb = av_mallocz(sizeof(int8_t)*s->num_possible_block_sizes);
> > +    s->sfb_offsets = av_mallocz(MAX_BANDS *
> > +                                sizeof(int16_t) *
> > s->num_possible_block_sizes); +    s->subwoofer_cutoffs =
> > av_mallocz(sizeof(int16_t) *
> > +                                      s->num_possible_block_sizes);
> > +    s->sf_offsets = av_mallocz(MAX_BANDS * s->num_possible_block_sizes *
> > +                               s->num_possible_block_sizes *
> > sizeof(int16_t));
>
> There is no necessity of clearing s->num_sfb, it will be filled up in
> the next few lines (is clearing the others needed?), so you can use
> plain av_malloc().
>

I prefer to use av_mallocz on places where speed does not matter.

> > +/**
> > + *@brief Calculate a decorrelation matrix from the bitstream parameters.
> > + *@param s codec context
> > + *@param chgroup channel group for which the matrix needs to be
> > calculated + */
> > +static void decode_decorrelation_matrix(WMA3DecodeContext* s,
> > +                                            WMA3ChannelGroup* chgroup)
> > +{
> > +    int i;
> > +    int offset = 0;
> > +    char rotation_offset[WMAPRO_MAX_CHANNELS * WMAPRO_MAX_CHANNELS];
> > +    memset(chgroup->decorrelation_matrix,0,
> > +           sizeof(float) *s->num_channels * s->num_channels);
> > +
> > +    for (i=0;i<chgroup->num_channels  * (chgroup->num_channels - 1) >>
> > 1;i++) +        rotation_offset[i] = get_bits(&s->gb,6);
> > +
> > +    for (i=0;i<chgroup->num_channels;i++) {
> > +        if (get_bits1(&s->gb)) {
> > +            chgroup->decorrelation_matrix[chgroup->num_channels * i +
> > i]=  1.0; +        } else
> > +            chgroup->decorrelation_matrix[chgroup->num_channels * i +
> > i]= -1.0;
>
> chgroup->decorrelation_matrix[chgroup->num_channels * i + i] =
>                                        -1 + get_bits1(&s->gb)<<1;
>

That is slower.

> or
>
> chgroup->decorrelation_matrix[chgroup->num_channels * i + i] =
>
>                                     get_bits1(&s->gb) ? 1.0, -1.0;
>

Thanks. That seems to be slightly faster.

> > +                    if (s->num_channels == 2) {
> > +                        chgroup->transform = 1;
> > +                    } else {
> > +                        chgroup->transform = 2;
> > +                        /** cos(pi/4) */
> > +                        chgroup->decorrelation_matrix[0] = 0.70703125;
> > +                        chgroup->decorrelation_matrix[1] = -0.70703125;
> > +                        chgroup->decorrelation_matrix[2] = 0.70703125;
> > +                        chgroup->decorrelation_matrix[3] = 0.70703125;
>
> Already mentioned...
>

Yes. These values were used to produce binary identical output in the past. So 
I will keep using those value.

> > +
> > +    if (len <= 0 || buflen > MAX_FRAMESIZE) {
> > +         ff_log_ask_for_sample(s->avctx, "input buffer to small\n");
>
> s/to small/too small/
>

Fixed in the soc repository.

> Note also that the sample at
> http://samples.mplayerhq.hu/A-codecs/WMA9/wmapro/New%20Stories%20(Highway%2
>0Blues)-2.wma trigger this error.
>

Hm. Seems to be the same problem that I fixed in revision 4492 in the soc 
repository.

> > +    for (i=0;i<s->num_channels;i++) {
> > +        s->channel[i].decoded_samples = 0;
> > +        s->channel[i].cur_subframe = 0;
> > +        s->channel[i].reuse_sf = 0;
> > +    }
>
> memset(...)
>

Can't be used because some data like prev_block_len and the out buffer is not 
supposed to be resetted between the packets. They will be used for windowing.
 
Regards

Sascha