[FFmpeg-devel] [PATCH] wmapro decoder

[Vitor Sessak]

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.

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

> +                        /** 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)?

> +/**
> + *@brief Decode an uncompressed coefficient.

I think that doxy comments without the "@brief" tag is preferred.

> +/**
> + *@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...

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

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

-Vitor