[FFmpeg-devel] [PATCH] ATRAC3+ decoder, 2nd try

Michael Niedermayer michaelni at gmx.at
Sat Oct 19 13:52:49 CEST 2013


On Sat, Oct 19, 2013 at 12:37:07AM +0200, Maxim Polijakowski wrote:
> Hello guys,
> 
> due to a bunch of comments and fixes I've decided to start a new
> thread. Please find attached an updated patch.
> 
> Below a detailed overview of what has been changed:
> 
> --> init_get_bits has been replaced with init_get_bits8
> --> Fixed a memleak caused by allocation of ch_units
> --> Added several missing static const
> --> VLC tables have been made static. There is no dynamic allocation
> and deallocation anymore
> --> Loops simplifications and some other minor code refractions
> --> Several "delta = (delta_bits ? get_bits(gb, delta_bits) : 0)"
> have been made a macro
> --> Added several safety checks before reading from bitstream
> --> decode_frame now returns avpkt->size instead of avctx->block_align
> --> replace amp_mant_table with direct computation
> --> AVFloatDSPContext has been moved to the decoder context
> --> Several decoding tables have been aligned to be SIMD-friendly
> --> IMDCT windowing have been reworked to use ff_sine_64 directly
> --> Replace several loops with float vector operations
> --> Fix a bug causing random decoder crashes
> --> better support for mono samples coming form PSP
> 
> Best regards
> Maxim

