[FFmpeg-devel] [PATCH] wmapro decoder

[Vitor Sessak]

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

That's due to the lack of clipping. The attached patch is bit-exact for 
wav output for me for the sample 
http://samples.mplayerhq.hu/A-codecs/WMA9/wmapro/Beethovens%20nionde%20symfoni%20(Scherzo)-2.wma 
.

Note that outputting floats can make the "Output buffer too small" error 
twice as bad, since the decoder will need an output buffer twice the size.

> Also the scaling can be done in the imdct.

Yes, my patch was just a proof-of-concept.

>>> 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?

Change /** to /*

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

fine for me.

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

Indeed. 10l for me.

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wma_use_float2.diff
Type: text/x-diff
Size: 2031 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090621/ec775ee0/attachment.diff>