[FFmpeg-devel] [PATCH] RealAudio SIPR @16k decoder (1/4) Core
Michael Niedermayer
michaelni
Thu Jul 17 00:30:33 CEST 2008
On Tue, Jul 15, 2008 at 02:58:50PM +0700, Vladimir Voroshilov wrote:
> 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?
BFI provides error concealment in case of damaged frames i assume.
How does it perform quality wise in relation to generic audio error
concealment?
You dont know? Then test it, set the flag randomly to 1 and compare how it
sounds compared to other generic solutions, replicating the previous packet
replicating + windowing, ...
If you dont want or dont care then dont submit the bfi code
The same applies to the g729 error concealment code!
A clean audio error concealment system is surely something i want but.
I do not want to have dozends of pages of completely untested code in
each audio codec under if(0). It would be different if it where just
a few lines but its quite a lot more than that.
And above all i do not want to have the almost same ACELP error concealment
replicated in each decoder.
>
> > [...]
> >> +/**
> >> + * \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.
Choose what is simpler, in this case iam pretty sure the code in the patch
is not the simpler solution, i do not know about g729, thats a seperate issue.
>
> > [...]
> >> +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)
Help fixing them, in many cases the checks might not be needed as the
needed amount is tiny and the default buffers are large enough but its
better if they are there nonetheless.
>
> > 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).
all fields in a structure that are never written to or are only set to a
constant are useless.
all fields that are never read are as well useless.
This has nothing to do with not enough knowledge/expirence/practice its
plain submission of random trash. Its not too much to expect from the patch
author to remove variables that are never set.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20080717/1120c93a/attachment.pgp>
More information about the ffmpeg-devel
mailing list