[...]
> +#define GET_DELTA(gb, delta_bits) \
> +    ((delta_bits) ? get_bits((gb), (delta_bits)) : 0)
> +
> +/**
> + * Generate canonical VLC table from given descriptor.
> + *
> + * @param[in]     cb          ptr to codebook descriptor
> + * @param[in]     xlat        ptr to translation table or NULL
> + * @param[in,out] tab_offset  ptr to the table offset
> + * @param[out]    out_vlc     ptr to vlc table to be generated
> + */
> +static av_cold void build_canonical_huff(const uint8_t *cb, const uint8_t *xlat,
> +                                         int *tab_offset, VLC *out_vlc)
> +{
> +    int i, b;
> +    uint16_t codes[256];
> +    uint8_t bits[256];
> +    unsigned code = 0;
> +    int index = 0;
> +    int min_len = *cb++; // get shortest codeword length
> +    int max_len = *cb++; // get longest  codeword length
> +
> +    for (b = min_len; b <= max_len; b++) {
> +        for (i = *cb++; i > 0; i--) {


the *cb++ accesses a global table, so when 2 decoders get inited
at the same time there is risk that the read,increase,write
sequence does get interrupted by another threads read, increase, write
causing the value to become corrupted

its not a real problem as the init is done under a mutex, and the
function would disappear when someone restructures the vlc tables
i just thought i mention it as i noticed it


[...]

> +static void add_wordlen_weights(Atrac3pChanUnitCtx *ctx,
> +                                Atrac3pChanParams *chan, int wtab_idx)
> +{
> +    int i;
> +    const int8_t *weights_tab =
> +        &ff_atrac3p_wl_weights[chan->ch_num * 3 + wtab_idx - 1][0];
> +

> +    for (i = 0; i < ctx->num_quant_units; i++)
> +        chan->qu_wordlen[i] += weights_tab[i];

this is probably missing a &7 or some other kind of check
qu_wordlen[] is used as index into an array


> +}
> +
> +static void subtract_sf_weights(Atrac3pChanUnitCtx *ctx,
> +                                Atrac3pChanParams *chan, int wtab_idx)
> +{
> +    int i;
> +    const int8_t *weights_tab;
> +
> +    weights_tab = &ff_atrac3p_sf_weights[wtab_idx - 1][0];
> +

> +    for (i = 0; i < ctx->used_quant_units; i++)
> +        chan->qu_sf_idx[i] -= weights_tab[i];

this too is probably missing some kind of check or wraparound on the
limits, also quite possibly other related routines need checks


> +}
> +
> +static void unpack_wordlen_shape(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                                 Atrac3pChanParams *chan)
> +{
> +    int i;
> +    const int8_t *shape_vec;
> +    int start_val = get_bits(gb, 3);
> +    int shape_idx = get_bits(gb, 4);
> +
> +    if (chan->num_coded_vals) {
> +        shape_vec = &ff_atrac3p_wl_shapes[start_val][shape_idx][0];
> +
> +        /* unpack shape vector values for each quant unit */
> +        chan->qu_wordlen[0] = start_val;
> +        chan->qu_wordlen[1] = start_val;
> +        chan->qu_wordlen[2] = start_val;
> +
> +        for (i = 3; i < chan->num_coded_vals; i++)
> +            chan->qu_wordlen[i] = start_val -
> +                                  shape_vec[ff_atrac3p_qu_num_to_seg[i] - 1];
> +    }
> +}
> +
> +static void unpack_scalefactor_shape(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                                     Atrac3pChanParams *chan)
> +{
> +    int i;
> +    const int8_t *shape_vec;
> +    int start_val = get_bits(gb, 6);
> +    int shape_idx = get_bits(gb, 6);
> +
> +    if (ctx->used_quant_units) {
> +        shape_vec = &ff_atrac3p_sf_shapes[shape_idx][0];
> +
> +        /* unpack shape vector values for each quant unit */
> +        chan->qu_sf_idx[0] = start_val;
> +        chan->qu_sf_idx[1] = start_val;
> +        chan->qu_sf_idx[2] = start_val;
> +
> +        for (i = 3; i < ctx->used_quant_units; i++)
> +            chan->qu_sf_idx[i] = start_val -
> +                                 shape_vec[ff_atrac3p_qu_num_to_seg[i] - 1];
> +    }
> +}

the qu_wordlen and qu_sf_idx related code looks quite similar in
various places of the code, possible it could be factorized to make
it simpler ?


> +
> +/**
> + * Decode word length for each quantization unit of a channel.
> + *
> + * @param[in,out] ctx           ptr to the decoder context
> + * @param[in]     num_channels  number of channels to process
> + * @param[in]     avctx         ptr to the AVCodecContext
> + * @return result code: 0 = OK, -1 = error
> + */
> +static int decode_channel_wordlen(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                                  int ch_num, AVCodecContext *avctx)
> +{
> +    int i, weight_idx = 0, delta, diff, pos, delta_bits, min_val, flag, ret;
> +    VLC *vlc_tab;
> +    Atrac3pChanParams *chan     = &ctx->channels[ch_num];
> +    Atrac3pChanParams *ref_chan = &ctx->channels[0];
> +
> +    chan->fill_mode = 0;
> +
> +    switch (get_bits(gb, 2)) { /* switch according to coding mode */
> +    case 0: /* coded using constant number of bits */
> +        for (i = 0; i < ctx->num_quant_units; i++)
> +            chan->qu_wordlen[i] = get_bits(gb, 3);
> +        break;
> +    case 1:
> +        if (ch_num) {
> +            if ((ret = num_coded_units(gb, chan, ctx, avctx)) < 0)
> +                return ret;
> +
> +            if (chan->num_coded_vals) {
> +                vlc_tab = &wl_vlc_tabs[get_bits(gb, 2)];
> +
> +                for (i = 0; i < chan->num_coded_vals; i++) {
> +                    delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
> +                    chan->qu_wordlen[i] = (ref_chan->qu_wordlen[i] + delta) & 7;
> +                }
> +            }
> +        } else {
> +            weight_idx = get_bits(gb, 2);
> +            if ((ret = num_coded_units(gb, chan, ctx, avctx)) < 0)
> +                return ret;
> +
> +            if (chan->num_coded_vals) {
> +                pos = get_bits(gb, 5);
> +                if (pos > chan->num_coded_vals) {
> +                    av_log(avctx, AV_LOG_ERROR,
> +                           "WL mode 1: invalid position!\n");
> +                    return AVERROR_INVALIDDATA;
> +                }
> +
> +                delta_bits = get_bits(gb, 2);
> +                min_val    = get_bits(gb, 3);
> +
> +                for (i = 0; i < pos; i++)
> +                    chan->qu_wordlen[i] = get_bits(gb, 3);
> +

> +                for (i = pos; i < chan->num_coded_vals; i++)
> +                    chan->qu_wordlen[i] = min_val + GET_DELTA(gb, delta_bits);

this is probably missing a &7 or some other kind of check


[...]
> +static inline void gainc_loc_mode0(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                                   AtracGainInfo *dst, int pos)
> +{
> +    int delta_bits;
> +
> +    if (!pos || dst->loc_code[pos - 1] < 15)
> +        dst->loc_code[pos] = get_bits(gb, 5);
> +    else if (dst->loc_code[pos - 1] == 30)
> +        dst->loc_code[pos] = 31;
> +    else {

> +        delta_bits         = av_log2(30 - dst->loc_code[pos - 1]) + 1;

for loc code 30 or larger this looks odd


> +        dst->loc_code[pos] = dst->loc_code[pos - 1] +
> +                             get_bits(gb, delta_bits) + 1;

i suspect this should be checked against some max or maybe wrap around


> +    }
> +}
> +
> +static inline void gainc_loc_mode1(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                                   AtracGainInfo *dst)
> +{
> +    int i;
> +    VLC *tab;
> +
> +    if (dst->num_points > 0) {
> +        /* 1st coefficient is stored directly */
> +        dst->loc_code[0] = get_bits(gb, 5);
> +
> +        for (i = 1; i < dst->num_points; i++) {
> +            /* switch VLC according to the curve direction
> +             * (ascending/descending) */
> +            tab              = (dst->lev_code[i] <= dst->lev_code[i - 1])
> +                               ? &gain_vlc_tabs[7]
> +                               : &gain_vlc_tabs[9];

> +            dst->loc_code[i] = dst->loc_code[i - 1] +
> +                               get_vlc2(gb, tab->table, tab->bits, 1);

like mode0, i suspect this should be checked against some max or maybe wrap around


[...]
> +/**
> + * Decode gain control data for all channels.
> + *
> + * @param[in,out] ctx           ptr to the decoder context
> + * @param[in]     num_channels  number of channels to process
> + * @param[in]     avctx         ptr to the AVCodecContext
> + */
> +static int decode_gainc_data(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                             int num_channels, AVCodecContext *avctx)
> +{
> +    int ch_num, coded_subbands, sb, ret;
> +
> +    for (ch_num = 0; ch_num < num_channels; ch_num++) {
> +        memset(ctx->channels[ch_num].gain_data, 0,
> +               sizeof(*ctx->channels[ch_num].gain_data) * ATRAC3P_SUBBANDS);
> +
> +        if (get_bits1(gb)) { /* gain control data present? */
> +            coded_subbands = get_bits(gb, 4) + 1;
> +            if (get_bits1(gb)) /* is high band gain data replication on? */
> +                ctx->channels[ch_num].num_gain_subbands = get_bits(gb, 4) + 1;
> +            else
> +                ctx->channels[ch_num].num_gain_subbands = coded_subbands;
> +

> +            if ((ret = decode_gainc_npoints(gb, ctx, ch_num, coded_subbands, avctx)   < 0) ||
> +                (ret = decode_gainc_levels(gb, ctx, ch_num, coded_subbands, avctx)    < 0) ||
> +                (ret = decode_gainc_loc_codes(gb, ctx, ch_num, coded_subbands, avctx) < 0))

this looks like its missing some ()


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131019/dd6cfd45/attachment.asc>


More information about the ffmpeg-devel mailing list