[FFmpeg-devel] [PATCH] wmapro decoder
Sascha Sommer
saschasommer
Sat Jun 20 14:50:24 CEST 2009
Hi Michael,
thanks for the review. See my answers below.
> > +/**
> > + *@brief Get the samples per frame for this stream.
> > + *@param sample_rate output sample_rate
> > + *@param decode_flags codec compression features
> > + *@return number of output samples per frame
> > + */
> > +static int av_cold wma_get_samples_per_frame(int sample_rate,
> > + unsigned int decode_flags)
> > +{
> > +
> > + int samples_per_frame;
> > + int tmp;
> > +
> > + if (sample_rate <= 16000)
> > + samples_per_frame = 512;
> > + else if (sample_rate <= 22050)
> > + samples_per_frame = 1024;
> > + else if (sample_rate <= 48000)
> > + samples_per_frame = 2048;
> > + else if (sample_rate <= 96000)
> > + samples_per_frame = 4096;
> > + else
> > + samples_per_frame = 8192;
>
> mergeable with the related code from ff_wma_init()
>
Done.
> > +
> > + av_free(s->def_decorrelation_mat);
>
> i tend to prefer av_freep() for sake of saftey
>
Changed.
> > + /** dump the extradata */
> > + for (i=0 ; i<avctx->extradata_size ; i++)
> > + av_log(avctx, AV_LOG_DEBUG, "[%x] ",avctx->extradata[i]);
> > + av_log(avctx, AV_LOG_DEBUG, "\n");
>
> this stuff will have to be droped or made accessible via
> AVCodecContext.debug in the final version
>
I changed it to dprintf so that it is not compiled in by default.
> > + if (!s->len_prefix) {
> > + av_log(avctx, AV_LOG_ERROR, "no length prefix, please
> > report\n");
>
> ff_log_ask_for_sample
>
Replaced.
> >
> > + s->max_num_subframes = 1 << ((s->decode_flags & 0x38) >> 3);
> > + s->num_possible_block_sizes = av_log2(s->max_num_subframes) + 1;
>
> IMHO ugly, i mean 1<< and then log2
>
Changed.
> > + if (channel_mask & 8) {
> > + unsigned int mask = 1;
> > + for (i=0;i<32;i++) {
> > + if (channel_mask & mask)
> > + ++s->lfe_channel;
> > + if (mask & 8)
> > + break;
> > + mask <<= 1;
> > + }
> > + }
>
> looks buggy
> the loops would do 32 iterations but the tests limit it to 4
>
Fixed.
> > + int subframe_len = s->samples_per_frame / (1 << i);
>
> samples_per_frame >> i
>
Fixed.
> > + /** calculate subframe len bits */
> > + if (s->lossless)
> > + subframe_len_bits = av_log2(s->max_num_subframes - 1) + 1;
> > + else if (s->max_num_subframes == 16) {
>
> if{
> }else
>
Fixed.
> > + i = 0;
> > + while (i < 4) {
>
> [...]
>
> > + i += 2;
>
> for()
>
Changed.
> > sf++) { + val = get_vlc2(&s->gb, sf_vlc.table,
> > SCALEVLCBITS, +
> > ((FF_WMA3_HUFF_SCALE_MAXBITS+SCALEVLCBITS-1)/SCALEVLCBITS)); +
> > *sf = *(sf - 1) + val - 60;
> > + }
>
> the way this line is wraped is very ugly, even not wraping at all would be
> better
> also the calculation of the stages could be moved into some define, greatly
> simplifyng the look of get_vlc*
>
Done.
> val= 45 / s->channel[c].scale_factor_step;
> for(sf= s->channel[c].scale_factors...){
> val += get_vlc2() - 60;
> *sf= val;
> }
>
Fixed.
> > + for (i=0;i<s->num_bands;i++) {
> > + int idx;
> > + short skip;
> > + short val;
> > + short sign;
>
> is there any reason why they are short ?
>
No. Changed to int.
> > +
> > + idx = get_vlc2(&s->gb, sf_rl_vlc.table, VLCBITS,
> > +
> > ((FF_WMA3_HUFF_SCALE_RL_MAXBITS+VLCBITS-1)/VLCBITS)); +
> > + if ( !idx ) {
> >
> > + uint32_t code = get_bits(&s->gb,14);
> > + val = code >> 6;
> > + sign = (code & 1) - 1;
> > + skip = (code & 0x3f)>>1;
>
> is that faster than 3 get_bits() ?
>
Yes.
> > +/**
> > + *@brief Decorrelate and undo M/S stereo coding.
>
> decorrelation is what the encoder does
>
Fixed.
> > + *@param s codec context
> > + */
> > +static void wma_inverse_channel_transform(WMA3DecodeContext *s)
> > +{
> > + int i;
> > +
> > + for (i=0;i<s->num_chgroups;i++) {
> > +
> > + if (s->chgroup[i].transform == 1) {
> > + /** M/S stereo decoding */
> > + int16_t* sfb_offsets = s->cur_sfb_offsets;
> > + float* ch0 = *sfb_offsets + s->channel[0].coeffs;
> > + float* ch1 = *sfb_offsets++ + s->channel[1].coeffs;
> > + const char* tb = s->chgroup[i].transform_band;
> > + const char* tb_end = tb + s->num_bands;
> > +
> > + while (tb < tb_end) {
> > + const float* ch0_end = s->channel[0].coeffs +
> > +
> > FFMIN(*sfb_offsets,s->subframe_len); + if (*tb++ == 1) {
> > + while (ch0 < ch0_end) {
> > + const float v1 = *ch0;
> > + const float v2 = *ch1;
> > + *ch0++ = v1 - v2;
> > + *ch1++ = v1 + v2;
> > + }
> > + }else{
> > + while (ch0 < ch0_end) {
> > + *ch0++ *= 181.0 / 128;
> > + *ch1++ *= 181.0 / 128;
> > + }
> > + }
> > + ++sfb_offsets;
> > + }
> > + }else if (s->chgroup[i].transform) {
> > + float data[MAX_CHANNELS];
> > + const int num_channels = s->chgroup[i].num_channels;
> > + float** ch_data = s->chgroup[i].channel_data;
> > + float** ch_end = ch_data + num_channels;
> > + const int8_t* tb = s->chgroup[i].transform_band;
> > + int16_t* sfb;
> > +
> > + /** multichannel decorrelation */
> > + for (sfb = s->cur_sfb_offsets ;
> > + sfb < s->cur_sfb_offsets + s->num_bands;sfb++) {
> > + if (*tb++ == 1) {
> > + int y;
> > + /** multiply values with the decorrelation_matrix */
> > + for (y=sfb[0];y<FFMIN(sfb[1], s->subframe_len);y++)
> > { + const float* mat =
> > s->chgroup[i].decorrelation_matrix; + const float*
> > data_end= data + num_channels; + float* data_ptr=
> > data;
> > + float** ch;
> > +
> > + for (ch = ch_data;ch < ch_end; ch++)
> > + *data_ptr++ = (*ch)[y];
> > +
> > + for (ch = ch_data; ch < ch_end; ch++) {
> > + float sum = 0;
> > + data_ptr = data;
> > + while (data_ptr < data_end)
> > + sum += *data_ptr++ * *mat++;
> > +
> > + (*ch)[y] = sum;
> > + }
> > + }
> > + }
> > + }
> > + }
> > + }
> > +}
>
> isnt the if() a special case of te else ? if so this can be simplified
> and if its faster as is the special case could be limited to the inner
> loop
>
I don't understand what you mean here. Transform may either be 0, 1, 2. Of
course both 1 and 2 are larger than 0 but they stand for the transform type
so keeping all of them on the same level does not sound wrong to me even if
the 0 case does nothing. Maybe it should be changed to
else if(s->chgroup[i].transform == 2)?
> > + /** decode quantization step */
> > + quant = get_bits(&s->gb,6);
> > + if (quant & 0x20) {
> > + quant |= 0xFFFFFFC0u;
> > + sign = -1;
> > + }
>
> get_sbits()
>
Fixed.
> > + int end = s->cur_sfb_offsets[b+1];
> > + int sf = s->channel[c].max_scale_factor;
> > + float quant;
> > + if (end > s->subframe_len)
> > + end = s->subframe_len;
>
> FFMIN
Fixed.
> > +
> > + if (s->channel[c].transmit_sf)
> > + sf -= s->channel[c].scale_factors[b];
> > + else
> > + sf -= s->channel[c].resampled_scale_factors[b];
> > + sf *= -s->channel[c].scale_factor_step;
> > + sf += s->quant_step + s->channel[c].quant_step_modifier;
> >
> > + quant = pow(10.0,sf / 20.0);
>
> this can be simplified
I moved the condition out of the loop and moved quant_step to the per channel
context.
> > + while (start < end) {
> > + s->tmp[start] = s->channel[c].coeffs[start] * quant;
> > + ++start;
> > + }
>
> for(start= s->cur_sfb_offsets[b]; start < end; ++start)
>
Fixed.
> > + if (!s->packet_loss) {
> > + /** save the rest of the data so that it can be decoded
> > + with the next packet */
> > + wma_save_bits(s, &gb, wma_remaining_bits(s,&gb), 0);
> > + }
>
> and if a packet is lost the data is thrown away?
> is the data always useless or could it be of a correctly decodeable frame?
>
If packet_loss is set at this point we failed to decode the bitstream. When
this is the case we do not really know where we are. It is possible that a
(partial) valid frame can be found after the broken part but finding out
where it starts is not easy as frames have variable lengths and no common
startcode.
> you use *int8_t at some points and char here ...
> besides that i wonder why intXY_t instead of int in some cases
>
The intXY_t thing in the context came after it was more clear what the
upper/lower bounds of the variables are. I changed the chars in the table to
int8.
> > +static const uint32_t ff_wma3_scale_huffcodes[FF_WMA3_HUFF_SCALE_SIZE] =
> > { + 0x0E639, 0x0E6C2, 0x0E6C1, 0x0E6C0, 0x0E63F, 0x0E63E, 0x0E63D,
> > 0x0E63C, + 0x0E63B, 0x0E63A, 0x0E638, 0x0E637, 0x0E636, 0x0E635,
> > 0x0E634, 0x0E632, + 0x0E633, 0x0E620, 0x0737B, 0x0E610, 0x0E611,
> > 0x0E612, 0x0E613, 0x0E614, + 0x0E615, 0x0E616, 0x0E617, 0x0E618,
> > 0x0E619, 0x0E61A, 0x0E61B, 0x0E61C, + 0x0E61D, 0x0E61E, 0x0E61F,
> > 0x0E6C3, 0x0E621, 0x0E622, 0x0E623, 0x0E624, + 0x0E625, 0x0E626,
> > 0x0E627, 0x0E628, 0x0E629, 0x0E62A, 0x0E62B, 0x0E62C, + 0x0E62D,
> > 0x0E62E, 0x0E62F, 0x0E630, 0x0E631, 0x01CDF, 0x00E60, 0x00399, +
> > 0x000E7, 0x0001D, 0x00000, 0x00001, 0x00001, 0x00001, 0x00002, 0x00006, +
> > 0x00002, 0x00007, 0x00006, 0x0000F, 0x00038, 0x00072, 0x0039A,
> > 0x0E6C4, + 0x0E6C5, 0x0E6C6, 0x0E6C7, 0x0E6C8, 0x0E6C9, 0x0E6CA,
> > 0x0E6CB, 0x0E6CC, + 0x0E6CD, 0x0E6CE, 0x0E6CF, 0x0E6D0, 0x0E6D1,
> > 0x0E6D2, 0x0E6D3, 0x0E6D4, + 0x0E6D5, 0x0E6D6, 0x0E6D7, 0x0E6D8,
> > 0x0E6D9, 0x0E6DA, 0x0E6DB, 0x0E6DC, + 0x0E6DD, 0x0E6DE, 0x0E6DF,
> > 0x0E6E0, 0x0E6E1, 0x0E6E2, 0x0E6E3, 0x0E6E4, + 0x0E6E5, 0x0E6E6,
> > 0x0E6E7, 0x0E6E8, 0x0E6E9, 0x0E6EA, 0x0E6EB, 0x0E6EC, + 0x0E6ED,
> > 0x0E6EE, 0x0E6EF, 0x0E6F0, 0x0E6F1, 0x0E6F2, 0x0E6F3, 0x0E6F4, +
> > 0x0E6F5,
> > +};
>
> this one fits in 16 bits and its not the only such table
>
Fixed.
Besides that I also fixed following points from Vitors mails in the same
thread:
- set avctx->sample_fmt
- use const matrix offset table
- get rid of wma3.h
- removed encoding X in general.texi
- removed prefix from static functions and tables
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.
Float output can also be added after the code has been added to svn imho.
New patch attached.
Btw. should the code be renamed to wmaprodec.c and wmaprodata.h?
wma3 includes the voice and lossless codecs so wma3dec.c may not be the best
name for the pro decoder.
Regards
Sascha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wmapro_try2.patch
Type: text/x-diff
Size: 98439 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090620/b11a07d7/attachment.patch>
More information about the ffmpeg-devel
mailing list