[FFmpeg-devel] [PATCH] RealAudio SIPR @16k decoder (1/4) Core

Michael Niedermayer michaelni
Sun Jul 13 15:37:28 CEST 2008


On Sat, Jul 12, 2008 at 02:16:24PM +0700, Vladimir Voroshilov wrote:
> Hi, All
> 
> Here is first draft version of fixed-point RealAudio sipr decoder
> (only 16kHz mode)
> 
> This patch contains core routine.
[...]
> +/**
> + * \brief main decoding routine
> + */
> +static int decod_frame_16k(Sipr_Context * ctx, Sipr_Parameters *params, int16_t* out_data)
> +{
> +    int16_t fc_v[L_SUBFR_16k0];
> +    int16_t gain_pitch, gain_code;
> +    int16_t lp_filter_coeffs[2][LP_FILTER_ORDER + 1];
> +    int16_t lsp[LP_FILTER_ORDER];
> +    int16_t lsf[LP_FILTER_ORDER];
> +    int16_t synth[LP_FILTER_ORDER + 2 * L_SUBFR_16k0];
> +
> +    int i, i_subfr;
> +    int pitch_delay_3x, pitch_delay_int;
> +    int16_t B[LP_FILTER_ORDER];           // FIXME: I don't know what is this
> +    int16_t var348[LP_FILTER_ORDER + 30]; // FIXME: I don't know what is this
> +
> +    if(params->bfi)

The code is full of bfi checks but bfi is never set to a non zero value


[...]
> +/**
> + * \brief Extract decoding parameters from the input bitstream.
> + * \param parms          parameters structure
> + * \param bits_per_frame frame size in bits
> + * \param in_buf         pointer to the input bitstream
> + */
> +static void decode_parameters(Sipr_Parameters* parms, int bits_per_frame, const uint8_t *in_buf)
> +{
> +    GetBitContext       gb;
> +
> +    init_get_bits(&gb, in_buf, bits_per_frame);
> +
> +    parms->bfi              = 0;
> +    parms->ma_pred_switch   = get_bits(&gb, MA_PREDICTOR_BITS);
> +    parms->vq_indexes[0]    = get_bits(&gb, VQ_INDEX1_BITS);
> +    parms->vq_indexes[1]    = get_bits(&gb, VQ_INDEX2_BITS);
> +    parms->vq_indexes[2]    = get_bits(&gb, VQ_INDEX3_BITS);
> +    parms->vq_indexes[3]    = get_bits(&gb, VQ_INDEX4_BITS);
> +    parms->vq_indexes[4]    = get_bits(&gb, VQ_INDEX5_BITS);
> +    parms->pitch_delay[0]   = get_bits(&gb, AC_INDEX_1ST_BITS);
> +    parms->gp_index[0]      = get_bits(&gb, GP_INDEX_BITS);
> +    parms->fc_indexes[0][0] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[0][1] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[0][2] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[0][3] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[0][4] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->gc_index[0]      = get_bits(&gb, GC_INDEX_BITS);
> +    parms->pitch_delay[1]   = get_bits(&gb, AC_INDEX_2ND_BITS);
> +    parms->gp_index[1]      = get_bits(&gb, GP_INDEX_BITS);
> +    parms->fc_indexes[1][0] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[1][1] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[1][2] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[1][3] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->fc_indexes[1][4] = get_bits(&gb, FC_INDEX_BITS);
> +    parms->gc_index[1]      = get_bits(&gb, GC_INDEX_BITS);
> +}

looks senseless, why is data not read where its needed?


[...]
> +static int ff_sipr_decode_frame(AVCodecContext *avctx,
> +                             void *data, int *data_size,
> +                             const uint8_t *buf, int buf_size)
> +{
> +    Sipr_Context *ctx = avctx->priv_data;
> +    Sipr_Parameters parm;
> +
> +    if (buf_size < avctx->block_align)
> +        return buf_size;
> +    decode_parameters(&parm, ctx->bits_per_frame, buf);
> +
> +    *data_size = decod_frame_16k(ctx, &parm, data) * 2;
> +    return *data_size;
> +};

You are missing a check for the amount of the available output space
Please review your code and ALWAYS ensure that you do not write
outside arrays.
(Note, too meny exploitable bugs will cause me to ignore patches from
you)

Also it seems your patches are always of rather poor quality, i suggest
you review them before submitting.
If i have to keep pointing at the same isses in every patch you post i
might choose not to review them any more.
Some such thing are unused or useless variables, useless memcpy, its very
boring to go over the variables in the context to find half to be constants
or unused like bits_per_frame or bfi and these where the only 2 i checked
this time.
And a grep for [0-9]\.[0-9] shows plenty of hits while float has none.
Float constants in fixed point expressions isnt a good sign ...
So again, go review your code, and when you find nothing you can improve
anymore submit it, not before!

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20080713/00a52a4e/attachment.pgp>



More information about the ffmpeg-devel mailing list