[FFmpeg-devel] [PATCH] MPEG-4 ALS decoder MCC decoding bug (non-lossless output) (Fixes issue 2387)

Thilo Borgmann thilo.borgmann
Fri Nov 26 19:49:42 CET 2010


Hi,

> This is a patch for your consideration for issue 2387.

thanks a lot for reporting and submitting a patch!

I can approve this issue. Whoever is responsible for roundup: either I need
permission to change issues or please change this issue to approved and assign
it to me.

Just a little remark, if you would not have submitted a patch that successfully
decodes the sample you've uploaded, the original waveform would have been nice
to have, too.

>
> Some of the block parameters were not properly stored per channel prior to
> decoding and thus got overwritten by the parameters of other channels.
>
> A few other block parameters were already correctly stored per-channel in the
> source code, so the patch simply follows the example of the exising
> implementation, and extends it for all the necessary block parameters.

Yes, very well - like it.

>
> According to my tests, the patch does not introduce any new decoder failure,
> and fixes issue 2387.

My local test suite is pleased as well. FATE might tell more on this...

See my review below:


> Index: libavcodec/alsdec.c
> ===================================================================
> --- libavcodec/alsdec.c	(revision 25831)
> +++ libavcodec/alsdec.c	(working copy)
> @@ -205,6 +205,9 @@ typedef struct {
>      uint8_t *bgmc_lut;              ///< pointer at lookup tables used for BGMC
>      int *bgmc_lut_status;           ///< pointer at lookup table status flags
used for BGMC
>      int ltp_lag_length;             ///< number of bits used for ltp lag value
> +    int *const_block;               ///< contains const_block flags for all
channels
> +    unsigned int *shift_lsbs;       ///< contains shift_lsbs flags for all
channels
> +    unsigned int *opt_order;        ///< contains opt_order flags for all
channels
>      int *use_ltp;                   ///< contains use_ltp flags for all channels
>      int *ltp_lag;                   ///< contains ltp lag values for all channels
>      int **ltp_gain;                 ///< gain values for ltp 5-tap filter for
a channel

ok


> @@ -227,12 +230,10 @@ typedef struct {
>  typedef struct {
>      unsigned int block_length;      ///< number of samples within the block
>      unsigned int ra_block;          ///< if true, this is a random access block
> -    int          const_block;       ///< if true, this is a constant value block
> -    int32_t      const_val;         ///< the sample value of a constant block
                    ^^^^^^^^^

Removing const_val is unrelated, see comments below.


> +    int          *const_block;      ///< if true, this is a constant value block
>      int          js_blocks;         ///< true if this block contains a
difference signal
> -    unsigned int shift_lsbs;        ///< shift of values for this block
> -    unsigned int opt_order;         ///< prediction order of this block

> -    int          store_prev_samples;///< if true, carryover samples have to
be stored
                    ^^^^^^^^^^^^^^^^^^

Unrelated, more comments below.


> +    unsigned int *shift_lsbs;       ///< shift of values for this block
> +    unsigned int *opt_order;        ///< prediction order of this block
>      int          *use_ltp;          ///< if true, long-term prediction is used
>      int          *ltp_lag;          ///< lag value for long-term prediction
>      int          *ltp_gain;         ///< gain values for ltp 5-tap filter
> @@ -244,6 +245,15 @@ typedef struct {
>  } ALSBlockData;
>
>
> +/**
> + * Return whether or not carryover samples have to be stored.
> + */
> +static inline int store_prev_samples(const ALSBlockData *bd)
> +{
> +    return (bd->js_blocks && bd->raw_other) || *bd->shift_lsbs;
> +}
> +
> +

I don't see why this should be necessary. Storing a value for this saves
computation and your patch works fine without this function and keeping the
value stored.


>  static av_cold void dprint_specific_config(ALSDecContext *ctx)
>  {
>  #ifdef DEBUG
> @@ -554,20 +564,20 @@ static void read_const_block_data(ALSDecContext *c
>      AVCodecContext *avctx    = ctx->avctx;
>      GetBitContext *gb        = &ctx->gb;
>
> -    bd->const_val    = 0;
> -    bd->const_block  = get_bits1(gb);    // 1 = constant value, 0 = zero
block (silence)
> +    *bd->raw_samples = 0;

Storing the value of the constant block in the first element of raw_samples is a
nice idea I would like to see in a seperate patch! For this bug it is unrelated.


> +    *bd->const_block = get_bits1(gb);    // 1 = constant value, 0 = zero
block (silence)
>      bd->js_blocks    = get_bits1(gb);
>
>      // skip 5 reserved bits
>      skip_bits(gb, 5);
>
> -    if (bd->const_block) {
> +    if (*bd->const_block) {
>          unsigned int const_val_bits = sconf->floating ? 24 :
avctx->bits_per_raw_sample;
> -        bd->const_val = get_sbits_long(gb, const_val_bits);
> +        *bd->raw_samples = get_sbits_long(gb, const_val_bits);
>      }
>
>      // ensure constant block decoding by reusing this field
> -    bd->const_block = 1;
> +    *bd->const_block = 1;
>  }

ok


>
>
> @@ -575,9 +585,9 @@ static void read_const_block_data(ALSDecContext *c
>   */
>  static void decode_const_block_data(ALSDecContext *ctx, ALSBlockData *bd)
>  {
> -    int      smp = bd->block_length;
> -    int32_t  val = bd->const_val;
> -    int32_t *dst = bd->raw_samples;
> +    int      smp = bd->block_length - 1;
> +    int32_t  val = *bd->raw_samples;
> +    int32_t *dst = bd->raw_samples + 1;

Belongs to the unrelated change...


>
>      // write raw samples into buffer
>      for (; smp; smp--)
> @@ -604,12 +614,12 @@ static int read_var_block_data(ALSDecContext *ctx,
>
>
>      // ensure variable block decoding by reusing this field
> -    bd->const_block = 0;
> +    *bd->const_block = 0;
>
> -    bd->opt_order   = 1;
> +    *bd->opt_order  = 1;
>      bd->js_blocks   = get_bits1(gb);
>
> -    opt_order       = bd->opt_order;
> +    opt_order       = *bd->opt_order;
>
>      // determine the number of subblocks for entropy decoding
>      if (!sconf->bgmc && !sconf->sb_part) {
> @@ -649,21 +659,18 @@ static int read_var_block_data(ALSDecContext *ctx,
>      }
>
>      if (get_bits1(gb))
> -        bd->shift_lsbs = get_bits(gb, 4) + 1;
> +        *bd->shift_lsbs = get_bits(gb, 4) + 1;

ok


>
> -    bd->store_prev_samples = (bd->js_blocks && bd->raw_other) || bd->shift_lsbs;

See comments above, this line works just fine here.


> -
> -
>      if (!sconf->rlslms) {
>          if (sconf->adapt_order) {
>              int opt_order_length = av_ceil_log2(av_clip((bd->block_length >>
3) - 1,
>                                                  2, sconf->max_order + 1));
> -            bd->opt_order        = get_bits(gb, opt_order_length);
> +            *bd->opt_order       = get_bits(gb, opt_order_length);
>          } else {
> -            bd->opt_order = sconf->max_order;
> +            *bd->opt_order = sconf->max_order;
>          }
>
> -        opt_order = bd->opt_order;
> +        opt_order = *bd->opt_order;
>
>          if (opt_order) {
>              int add_base;
> @@ -840,7 +847,7 @@ static int decode_var_block_data(ALSDecContext *ct
>      unsigned int block_length = bd->block_length;
>      unsigned int smp = 0;
>      unsigned int k;
> -    int opt_order             = bd->opt_order;
> +    int opt_order             = *bd->opt_order;
>      int sb;
>      int64_t y;
>      int32_t *quant_cof        = bd->quant_cof;
> @@ -885,7 +892,7 @@ static int decode_var_block_data(ALSDecContext *ct
>              parcor_to_lpc(k, quant_cof, lpc_cof);
>
>          // store previous samples in case that they have to be altered
> -        if (bd->store_prev_samples)
> +        if (store_prev_samples(bd))

See above...


>              memcpy(bd->prev_raw_samples, raw_samples - sconf->max_order,
>                     sizeof(*bd->prev_raw_samples) * sconf->max_order);
>
> @@ -906,9 +913,9 @@ static int decode_var_block_data(ALSDecContext *ct
>          }
>
>          // reconstruct shifted signal
> -        if (bd->shift_lsbs)
> +        if (*bd->shift_lsbs)
>              for (sb = -1; sb >= -sconf->max_order; sb--)
> -                raw_samples[sb] >>= bd->shift_lsbs;
> +                raw_samples[sb] >>= *bd->shift_lsbs;
>      }
>
>      // reverse linear prediction coefficients for efficiency
> @@ -933,7 +940,7 @@ static int decode_var_block_data(ALSDecContext *ct
>      raw_samples = bd->raw_samples;
>
>      // restore previous samples in case that they have been altered
> -    if (bd->store_prev_samples)
> +    if (store_prev_samples(bd))

See above...


>          memcpy(raw_samples - sconf->max_order, bd->prev_raw_samples,
>                 sizeof(*raw_samples) * sconf->max_order);
>
> @@ -947,6 +954,7 @@ static int read_block(ALSDecContext *ctx, ALSBlock
>  {
>      GetBitContext *gb        = &ctx->gb;
>
> +    *bd->shift_lsbs = 0;
>      // read block type flag and read the samples accordingly
>      if (get_bits1(gb)) {
>          if (read_var_block_data(ctx, bd))
> @@ -966,16 +974,16 @@ static int decode_block(ALSDecContext *ctx, ALSBlo
>      unsigned int smp;
>
>      // read block type flag and read the samples accordingly
> -    if (bd->const_block)
> +    if (*bd->const_block)
>          decode_const_block_data(ctx, bd);
>      else if (decode_var_block_data(ctx, bd))
>          return -1;
>
>      // TODO: read RLSLMS extension data
>
> -    if (bd->shift_lsbs)
> +    if (*bd->shift_lsbs)
>          for (smp = 0; smp < bd->block_length; smp++)
> -            bd->raw_samples[smp] <<= bd->shift_lsbs;
> +            bd->raw_samples[smp] <<= *bd->shift_lsbs;

These and all following changes are ok.


Nice work except the things I've mentioned. I would really like to see another
patch getting const_val to be stored in the raw_buffer.

-Thilo



More information about the ffmpeg-devel mailing list