[FFmpeg-devel] [PATCH] ALS decoder

Michael Niedermayer michaelni
Sat Aug 22 15:56:55 CEST 2009


On Fri, Aug 21, 2009 at 01:40:56AM +0200, Thilo Borgmann wrote:
> Hi,
> 
> >> +    uint32_t     header_size;            ///< header size of original audio file in bytes, provided for debugging
> >> +    uint32_t     trailer_size;           ///< Trailer size of original audio file in bytes, provided for debugging
> >> +    uint32_t     crc;                    ///< 32-bit CCITT-32 CRC checksum
> >> +} ALSSpecificConfig;
> > 
> > Please try not to make *every* line exceed 80 columns.  You could
> > easily shorten them here by reducing the amount of whitespace between
> > the type and name and before the comment.
> > 
> >> +    int64_t           *prev_raw_samples; ///< contains unshifted raw samples from the previous block
> >> +    int64_t           **raw_samples;     ///< decoded raw samples for each channel
> >> +    int64_t           *raw_buffer;       ///< contains all decoded raw samples including carryover samples
> >> +} ALSDecContext;
> > 
> > Ditto.
> ok, reduced whitespaces.
> 
> 
> > 
> >> +/** Computes ceil(log2(x)) using av_log2.
> >> + */
> >> +static inline int ceil_log2(int x) {
> >> +    return x > 0 ? av_log2((x - 1) << 1) : 0;
> >> +}
> > 
> > Little functions like this are likely to be needed again some time.
> > They should be placed in a common location.
> Moved to libavutil/common.h. Please review my bad doxy wording.
> 
> 
> > 
> >> +/** Reads an ALSSpecificConfig from a buffer into the output struct.
> >> + */
> >> +static av_cold int read_specific_config(ALSDecContext *ctx)
> >> +{
> >> +    GetBitContext gb;
> >> +    uint64_t ht_size;
> >> +    int i, config_offset;
> >> +    MPEG4AudioConfig m4ac;
> >> +    ALSSpecificConfig *sconf = &ctx->sconf;
> >> +    const uint8_t *buffer    = ctx->avctx->extradata;
> >> +    int buffer_size          = ctx->avctx->extradata_size;
> >> +
> >> +    init_get_bits(&gb, buffer, buffer_size * 8);
> >> +
> >> +    config_offset = ff_mpeg4audio_get_config(&m4ac, buffer, buffer_size);
> >> +
> >> +    if (config_offset < 0)
> >> +        return -1;
> >> +
> >> +    skip_bits_long(&gb, config_offset);
> >> +    buffer_size -= config_offset >> 3;
> >> +
> >> +    if (buffer_size < 22)
> >> +        return -1;
> >> +
> >> +    // read the fixed items
> >> +    sconf->als_id               = get_bits_long(&gb, 32);
> >> +    sconf->samp_freq            = m4ac.sample_rate;
> >> +    skip_bits_long(&gb, 32);
> > 
> > Maybe comment what is being skipped here and elsewhere.
> Ok, I thought it is obvious. Done.
> 
> 
> > 
> >> +    sconf->samples              = get_bits_long(&gb, 32);
> >> +    sconf->channels             = m4ac.channels;
> >> +    skip_bits(&gb, 16);
> >> +    sconf->file_type            = get_bits(&gb, 3);
> >> +    sconf->resolution           = get_bits(&gb, 3);
> >> +    sconf->floating             = get_bits1(&gb);
> >> +    sconf->msb_first            = get_bits1(&gb);
> >> +    sconf->frame_length         = get_bits(&gb, 16) + 1;
> >> +    sconf->random_access        = get_bits(&gb, 8);
> >> +    sconf->ra_flag              = get_bits(&gb, 2);
> >> +    sconf->adapt_order          = get_bits1(&gb);
> >> +    sconf->coef_table           = get_bits(&gb, 2);
> >> +    sconf->long_term_prediction = get_bits1(&gb);
> >> +    sconf->max_order            = get_bits(&gb, 10);
> >> +    sconf->block_switching      = get_bits(&gb, 2);
> >> +    sconf->bgmc_mode            = get_bits1(&gb);
> >> +    sconf->sb_part              = get_bits1(&gb);
> >> +    sconf->joint_stereo         = get_bits1(&gb);
> >> +    sconf->mc_coding            = get_bits1(&gb);
> >> +    sconf->chan_config          = get_bits1(&gb);
> >> +    sconf->chan_sort            = get_bits1(&gb);
> >> +    sconf->crc_enabled          = get_bits1(&gb);
> >> +    sconf->rlslms               = get_bits1(&gb);
> >> +    skip_bits(&gb, 5);                                      // skip 5 reserved bits
> > 
> > Please lose some of that whitespace.  Compliments on the alignment
> > though.  Diego will love you.
> I did this above, but here, not even the 80-characters reason exist.
> Why give up readability?
> 
> 
> 
> >> +
> >> +        for (i = 0; i < sconf->channels; i++) {
> >> +            sconf->chan_pos[i] = get_bits(&gb, chan_pos_bits);
> >> +        }
> > 
> > We usually omit {} for single-line for/if/while statements...
> I usually do that, too... done.
> 
> 
> > 
> >> +        align_get_bits(&gb);
> >> +        buffer_size -= bytes_needed;
> >> +        // TODO: use this to actually do channel sorting
> >> +    } else {
> >> +        sconf->chan_sort = 0;
> >> +    }
> > 
> > ... except for one-line else clauses after a multiline if.
> Yes.
> 
> 
> 
> >> +/** Reads and decodes a Rice codeword.
> >> + */
> >> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
> >> +{
> >> +    int64_t value = 0;
> >> +    int64_t q = 0;
> >> +    int     max = gb->size_in_bits - get_bits_count(gb) - k;
> >> +
> >> +    if (!k) {
> >> +        q = get_unary(gb, 0, max);
> >> +        return (q & 1) ? -((q + 1) >> 1) : ((q + 1) >> 1);
> >> +    } else if (k == 1) {
> >> +        q = get_unary(gb, 0, max);
> >> +        return get_bits1(gb) ? q : -(q + 1);
> >> +    } else {
> >> +        unsigned int r, sub_sign;
> >> +
> >> +        q         = get_unary(gb, 0, max);
> >> +        sub_sign  = get_bits1(gb);
> >> +        r         = get_bits_long(gb, k - 1);
> >> +
> >> +        value = (q << (k - 1)) + r;
> >> +
> >> +        return sub_sign ? value : -(value + 1);
> >> +    }
> >> +}
> > 
> > This function looks like it was designed specifically to make gcc fail:
> > 
> > 1.  64-bit variables used unnecessarily.
> > 2.  GCC hates complex conditionals.
> > 3.  Compilers in general are bad at bit-hacks.
> > 
> > It seems to me that int is enough for 'q' in the first two cases.
> > get_unary() returns int, so we start with at most that many bits.  To
> > avoid overflow in q+1, do this instead:
> > 
> >     int r = q & 1;
> >     return r ? -((q >> 1) + r) : ((q >> 1) + r);
> > 
> > Furthermore, gcc does stupid things with that code even with plain int
> > variables.  Rewriting in a few more steps helps massively:
> > 
> >     int r = q & 1;
> >     q = (q >> 1) + r;
> >     return r ? -q : q;
> > 
> > This reduces the number of instructions on ARM from 6 to 4 (from 4 to
> > 3 with armcc).  On MIPS it goes from 9 with a branch to 5 branch-free.
> > 
> > Finally, -(v + 1) is equivalent to ~v on two's complement machines,
> > which we have previously agreed to assume we have.  Hence we should
> > write the return values in the second and third cases like this:
> > 
> >     return sign ? value : ~value;
> > 
> > This prevents 32-bit overflow in the k==1 case and gives better code
> > with several compilers in the final case, where we must resort to
> > 64-bit maths.
> Tricky. Works like a charm. Done.
> 
> 
> > 
> >> +/** Converts PARCOR coefficient k to direct filter coefficient.
> >> + */
> >> +static void parcor_to_lpc(unsigned int k, int64_t *par, int64_t *cof)
> >> +{
> >> +    int i;
> >> +    int64_t tmp1, tmp2;
> >> +
> >> +    for (i = 0; i < (k+1) >> 1; i++) {
> >> +        tmp1 = cof[    i    ] + ((par[k] * cof[k - i - 1] + (1 << 19)) >> 20);
> >> +        tmp2 = cof[k - i - 1] + ((par[k] * cof[    i    ] + (1 << 19)) >> 20);
> >> +        cof[k - i - 1] = tmp2;
> >> +        cof[    i    ] = tmp1;
> >> +    }
> >> +
> >> +    cof[k] = par[k];
> >> +}
> > 
> > This is again a perfect trap for gcc.  64-bit maths in tight loops is
> > something it is *really* bad at.  Is there no way to avoid 64-bit
> > elements here?  Is perhaps the range usually smaller, so we could
> > choose a 32-bit version most of the time and fall back on 64-bit only
> > when required?  Regardless of compiler, 64-bit multiplication is
> > expensive on any 32-bit machine.
> I think the usual case uses 64 bit here, but I'm not sure. Maybe Justin
> can give a comment on this?
> 
> 
> 
> >> +/** Reads the block data.
> >> + */
> >> +static int read_block_data(ALSDecContext *ctx, unsigned int ra_block,
> >> +                            int64_t *raw_samples, unsigned int block_length,
> >> +                            unsigned int *js_blocks, int64_t *raw_other)
> >> +{
> >> ...
> > The block nesting is very deep here.  Perhaps splitting this large
> > function into several smaller ones would help readability.  Even gcc
> > should manage to inline them.
> I agree, done.
> 
> 
> > 
> >> +        if (sconf->long_term_prediction) {
> >> +            // TODO: LTP mode
> >> +        }
> >> +
> >> +        start = 0;
> >> ...
> >> +        } else {
> >> +            int store_prev_samples = (*js_blocks && raw_other) || shift_lsbs;
> >> +
> >> +            // store previous smaples in case that they have to be altered
> >> +            if (store_prev_samples)
> >> +                memcpy(ctx->prev_raw_samples, raw_samples - sconf->max_order,
> >> +                       sizeof(int64_t) * sconf->max_order);
> >> +
> >> +            // reconstruct difference signal for prediction (joint-stereo)
> >> +            if (*js_blocks && raw_other) {
> >> +                int i;
> >> +                if (raw_other > raw_samples) {          // D = R - L
> >> +                    for (i = -1; i >= -sconf->max_order; i--)
> >> +                        raw_samples[i] = raw_other[i] - raw_samples[i];
> >> +                } else {                                // D = R - L
> > 
> > Are those two comments meant to be the same?
> Yes but semantically different. This is meant to be that way.
> It shows, which channel is represented by which variable.
> 
> 
> >> ...
> >> +
> >> +    if (shift_lsbs) {
> >> +        for (k = 0; k < block_length; k++)
> >> +            raw_samples[k] <<= shift_lsbs;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> > 
> > Many of the loops above look like they could be easily simdified.  I
> > don't know how much time is spent in each, so I haven't thought about
> > it in detail.  Breaking the function apart would also help in finding
> > where most time is spent.
> I don't think so, at least in terms of merging several of them.
> Otherwise or if you thought about another optimization, please give an
> example.
> 
> 
> > 
> >> +
> >> +/** Reads the frame data.
> >> + */
> >> +static int read_frame_data(ALSDecContext *ctx, unsigned int ra_frame)
> >> +{
> >> ...
> > 
> > Again, very deep nesting makes code hard to read.  Consider making
> > some blocks separate functions.
> Done.
> 
> 
> >> ...
> > 
> > Maybe you already tried to fix it, but there's a scary number of
> > memcpy/memmove calls above.
> All of them are really necessary.
> 
> 
> 
> >> +
> >> +    // transform decoded frame into output format
> >> +    #define INTERLEAVE_OUTPUT(bps)                                                 \
> > 
> > That's an awful lot of whitespace.  Please cut it back so the lines
> > fit in 80 columns, which will be easily possible when you address my
> > next comment.
> > 
> >> +    {                                                                              \
> >> +        int##bps##_t *dest = (int##bps##_t*) data;                                 \
> >> +        shift = bps - ctx->avctx->bits_per_raw_sample;                             \
> >> +        for (sample = 0; sample < ctx->cur_frame_length; sample++) {               \
> >> +            for (c = 0; c < sconf->channels; c++) {                                \
> >> +                *(dest++) = (int##bps##_t) (ctx->raw_samples[c][sample] << shift); \
> > 
> > Useless parens: *dest++ is good.  Useless cast => more useless parens.
> Yes, done.
> 
> 
> 
> >> +    // allocate previous raw sample buffer
> >> +    if (!(ctx->prev_raw_samples = av_malloc(sizeof(int64_t) * sconf->max_order))) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> >> +        decode_end(avctx);
> >> +        return AVERROR(ENOMEM);
> >> +    }
> >> +
> >> +    // allocate raw and carried sample buffer
> >> +    if (!(ctx->raw_buffer = av_mallocz(sizeof(int64_t) *
> >> +                                       avctx->channels * channel_size))) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> >> +        decode_end(avctx);
> >> +        return AVERROR(ENOMEM);
> >> +    }
> >> +
> >> +    // allocate raw sample array buffer
> >> +    if (!(ctx->raw_samples = av_malloc(sizeof(int64_t*) * avctx->channels))) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer array failed.\n");
> >> +        decode_end(avctx);
> >> +        return AVERROR(ENOMEM);
> >> +    }
> > 
> > This looks very repetitive...
> Thus.... it is good/bad/crazy?
> The log message depends on the buffer allocated, the size of the buffer
> is never the same...
> 
> 
> > 
> >> +    // allocate raw and carried samples buffers
> >> +    ctx->raw_samples[0] = ctx->raw_buffer + sconf->max_order;
> >> +    for (c = 1; c < avctx->channels; c++) {
> >> ...
> >> +
> >> Index: libavcodec/als_data.h
> >> ===================================================================
> >> --- libavcodec/als_data.h	(revision 0)
> >> +++ libavcodec/als_data.h	(revision 0)
> >> @@ -0,0 +1,147 @@
> >> +/*
> >> + * ALS header file for common data
> >> + * Copyright (c) 2009 Thilo Borgmann <thilo.borgmann _at_ googlemail.com>
> >> ...
> >> +#ifndef AVCODEC_ALS_DATA_H
> >> +#define AVCODEC_ALS_DATA_H
> >> +
> >> +
> >> +#include <stdint.h>
> >> +
> >> +/** Rice parameters and corresponding index offsets for decoding the
> >> + *  indices of scaled PARCOR values. The table choosen is set globally
> >> + *  by the encoder and stored in ALSSpecificConfig.
> >> + */
> >> +int8_t parcor_rice_table[3][20][2] = {
> >> +                        {
> >> +                        {-52, 4},
> > 
> > WTF happened to the indentation here?
> > 
> >> +
> >> +/** Scaled PARCOR values used for the first two PARCOR coefficients.
> >> + *  To be indexed by the Rice coded indices.
> >> + *  Generated by: parcor_scaled_values[i] = 32 + ((i * (i+1)) << 7) - (1 << 20)
> >> + */
> >> +int32_t parcor_scaled_values[] = {-1048544, -1048288, -1047776, -1047008,
> >> +                                  -1045984, -1044704, -1043168, -1041376,
> > 
> > And here.
> Would you like block- or function-like indents?
> No kidding - I ask that before it goes back and forth and I don't care
> which one...
> (Or are there table-like indention rules I've missed?)
> 
> Revision 4 attached.
[...]
> +
> +typedef struct {
> +    uint32_t als_id;          ///< ALS identifier

> +    uint32_t samp_freq;       ///< sampling frequency in Hz

i suspect that is redundant with AVCodecContext.sample_rate


> +    uint32_t samples;         ///< number of samples (per channel), 0xFFFFFFFF if unknown

doesnt seem to be needed to be stored in the context


> +    int channels;             ///< number of channels

> +    int file_type;            ///< not used, provided for debugging

if its just for debugging it can also be a local variable


> +    int resolution;           ///< 000 = 8-bit; 001 = 16-bit; 010 = 24-bit; 011 = 32-bit
> +    int floating;             ///< 1 = IEEE 32-bit floating-point, 0 = integer
> +    int msb_first;            ///< original byte order of the input audio data
> +    int frame_length;         ///< frame Length

> +    int random_access;        ///< distance between RA frames (in frames, 0...255)

thats the keyframe distance or gop length i assume, it should be named
accordingly


> +    enum RA_Flag ra_flag;     ///< indicates where the size of ra units is stored
> +    int adapt_order;          ///< adaptive order: 1 = on, 0 = off
> +    int coef_table;           ///< table index of Rice code parameters
> +    int long_term_prediction; ///< long term prediction (LTP): 1 = on, 0 = off
> +    int max_order;            ///< maximum prediction order (0..1023)
> +    int block_switching;      ///< number of block switching levels

> +    int bgmc_mode;            ///< BGMC Mode: 1 = on, 0 = off (Rice coding only)

bgmc?


> +    int sb_part;              ///< sub-block partition
> +    int joint_stereo;         ///< joint Stereo: 1 = on, 0 = off
> +    int mc_coding;            ///< extended inter-channel coding: 1 = on, 0 = off
> +    int chan_config;          ///< indicates that a chan_config_info field is present
> +    int chan_sort;            ///< channel rearrangement: 1 = on, 0 = off
> +    int crc_enabled;          ///< indicates that the crc field is present

> +    int rlslms;               ///< use RLS-LMS predictor: 1 = on, 0 = off

RLSMLS?
no i did not read the spec ...


> +    int aux_data_enabled;     ///< indicates that auxiliary data is present

> +    int chan_config_info;     ///< mapping of channels to loudspeaker locations

never read


> +    int *chan_pos;            ///< original channel positions

same


> +    uint32_t header_size;     ///< header size of original audio file in bytes, provided for debugging
> +    uint32_t trailer_size;    ///< Trailer size of original audio file in bytes, provided for debugging

> +    uint32_t crc;             ///< 32-bit CCITT-32 CRC checksum

and that one as well

(that is variables that are never read or never written can be removed
 and things that can be local variables should be instead of being in
 the context)


[...]
> +    // allocate quantized parcor coefficient buffer
> +    if (!(ctx->quant_cof = av_malloc(sizeof(int64_t) * sconf->max_order))) {
> +        av_log(ctx->avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    // allocate LPC coefficients
> +    if (!(ctx->lpc_cof = av_malloc(sizeof(int64_t) * sconf->max_order))) {
> +        av_log(ctx->avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> +        return AVERROR(ENOMEM);
> +    }

duplicate and factorizeable code


> +
> +    // calculate total number of frames to decode if possible
> +    if (sconf->samples != 0xFFFFFFFF) {
> +        ctx->num_frames        = ((sconf->samples - 1) / sconf->frame_length) + 1;
> +        ctx->last_frame_length = sconf->samples % ctx->sconf.frame_length;
> +        if (!ctx->last_frame_length) {
> +            ctx->last_frame_length = sconf->frame_length;
> +        }
> +    } else {
> +        ctx->num_frames        = 0;
> +        ctx->last_frame_length = 0;
> +    }
> +
> +
> +    // read channel config
> +    if (sconf->chan_config) {
> +        if (buffer_size < 2)
> +            return -1;
> +
> +        sconf->chan_config_info = get_bits(&gb, 16);
> +        buffer_size -= 2;
> +        // TODO: use this to set avctx->channel_layout
> +    }
> +
> +
> +    // read channel sorting
> +    if (sconf->chan_sort && sconf->channels > 1) {
> +        int chan_pos_bits = ceil_log2(sconf->channels);
> +        int bytes_needed  = (sconf->channels * chan_pos_bits + 7) >> 3;
> +        if (buffer_size < bytes_needed)
> +            return -1;
> +

> +        if(!(sconf->chan_pos = av_malloc(sconf->channels * sizeof(int))))
> +            return -1;

ENOMEM


> +
> +        for (i = 0; i < sconf->channels; i++)
> +            sconf->chan_pos[i] = get_bits(&gb, chan_pos_bits);
> +
> +        align_get_bits(&gb);
> +        buffer_size -= bytes_needed;
> +        // TODO: use this to actually do channel sorting
> +    } else {
> +        sconf->chan_sort = 0;
> +    }
> +
> +
> +    // read fixed header and trailer sizes, if size = 0xFFFFFFFF then there is no data field!
> +    if (buffer_size < 8)
> +        return -1;
> +
> +    sconf->header_size  = get_bits_long(&gb, 32);
> +    sconf->trailer_size = get_bits_long(&gb, 32);
> +    if (sconf->header_size  == 0xFFFFFFFF)
> +        sconf->header_size  = 0;
> +    if (sconf->trailer_size == 0xFFFFFFFF)
> +        sconf->trailer_size = 0;
> +

> +    ht_size = sconf->header_size + sconf->trailer_size;

overflow


> +
> +    buffer_size -= 8;
> +
> +
> +    // skip the header and trailer data
> +    if (buffer_size < ht_size)
> +        return -1;
> +
> +    ht_size <<= 3;
> +
> +    while (ht_size > 0) {
> +        int len = FFMIN(ht_size, INT32_MAX);
> +        skip_bits_long(&gb, len);
> +        ht_size -= len;
> +    }

i dont think theres sense in supporting skiping of more than 500mb
as valid packets should not have such sizes


[...]
> +/** Parses the bs_info item to extract the block partitioning.
> + */

without having read the spec, that says nothing to me ...
can this be clarified in 1 sentance or is the reader required to have read
the spec?


[...]
> +/** Reads and decodes a Rice codeword.
> + */
> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
> +{
> +    int     max = gb->size_in_bits - get_bits_count(gb) - k;
> +
> +    if (!k) {
> +        int r,q;
> +        q = get_unary(gb, 0, max);
> +        r = q & 1;
> +        q = (q >> 1) + r;
> +        return r ? -q : q;
> +    } else if (k == 1) {
> +        int q = get_unary(gb, 0, max);
> +        return get_bits1(gb) ? q : ~q;
> +    } else {
> +        int64_t value, q;
> +        unsigned int r, sub_sign;
> +
> +        q         = get_unary(gb, 0, max);
> +        sub_sign  = get_bits1(gb);
> +        r         = get_bits_long(gb, k - 1);
> +
> +        value = (q << (k - 1)) + r;
> +
> +        return sub_sign ? value : ~value;
> +    }
> +}

theres some obvious factorizeable code in there


> +
> +
> +/** Converts PARCOR coefficient k to direct filter coefficient.
> + */
> +static void parcor_to_lpc(unsigned int k, int64_t *par, int64_t *cof)
> +{
> +    int i;
> +    int64_t tmp1, tmp2;
> +
> +    for (i = 0; i < (k+1) >> 1; i++) {
> +        tmp1 = cof[    i    ] + ((par[k] * cof[k - i - 1] + (1 << 19)) >> 20);
> +        tmp2 = cof[k - i - 1] + ((par[k] * cof[    i    ] + (1 << 19)) >> 20);
> +        cof[k - i - 1] = tmp2;
> +        cof[    i    ] = tmp1;
> +    }
> +
> +    cof[k] = par[k];
> +}

doesnt look entirely unfamiliar, dont some of our other audio codecs have
something that can be used? (no i dont know which, i would have to RTFS too)


> +
> +
> +/** Reformat block sizes from log2 format to direct form. Also assure that the
> + *  block sizes of the last frame correspond to the actual number of samples.
> + */
> +static void reconstruct_block_sizes(ALSDecContext *ctx, uint32_t *div_blocks)
> +{
> +    unsigned int b;
> +
> +    // The last frame may have an overdetermined block structure given in
> +    // the bitstream. In that case the defined block structure would need
> +    // more samples than available to be consistent.
> +    // The block structure is actually used but the block sizes are adapted
> +    // to fit the actual number of available samples.
> +    // Example: 5 samples, 2nd level block sizes: 2 2 2 2.
> +    // This results in the actual block sizes:    2 2 1 0.

> +    // This is not specified in 14496-3 but actually done by the reference
> +    // codec RM22 revision 2.
> +    // This appears to happen in case of an odd number of samples in the last
> +    // frame which is actually not allowed by the block length switching part
> +    // of 14496-3.
> +    // The ALS conformance files feature an odd number of samples in the last
> +    // frame.

another high quality audio spec ...


[...]
> +/** Reads the block data for a constant block
> + */
> +static void read_const_block(ALSDecContext *ctx, int64_t *raw_samples,
> +                             unsigned int block_length, unsigned int *js_blocks)
> +{
> +    ALSSpecificConfig *sconf = &ctx->sconf;
> +    AVCodecContext *avctx    = ctx->avctx;
> +    GetBitContext *gb        = &ctx->gb;
> +    int32_t const_val        = 0;
> +    unsigned int const_block, k;
> +
> +    const_block  = get_bits1(gb);    // 1 = constant value, 0 = zero block (silence)
> +    *js_blocks   = get_bits1(gb);
> +

> +    // skip 5 reserved bits
> +    skip_bits(gb, 5);

if these have a required value like 0 that should be checked, always usefull
to know early if something unexpected turns up


> +
> +    if (const_block) {
> +        unsigned int const_val_bits;
> +
> +        if (sconf->resolution == 2 || sconf->floating)
> +            const_val_bits = 24;
> +        else
> +            const_val_bits = avctx->bits_per_raw_sample;

why would const_val_bits != avctx->bits_per_raw_sample ?


> +
> +        const_val = get_sbits_long(gb, const_val_bits);
> +    }
> +
> +    // write raw samples into buffer
> +    for (k = 0; k < block_length; k++)
> +        raw_samples[k] = const_val;
> +}
> +
> +
> +/** Reads the block data for a non-constant block
> + */
> +static int read_var_block(ALSDecContext *ctx, unsigned int ra_block,
> +                          int64_t *raw_samples, unsigned int block_length,
> +                          unsigned int *js_blocks, int64_t *raw_other,
> +                          unsigned int *shift_lsbs)
> +{
> +    ALSSpecificConfig *sconf = &ctx->sconf;
> +    AVCodecContext *avctx    = ctx->avctx;
> +    GetBitContext *gb        = &ctx->gb;
> +    unsigned int k;
> +    unsigned int s[8];
> +    unsigned int sub_blocks, sb_length;
> +    unsigned int opt_order  = 1;
> +    int64_t      *quant_cof = ctx->quant_cof;
> +    int64_t      *lpc_cof   = ctx->lpc_cof;
> +    unsigned int start      = 0;
> +    int          sb, smp;
> +    int64_t      y;
> +
> +    *js_blocks  = get_bits1(gb);
> +

> +    // determine the number of sub blocks for entropy decoding
> +    if (!sconf->bgmc_mode && !sconf->sb_part)
> +        sub_blocks = 1;
> +    else if (sconf->bgmc_mode && sconf->sb_part)
> +        sub_blocks = 1 << get_bits(gb, 2);
> +    else
> +        sub_blocks = get_bits1(gb) ? 4 : 1;

id write 1 << (2*get_bits1(gb))
because its more simlar to the others


> +
> +    // do not continue in case of a damaged stream since
> +    // block_length must be evenly divisible by sub_blocks
> +    if (block_length % sub_blocks) {
> +        av_log(avctx, AV_LOG_WARNING,
> +               "Block length is not evenly divisible by the number of sub blocks.\n");
> +        return -1;
> +    }
> +
> +    sb_length = block_length / sub_blocks;
> +
> +
> +    if (!sconf->bgmc_mode) {
> +        s[0] = get_bits(gb, (sconf->resolution > 1) ? 5 : 4);
> +        for (k = 1; k < sub_blocks; k++)
> +            s[k] = s[k - 1] + decode_rice(gb, 0);
> +    } else {
> +        // TODO: BGMC mode
> +    }

if(x)
else

is imho clearer than

if(!x)
else


> +
> +    if (get_bits1(gb)) {
> +        *shift_lsbs = get_bits(gb, 4) + 1;
> +    }
> +
> +
> +    if (!sconf->rlslms) {
> +        int64_t quant_index;
> +
> +        if (sconf->adapt_order) {
> +            int opt_order_length =
> +                    FFMIN(
> +                    ceil_log2(sconf->max_order+1),
> +                    FFMAX(ceil_log2((block_length >> 3) - 1), 1)
> +                    );
> +            opt_order = get_bits(gb, opt_order_length);
> +        } else {
> +            opt_order = sconf->max_order;
> +        }
> +
> +        if (opt_order) {
> +            if (sconf->coef_table == 3) {
> +                // read coefficient 0

> +                quant_index = get_bits(gb, 7) - 64;
> +                quant_cof[0] = parcor_scaled_values[quant_index + 64];

-64 + 64 ?


> +
> +                // read coefficient 1
> +                quant_index = get_bits(gb, 7) - 64;
> +                quant_cof[1] = -parcor_scaled_values[quant_index + 64];

same


> +
> +                // read coefficients 2 to opt_order
> +                for (k = 2; k < opt_order; k++) {
> +                    quant_index = get_bits(gb, 7) - 64;
> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);

the +- can be combined


> +                }
> +            } else {
> +                int offset, rice_param, k_max;
> +
> +                // read coefficient 0
> +                offset       = parcor_rice_table[sconf->coef_table][0][0];
> +                rice_param   = parcor_rice_table[sconf->coef_table][0][1];
> +                quant_index  = decode_rice(gb, rice_param) + offset;
> +                quant_cof[0] = parcor_scaled_values[quant_index + 64];
> +
> +                // read coefficient 1
> +                offset       = parcor_rice_table[sconf->coef_table][1][0];
> +                rice_param   = parcor_rice_table[sconf->coef_table][1][1];
> +                quant_index  = decode_rice(gb, rice_param) + offset;
> +                quant_cof[1] = -parcor_scaled_values[quant_index + 64];
> +
> +                // read coefficients 2 to 19
> +                k_max = FFMIN(20, opt_order);
> +                for (k = 2; k < k_max; k++) {
> +                    offset       = parcor_rice_table[sconf->coef_table][k][0];
> +                    rice_param   = parcor_rice_table[sconf->coef_table][k][1];
> +                    quant_index  = decode_rice(gb, rice_param) + offset;
> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);
> +                }
> +

> +                // read coefficients 20 to 126
> +                k_max = FFMIN(127, opt_order);
> +                for (k = 20; k < k_max; k++) {

k=20 is redundant


> +                    offset       = k & 1;
> +                    rice_param   = 2;
> +                    quant_index  = decode_rice(gb, rice_param) + offset;
> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);
> +                }
> +
> +                // read coefficients 127 to opt_order
> +                for (k = 127; k < opt_order; k++) {

so is k=127


> +                    offset       = 0;
> +                    rice_param   = 1;
> +                    quant_index  = decode_rice(gb, rice_param) + offset;
> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);
> +                }
> +            }
> +        }
> +    }
> +
> +    if (sconf->long_term_prediction) {
> +        // TODO: LTP mode
> +    }
> +
> +    start = 0;
> +
> +    // read first value and residuals in case of a random access block
> +    if (ra_block) {
> +        if (opt_order)
> +            raw_samples[0] = decode_rice(gb, avctx->bits_per_raw_sample - 4);
> +        if (opt_order > 1)
> +            raw_samples[1] = decode_rice(gb, s[0] + 3);
> +        if (opt_order > 2)
> +            raw_samples[2] = decode_rice(gb, s[0] + 1);
> +
> +        start = FFMIN(opt_order, 3);
> +    } else {
> +        for (k = 0; k < opt_order; k++)
> +            parcor_to_lpc(k, quant_cof, lpc_cof);
> +    }
> +
> +    // read all residuals
> +    if (sconf->bgmc_mode) {
> +        // TODO: BGMC mode
> +    } else {
> +        int64_t *current_res = raw_samples;
> +
> +        for (sb = 0; sb < sub_blocks; sb++) {
> +            for (k = start; k < sb_length; k++) {
> +                current_res[k] = decode_rice(gb, s[sb]);
> +            }
> +            current_res += sb_length;
> +            start = 0;
> +        }
> +     }
> +
> +    // reconstruct all samples from residuals
> +    if (ra_block) {
> +        unsigned int progressive = FFMIN(block_length, opt_order);
> +
> +        for (smp = 0; smp < block_length; smp++) {
> +            unsigned int max, dequant;
> +
> +            dequant = smp < progressive;
> +            max     = dequant ? smp : progressive;
> +
> +            y = 1 << 19;
> +
> +            for (sb = 0; sb < max; sb++)
> +                y += lpc_cof[sb] * raw_samples[smp - (sb + 1)];
> +
> +            raw_samples[smp] -= y >> 20;
> +            if (dequant)
> +                parcor_to_lpc(smp, quant_cof, lpc_cof);
> +        }
> +    } else {
> +        int store_prev_samples = (*js_blocks && raw_other) || *shift_lsbs;
> +
> +        // store previous smaples in case that they have to be altered
> +        if (store_prev_samples)

> +            memcpy(ctx->prev_raw_samples, raw_samples - sconf->max_order,
> +                   sizeof(int64_t) * sconf->max_order);

instead of duplicating the type (int64_t) the type of the array should be used


[...]
> +/** Decodes an ALS frame.
> + */
> +static int decode_frame(AVCodecContext *avctx,
> +                        void *data, int *data_size,
> +                        AVPacket *avpkt)
> +{
> +    ALSDecContext *ctx       = avctx->priv_data;
> +    ALSSpecificConfig *sconf = &ctx->sconf;
> +    const uint8_t *buffer    = avpkt->data;
> +    int buffer_size          = avpkt->size;
> +    int invalid_frame        = 0;
> +    unsigned int c, sample, ra_frame, bytes_read, shift;
> +
> +    init_get_bits(&ctx->gb, buffer, buffer_size * 8);
> +    ra_frame = sconf->random_access && ((!ctx->frame_id) ||
> +               !(ctx->frame_id % sconf->random_access));

the !ctx->frame_id condition seems unneeded


[...]
> +    // allocate previous raw sample buffer
> +    if (!(ctx->prev_raw_samples = av_malloc(sizeof(int64_t) * sconf->max_order))) {
> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> +        decode_end(avctx);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    // allocate raw and carried sample buffer
> +    if (!(ctx->raw_buffer = av_mallocz(sizeof(int64_t) *
> +                                       avctx->channels * channel_size))) {
> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
> +        decode_end(avctx);
> +        return AVERROR(ENOMEM);
> +    }

factorizeable with a goto


[...]
> Index: libavutil/common.h
> ===================================================================
> --- libavutil/common.h	(revision 19673)
> +++ libavutil/common.h	(working copy)
> @@ -225,6 +225,14 @@
>      else               return a;
>  }
>  
> +/** Computes ceil(log2(x)) using av_log2.
> + * @param x value used to compute ceil(log2(x))
> + * @param computed ceiling of log2(x)
> + */
> +static inline int ceil_log2(int x) {
> +    return x > 1 ? av_log2((x - 1) << 1) : 0;
> +}
> +
>  #define MKTAG(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))
>  #define MKBETAG(a,b,c,d) (d | (c << 8) | (b << 16) | (a << 24))
>  

should be a seperate patch

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090822/c06e26b2/attachment.pgp>



More information about the ffmpeg-devel mailing list