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

Vladimir Voroshilov voroshil
Tue Jul 15 09:58:50 CEST 2008


2008/7/13 Michael Niedermayer <michaelni at gmx.at>:
> 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

G.729 does not specify how does BFI (Bad Frame Indicator) flag is set,
but describes
procedures for fixing bad frames.
I think this is the demuxer's task. But since i don't know yet any
container which can hande BFI flag for G.729
i have made bfi always zero (assuming it's possible future use).

Perhaps, selected solution is not good. I though about one extra byte
in G.729 packet. It can carry BFI flag and, probably,
frame format type. The latter will be usefuly for mixed 8k/6.4k mode
(when decoder dynamically switches between 8k/6.4k modes in the same
stream) which is supported and described in the g.729 specification
(except frame type detection method). What will you say about this?

> [...]
>> +/**
>> + * \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?

Both solutions are good for me. I have seleted those which, i think, has
better readability. If you wish i can rewrite this (anf g.729 ?) code
for using in-place readings.

> [...]
>> +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)

I used 8svc.c file for understanding decoder structure (since it is
quite simple and belongs to top of directory structure).
I thought that output buffer is always at least acvxt->frame_size bytes long.
After your comment i'm afraid it is not. So i'll add buffer size
checks into my code.

Related nits:
$grep -c "\*data_size" libavcodec/*.c | grep ":2" | wc -l
81
$grep "\*data_size" libavcodec/*.c | wc -l
340

In other words 81 codecs of 340 checked does not have output buffer checks.
(Yes, yes, this is not the reason to skip such checks in my code)

> 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!

I'll try and fix issues pointed by you.
Not sure about useless variables and useless memcpy, though.
Sometimes my feeling/knowledge of "useless" is not comply with your
(not enough knowledge/expirence/practice ?. C programming is still my hobby).

P.S. Perhaps posting patch marked as "draft" was not good idea at all.

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719




More information about the ffmpeg-devel mailing list