[FFmpeg-devel] [PATCH] wmapro decoder

Sascha Sommer saschasommer
Mon Aug 31 21:55:25 CEST 2009


Hi,

> >
> > +    uint32_t         frame_num;                     ///< current frame
> > number
>
> unused
>

It is used for error output and some debugging messages. Does it waste too 
many resources?


> > +/**
> > + *@brief Decode the subframe length.
> > + *@param s context
> > + *@param offset sample offset in the frame
> > + *@return decoded subframe length on success, 0 in case of an error
> > + */
> > +static int decode_subframe_length(WMA3DecodeContext *s, int offset)
> > +{
> > +    int log2_subframe_len = 0;
> > +    int subframe_len;
> > +
> > +    /** no need to read from the bitstream when only one length is
> > possible */ +    if (offset == s->samples_per_frame -
> > s->min_samples_per_subframe) +        return s->min_samples_per_subframe;
> > +
> > +    /** 1 bit indicates if the subframe is of maximum length */
> > +    if (s->max_subframe_len_bit) {
> > +        if (get_bits1(&s->gb))
> > +            log2_subframe_len = 1 + get_bits(&s->gb,
> > s->subframe_len_bits-1); +    } else
> > +        log2_subframe_len = get_bits(&s->gb, s->subframe_len_bits);
> > +
> >
> > +    subframe_len = s->samples_per_frame / (1 << log2_subframe_len);
>
> subframe_len = s->samples_per_frame >> log2_subframe_len;
>
> and i would guess the names are not good because one would expect
> 1<<log2_X = X
>

Fixed.

> > +
> > +    /** sanity check the length */
> > +    if (subframe_len < s->min_samples_per_subframe
> > +              || subframe_len > s->samples_per_frame) {
> > +        av_log(s->avctx, AV_LOG_ERROR, "broken frame: subframe_len
> > %i\n", +               subframe_len);
> > +        return 0;
>
> within ffmpeg errors are negative values
>

Changed.

> > +static int decode_tilehdr(WMA3DecodeContext *s)
> > +{
> >
> > +    uint16_t num_samples[WMAPRO_MAX_CHANNELS];
> > +    uint8_t  contains_subframe[WMAPRO_MAX_CHANNELS];
> > +    int channels_for_cur_subframe = s->num_channels;
> > +    int fixed_channel_layout = 0;
> > +    int min_channel_len = 0;
>
> these could benefit from some doxy
>

Added.


> >
> > +            /** save transmitted scale factors so that they can be
> > reused for +                the next subframe */
> > +            memcpy(s->channel[c].saved_scale_factors,
> > +                   s->channel[c].scale_factors, s->num_bands *
> > +                   sizeof(*s->channel[c].saved_scale_factors));
>
> what happens with s->channel[c].scale_factors so that it cant just be used?
>

The saved scale factors might be resampled for several subframes before some 
new values are transmitted and saved_scale_factors is updated.
If the resampled scale factors would be used for the resampling, the output 
would change. Therefore two buffers are needed.

> > +        memset(s->channel[c].coeffs, 0,
> > +               sizeof(*s->channel[c].coeffs) * subframe_len);
>
> IMHO it would be better to do this in a else clause of the
> if(transmit_coefs) and before the rle decoding is started
>

Changed then.

> > +    /** interleave samples and write them to the output buffer */
> > +    for (i = 0; i < s->num_channels; i++) {
> > +        float* ptr;
> > +        int incr = s->num_channels;
> > +        float* iptr = s->channel[i].out;
> > +        int x;
> > +
> > +        ptr = s->samples + i;
> > +
> > +        for (x = 0; x < s->samples_per_frame; x++) {
> > +            *ptr = av_clipf(*iptr++, -1.0, 32767.0 / 32768.0);
> > +            ptr += incr;
> > +        }
> > +
> > +        /** reuse second half of the IMDCT output for the next frame */
> > +        memcpy(&s->channel[i].out[0],
> > +               &s->channel[i].out[s->samples_per_frame],
> > +               s->samples_per_frame * sizeof(*s->channel[i].out) >> 1);
> > +    }
>
> did you try to not do several passes over the output buffer?
> (maybe its faster ...)
>

This takes twice as long.

> > +/**
> > + *@brief WMA9 decoder
> > + */
> > +AVCodec wmapro_decoder = {
> > +    "wmapro",
> > +    CODEC_TYPE_AUDIO,
> > +    CODEC_ID_WMAPRO,
> > +    sizeof(WMA3DecodeContext),
>
> WMA9 decoder, WMAPRO, WMA3DecodeContext
> isnt that naming a little inconsistent?
>

Everything renamed to wmapro.

New patch attached...

Regards

Sascha

-------------- next part --------------
A non-text attachment was scrubbed...
Name: wmapro_try7.patch
Type: text/x-diff
Size: 55209 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090831/f4d647c8/attachment.patch>



More information about the ffmpeg-devel mailing list