[FFmpeg-devel] [PATCH] wmapro decoder

[Vitor Sessak]

Sascha Sommer wrote:
> 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.

BTW, I forgot to note in my previous message that this ff_ prefix here 
is useless. wma3data.h is not considered a public header, since 
including it elsewhere would means duplicating all the tables (which is 
unacceptable).


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

Indeed, 10l for me.

-Vitor