[FFmpeg-devel] [PATCH] wmapro decoder
Vitor Sessak
vitor1001
Mon Jun 1 17:58:34 CEST 2009
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
More information about the ffmpeg-devel
mailing list