[FFmpeg-devel] [PATCH] G.729 initialization routine (skeleton)

[Stefano Sabatini]

On date Friday 2009-06-05 14:24:39 +0700, Vladimir Voroshilov encoded:
> Hi, All.
> 
> Routines in patch just checks the passed sample_rate for supported modes.
> After check it properly initializes "format" and "subframe_size"
> context members.
> 
> All other initialization code will be added later (LSP structures
> initialization in LSP patch and so on).
> 
> Hopefully it is enough small and clean for easy review.
> Is such patches enough good ?
> Should i post remaining (i don't want to post all patches at the same
> time, since not all patches are
> ready yet).
> 
> -- 
> Regards,
> Vladimir Voroshilov     mailto:voroshil at gmail.com
> JID: voroshil at gmail.com, voroshil at jabber.ru
> ICQ: 95587719

> From 02ea4848b318e8c189f5eb48abdba500c15937e0 Mon Sep 17 00:00:00 2001
> From: Vladimir Voroshilov <voroshil at gmail.com>
> Date: Fri, 5 Jun 2009 00:47:25 +0700
> Subject: [PATCH] Initialization routine

Thanks for the patches, just some nits.
 
> diff --git ffmpeg-r19088.orig/libavcodec/g729dec.c ffmpeg-r19088.mod/libavcodec/g729dec.c
> index 0b70a75..b86874a 100644
> --- ffmpeg-r19088.orig/libavcodec/g729dec.c
> +++ ffmpeg-r19088.mod/libavcodec/g729dec.c
> @@ -81,6 +81,16 @@ typedef struct {
>      int mr_energy;
>  } G729_format_description;
>  
> +typedef enum {
> +    FORMAT_G729_8K = 0,
> +    FORMAT_G729_4K4,
> +} G729_formats;

Maybe G729Format is more FFmpeg style compliant.

> +
> +typedef struct {
> +    G729_formats format;        ///< format index from formats array
> +    int subframe_size;          ///< number of samples produced from one subframe
> +}  G729_Context;

Again ThisIsAStruct style is preferred over This_Is_A_Struct style.

> +
>  /**
>   * \brief pseudo random number generator
>   */

I believe \brief in doxies is deprecated.

> @@ -97,11 +107,45 @@ static inline int g729_get_parity(uint8_t value)
>     return (0x6996966996696996ULL >> (value >> 2)) & 1;
>  }
>  
> +/**
> + * \brief G.729 decoder initialization

I'd prefer "Initializes G.729 decoder", but maybe in this case the
doxy is just redundant.

> + * \param avctx private data structure
> + * \return 0 on success, non-zero otherwise
> + *
> + * For decoder initialization Table 9 from section 4.3 of the specification
> + * is used.
> + */
> +static int g729_decoder_init(AVCodecContext * avctx)
> +{
> +    G729_Context* ctx = avctx->priv_data;
> +
> +    if (avctx->sample_rate == 8000)
> +        ctx->format = FORMAT_G729_8K;
> +#ifdef G729_SUPPORT_4400
> +    else if (avctx->sample_rate == 4400)
> +        ctx->format = FORMAT_G729_4K4;
> +#endif
> +    else {
> +        av_log(avctx, AV_LOG_ERROR, "Sample rate %d is not supported.\n", avctx->sample_rate);
> +        return AVERROR_NOFMT;

I'd prefer avoid the point at the end of the message sentences.

[...]

Regards.
-- 
FFmpeg = Fast and Faboulous Mysterious Portable Evangelical Gadget