[FFmpeg-devel] [PATCH] ALS decoder

[Diego Biurrun]

On Thu, Sep 17, 2009 at 12:49:22AM +0200, Thilo Borgmann wrote:
> Revision 22 attached.
> 
> --- libavcodec/alsdec.c	(revision 0)
> +++ libavcodec/alsdec.c	(revision 0)
> @@ -0,0 +1,1035 @@
> +
> +typedef struct {
> +    AVCodecContext *avctx;
> +    ALSSpecificConfig sconf;
> +    GetBitContext gb;
> +    unsigned int num_frames;   ///< number of frames to decode, 0 if unknown
> +    unsigned int cur_frame_length;  ///< length of the current frame to decode
> +    unsigned int last_frame_length; ///< length of the last frame to decode, 0 if unknown
> +    unsigned int frame_id;     ///< the frame ID / number of the current frame
> +    unsigned int js_switch;    ///< if true, joint-stereo decoding is enforced
> +    unsigned int num_blocks;   ///< number of blocks used in the current frame
> +    int32_t *quant_cof;        ///< quantized parcor coefficients
> +    int32_t *lpc_cof;          ///< coefficients of the direct form prediction filter
> +    int32_t *prev_raw_samples; ///< contains unshifted raw samples from the previous block
> +    int32_t **raw_samples;     ///< decoded raw samples for each channel
> +    int32_t *raw_buffer;       ///< contains all decoded raw samples including carryover samples
> +} ALSDecContext;

The comments could be aligned.

> +/** Checks the ALSSpecificConfig for unsupported features.
> + */
> +static int check_specific_config(ALSDecContext *ctx)
> +{
> +    ALSSpecificConfig *sconf = &ctx->sconf;
> +    int error = 0;
> +
> +    if (sconf->floating) {
> +        av_log_missing_feature(ctx->avctx, "Floating point decoding", 0);
> +        error = -1;
> +    }
> +
> +    if (sconf->long_term_prediction) {
> +        av_log_missing_feature(ctx->avctx, "Long-term prediction", 0);
> +        error = -1;
> +    }
> +
> +    if (sconf->bgmc) {
> +        av_log_missing_feature(ctx->avctx, "BGMC entropy decoding", 0);
> +        error = -1;
> +    }
> +
> +    if (sconf->mc_coding) {
> +        av_log_missing_feature(ctx->avctx, "Multi-channel correlation", 0);
> +        error = -1;
> +    }
> +
> +    if (sconf->chan_sort) {
> +        av_log_missing_feature(ctx->avctx, "Channel sorting", 0);
> +    }
> +
> +    if (sconf->rlslms) {
> +        av_log_missing_feature(ctx->avctx, "Adaptive RLS-LMS prediction", 0);
> +        error = -1;
> +    }

Why do you not return an error for channel sorting?

You could maybe use a macro instead of repeating the same block six
times.

> +static void parcor_to_lpc(unsigned int k, const int32_t *par, int32_t *cof)
> +{
> +    int i, j;
> +
> +    for (i = 0, j = k - 1; i < j; i++, j--) {
> +        int tmp1 = ((MUL64(par[k], cof[j]) + (1 << 19)) >> 20);
> +        cof[j]  += ((MUL64(par[k], cof[i]) + (1 << 19)) >> 20);
> +        cof[i]  += tmp1;
> +    }
> +    if (i == j) cof[i] += ((MUL64(par[k], cof[j]) + (1 << 19)) >> 20);

Please put the statement on the next line.

> --- libavcodec/als_data.h	(revision 0)
> +++ libavcodec/als_data.h	(revision 0)
> @@ -0,0 +1,96 @@
> +
> +/**
> + * @file libavcodec/als_data.h
> + * MPEG-4 ALS common data tables
> + * @author Thilo Borgmann <thilo.borgmann _at_ googlemail.com>
> + */
> +
> +
> +#ifndef AVCODEC_ALS_DATA_H
> +#define AVCODEC_ALS_DATA_H

I don't know if it's a problem for Doxygen, but we usually place the
multiple inclusion guards right after the copyright header.  Yes, this
is pure speculation.

Diego