[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