On 9/30/2013 1:38 PM, Maxim Polijakowski wrote:
> Hi crews,
> 
> the attached patch set delivers a generalized version of the ATRAC3 gain 
> compensation code so it can be reused in the upcoming ATRAC3+ decoder 
> without reinventing the wheel. Moreover, a minor cleanup (improving 
> descriptions and doxygen comments) has been made as well.

I'll do the Diego thing in his absence.

>>From b8db1f2eb215ca20fad0783c63742cac9ea1fbda Mon Sep 17 00:00:00 2001
> From: Maxim Poliakovski <[email protected]>
> Date: Mon, 30 Sep 2013 14:27:31 +0200
> Subject: [PATCH 3/3] Generalize ATRAC3 gain compensation code and move it to
>  atrac.c so it can be reused within ATRAC3+ decoder.

Suggestion:

    atrac3: Generalize gain compensation code

    Move it to atrac.c, so it can be reused within the ATRAC3+ decoder.

> +            /* interpolate between two different gain levels */
> +            for (; pos < lastpos + gctx->loc_size; pos++) {
> +                out[pos] = (in[pos] * gc_scale + prev[pos]) * lev;
> +                lev *= gain_inc;

Align.

> +    /* copy the overlapping part into delay buffer */
> +    memcpy(prev, &in[num_samples], num_samples*sizeof(float));

s/*/ * /

> +/**
> + *  Gain control parameters for one subband.
> + */
> +typedef struct {
> +    int   num_points; ///< number of gain control points
> +    int   levcode[7]; ///< level at corresponding control point
> +    int   loccode[7]; ///< location of gain control points
> +} GainInfo;

We don't use anonymous structs anymore. Change to:

    typedef struct GainInfo {
        int   num_points; ///< number of gain control points
        int   levcode[7]; ///< level at corresponding control point
        int   loccode[7]; ///< location of gain control points
    } GainInfo;

> +/**
> + *  Gain compensation context structure.
> + */
> +typedef struct {
> +    float   gain_tab1[16]; ///< gain compensation level table
> +    float   gain_tab2[31]; ///< gain compensation interpolation table
> +    int     id2exp_offset; ///< offset for converting level index into level 
> exponent
> +    int     loc_scale;     ///< scale of location code = 2^loc_scale samples
> +    int     loc_size;      ///< size of location code in samples
> +} GCContext;

Change to:

    typedef struct GCContext {
        float   gain_tab1[16]; ///< gain compensation level table
        float   gain_tab2[31]; ///< gain compensation interpolation table
        int     id2exp_offset; ///< offset for converting level index into 
level exponent
        int     loc_scale;     ///< scale of location code = 2^loc_scale samples
        int     loc_size;      ///< size of location code in samples
    } GCContext;

> -        for (cf = 0; cf < gain[i].num_gain_data; cf++) {
> -            level[cf] = get_bits(gb, 4);
> -            loc  [cf] = get_bits(gb, 5);
> -            if (cf && loc[cf] <= loc[cf - 1])
> +        for (i = 0; i < gain[b].num_points; i++) {
> +            level[i] = get_bits(gb, 4);
> +            loc  [i] = get_bits(gb, 5);
> +            if (i && loc[i] <= loc[i-1])
>                  return AVERROR_INVALIDDATA;

Unless this error is propagated with a proper av_log() call
elsewhere, one should be added.

Rest LGTM.

IMO these changes are trivial and can be applied by whoever pushes
this patch set.

- Derek
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to