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

[Maxim Polijakowski]

Hello Michael,

a new patch has been sent in a separate mail.

My comments are located below...

> [...]
>> +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

Checks added...

>
>> +}
>> +
>> +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

Yes, added checks and war-arounds...


>
>> +}
>> +
>> +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 ?


The above-mentioned vector unpacking operation has been factorized out.

The rest has been left as is. You're right that the code untilizes the 
same operation set again and again. Though its factorization doesn't 
actually worth the hassle because parts to be factorized out are:
--> rather small (1-3 lines)
--> using several parameters (masks, lengths, ranges etc.)

I already tried to do that with "decode_channel_code_tab". The result 
isn't impressive at all and looks like 
"impossible-to-understand-code-magic"...


> [...]
>> +                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

Yes, checks added...

>
> [...]
>> +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

Yes, it's wrong also.
I've changed the condition from "== 30" to ">= 30" and add validation of 
decoded values at the end of the function.

>
>> +        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

Done...

>
>> +    }
>> +}
>> +
>> +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

Done...

>
> [...]
>> +/**
>> + * 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 ()

It actually doesn't do. Otherwise it won't compile....

Best regards
Maxim