[FFmpeg-devel] [PATCH] wmapro decoder

[Sascha Sommer]

Hi,

On Montag, 1. Juni 2009, Vitor Sessak wrote:
> Sascha Sommer wrote:
> > Hi,
> >
> > attached patch adds support for decoding wmapro.
> >
> > I'm awaiting your review so that we can sort out the remaining issues.
>
> Nice! Some nitpicking:
> > +    /** FIXME: is this really the right thing to do for 24 bits? */
> > +    s->sample_bit_depth = 16; // avctx->bits_per_sample;
>
> I'd say that for 24 bits you should set somewhere
>
>     avctx->sample_fmt = SAMPLE_FMT_S32;
>
> and use a 32-bit buffer for output. Also for the 16 bit case, it is a
> good idea to set sample_fmt to SAMPLE_FMT_S16.
>

or SAMPLE_FMT_FLOAT

> > +    /** set up decorrelation matrixes */
> > +    s->def_decorrelation_mat = av_mallocz(sizeof(float*) * (MAX_CHANNELS
> > + 1)); +    if (!s->def_decorrelation_mat) {
> > +        av_log(avctx, AV_LOG_ERROR,
> > +               "failed to allocate decorrelation matrix\n");
> > +        wma_decode_end(avctx);
> > +        return -1;
> > +    }
> > +
> > +    /** FIXME more than 6 coupled channels not supported */
> > +    s->def_decorrelation_mat[1] =
> > &ff_wma3_default_decorrelation_matrices[0]; +   
> > s->def_decorrelation_mat[2] = &ff_wma3_default_decorrelation_matrices[1];
> > +    s->def_decorrelation_mat[3] =
> > &ff_wma3_default_decorrelation_matrices[5]; +   
> > s->def_decorrelation_mat[4] =
> > &ff_wma3_default_decorrelation_matrices[14]; +   
> > s->def_decorrelation_mat[5] =
> > &ff_wma3_default_decorrelation_matrices[30]; +   
> > s->def_decorrelation_mat[6] =
> > &ff_wma3_default_decorrelation_matrices[55]; +
>
> This can be vastly simplified by adding
>
> static const float *ff_wma3_default_decorrelation[] = {
>      &ff_wma3_default_decorrelation_matrices[0],
>      &ff_wma3_default_decorrelation_matrices[1],
>      &ff_wma3_default_decorrelation_matrices[5],
>      &ff_wma3_default_decorrelation_matrices[14],
>      &ff_wma3_default_decorrelation_matrices[55]
> }
>
> Right after the definition of ff_wma3_default_decorrelation_matrices
> (which them shouldn't be used anywhere else).
>

Ok, I will change that.

> > +                        /** 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;
>
> Is this supposed to be this value or M_SQRT1_2 (that is actually
> 0.70710678118654752440)?
>

These values are used in the binary decoder. I think Benjamins guess of 
cos(pi/4) should be more correct though.

> > +/**
> > + *@brief Decode an uncompressed coefficient.
>
> I think that doxy comments without the "@brief" tag is preferred.
>

Ok.

> > +/**
> > + *@brief Apply sine window and reconstruct the output buffer.
> > + *@param s codec context
> > + */
> > +static void wma_window(WMA3DecodeContext *s)
> > +{
> > +    int i;
> > +    for (i=0;i<s->channels_for_cur_subframe;i++) {
> > +        int c = s->channel_indexes_for_cur_subframe[i];
> > +        float* start;
> > +        float* window;
> > +        int winlen = s->channel[c].prev_block_len;
> > +        start = s->channel[c].coeffs - (winlen >> 1);
> > +
> > +        if (s->subframe_len < winlen) {
> > +            start += (winlen - s->subframe_len)>>1;
> > +            winlen = s->subframe_len;
> > +        }
> > +
> > +        window = s->windows[av_log2(winlen)-BLOCK_MIN_BITS];
> > +
> > +        winlen >>= 1;
> > +
> > +        s->dsp.vector_fmul_window(start, start, start + winlen,
> > +                                  window, 0, winlen);
>
> Hmmm... I think the start buffer here is not properly aligned...

What makes you think so?

Coeff is calculated as:
        s->channel[c].coeffs = &s->channel[c].out[(s->samples_per_frame>>1)
                                                  + offset];

Out is 16byte aligned. samples_per_frame goes from 128 to 4096 and is always 
in the form 2^N. Offset is a multiple of 128.
Winlen and subframe_len are also 2^N numbers.


>
> > ===================================================================
> > --- libavcodec/wma3.h	(revision 0)
> > +++ libavcodec/wma3.h	(revision 0)
>
> I'd say this header is only used by wma3dec.c. Unless there is a plan
> for in the future using it elsewhere, I think it should just be
> copy-and-pasted to wma3dec.c.
>

Can be done.

> > Index: doc/general.texi
> > ===================================================================
> > --- doc/general.texi	(revision 19071)
> > +++ doc/general.texi	(working copy)
> > @@ -613,6 +613,7 @@
> >  @item Windows Media Audio 1  @tab  X  @tab  X
> >  @item Windows Media Audio 2  @tab  X  @tab  X
> >      @tab Used in Origin's Wing Commander IV AVI files.
> > + at item Windows Media Audio Pro @tab  X  @tab  X
>
> I think only decoding is supported (hence only the second "X" should be
> there).
>

Oups. Also the  "@tab Used in Origin's Wing Commander IV AVI files." looks 
wrong.

Regards

Sascha