On Thu, Oct 10, 2013 at 09:14:36PM +0200, Maxim Polijakowski wrote:
> Hi crews,
> 
> the attached patch adds an open-source decoder for Sony's ATRAC3+
> format. There is a partial description of its internals located
> here: http://wiki.multimedia.cx/index.php?title=ATRAC3plus
> 
> Samples: http://samples.mplayerhq.hu/A-codecs/ATRAC3+/
> 
> Because the patch is huge, I expect several review rounds.
> 
> Therefore, I would like to kindly ask you to sort your review
> comments according to the following priority list:
> --> functionality improvements (code refractions resulting in faster
> or smaller code)
> --> security improvements (code refractions resulting in more secure code)
> --> cosmetic improvements (grammar, spelling etc.)
> 
> Any help regarding fuzzy testing and pointing out insecure code
> would be very appreciated!
> 
> Thank you in advance!
> Best regards
> Maxim

> >From b29fed72addcbdf8043bd9e6d9f74c745edebabb Mon Sep 17 00:00:00 2001
> From: Maxim Poliakovski <[email protected]>
> Date: Mon, 2 Sep 2013 12:31:58 +0200
> Subject: [PATCH 1/2] ATRAC3+ decoder
> 
> Cleanup by Diego Biurrun.
> ---
>  Changelog                    |    1 +
>  configure                    |    1 +
>  doc/general.texi             |    1 +
>  libavcodec/Makefile          |    3 +
>  libavcodec/allcodecs.c       |    1 +
>  libavcodec/atrac3plus.c      | 1735 +++++++++++++++++++++++++++++++++++++
>  libavcodec/atrac3plus.h      |  183 ++++
>  libavcodec/atrac3plus_data.c | 1940 
> ++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/atrac3plus_data.h |  144 ++++
>  libavcodec/atrac3plusdec.c   |  394 +++++++++
>  libavcodec/atrac3plusdsp.c   |  658 ++++++++++++++
>  libavcodec/version.h         |    2 +-
>  12 files changed, 5062 insertions(+), 1 deletion(-)
>  create mode 100644 libavcodec/atrac3plus.c
>  create mode 100644 libavcodec/atrac3plus.h
>  create mode 100644 libavcodec/atrac3plus_data.c
>  create mode 100644 libavcodec/atrac3plus_data.h
>  create mode 100644 libavcodec/atrac3plusdec.c
>  create mode 100644 libavcodec/atrac3plusdsp.c
> 
> diff --git a/libavcodec/atrac3plus.c b/libavcodec/atrac3plus.c
> new file mode 100644
> index 0000000..44b2fc8
> --- /dev/null
> +++ b/libavcodec/atrac3plus.c
> @@ -0,0 +1,1735 @@
> +/*
> + * ATRAC3+ compatible decoder
> + *
> + * Copyright (c) 2010-2013 Maxim Poliakovski
> + *
> + * This file is part of Libav.
> + *
> + * Libav is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * Libav is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> + */
> +
> +/**
> + * @file
> + * Bitstream parser for ATRAC3+ decoder.
> + */
> +
> +#include "avcodec.h"
> +#include "get_bits.h"
> +#include "atrac3plus.h"
> +#include "atrac3plus_data.h"
> +
> +/**
> + * Generate canonical VLC table from given descriptor.
> + *
> + * @param[in]  cb       ptr to codebook descriptor
> + * @param[in]  xlat     ptr to translation table or NULL
> + * @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,
> +                                         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--) {
> +            bits[index]  = b;
> +            codes[index] = code++;
> +            index++;
> +        }
> +        code <<= 1;
> +    }
> +
> +    ff_init_vlc_sparse(out_vlc, max_len, index, bits, 1, 1, codes, 2, 2,
> +                       xlat, 1, 1, 0);

IIRC building VLC might fail (and not only because of wrong codes but also
because of allocation error), so it should be checked.

> +}
> +
> +av_cold void ff_atrac3p_init_vlcs(ATRAC3PVlcTabs *vlc_tabs)
> +{
> +    int i;
> +
> +    ff_init_vlc_sparse(&vlc_tabs->wl_vlc_tabs[0], 2, 3,
> +                       ff_atrac3p_wl_huff_bits1, 1, 1,
> +                       ff_atrac3p_wl_huff_code1, 1, 1,
> +                       ff_atrac3p_wl_huff_xlat1, 1, 1, 0);
> +
> +    ff_init_vlc_sparse(&vlc_tabs->wl_vlc_tabs[1], 3, 5,
> +                       ff_atrac3p_wl_huff_bits2, 1, 1,
> +                       ff_atrac3p_wl_huff_code2, 1, 1,
> +                       ff_atrac3p_wl_huff_xlat2, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->wl_vlc_tabs[2], 5, 8,
> +             ff_atrac3p_wl_huff_bits3, 1, 1,
> +             ff_atrac3p_wl_huff_code3, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->wl_vlc_tabs[3], 5, 8,
> +             ff_atrac3p_wl_huff_bits4, 1, 1,
> +             ff_atrac3p_wl_huff_code4, 1, 1, 0);
> +
> +    ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[0], 9, 64,
> +                       ff_atrac3p_sf_huff_bits1, 1, 1,
> +                       ff_atrac3p_sf_huff_code1, 2, 2,
> +                       ff_atrac3p_sf_huff_xlat1, 1, 1, 0);
> +
> +    ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[1], 9, 64,
> +                       ff_atrac3p_sf_huff_bits1, 1, 1,
> +                       ff_atrac3p_sf_huff_code1, 2, 2,
> +                       ff_atrac3p_sf_huff_xlat2, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->sf_vlc_tabs[2], 9, 64,
> +             ff_atrac3p_sf_huff_bits2, 1, 1,
> +             ff_atrac3p_sf_huff_code2, 2, 2, 0);
> +
> +    init_vlc(&vlc_tabs->sf_vlc_tabs[3], 9, 64,
> +             ff_atrac3p_sf_huff_bits3, 1, 1,
> +             ff_atrac3p_sf_huff_code3, 2, 2, 0);
> +
> +    ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[4], 6, 16,
> +                       ff_atrac3p_sf_huff_bits4, 1, 1,
> +                       ff_atrac3p_sf_huff_code4, 1, 1,
> +                       ff_atrac3p_sf_huff_xlat4, 1, 1, 0);
> +
> +    ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[5], 6, 16,
> +                       ff_atrac3p_sf_huff_bits4, 1, 1,
> +                       ff_atrac3p_sf_huff_code4, 1, 1,
> +                       ff_atrac3p_sf_huff_xlat5, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->sf_vlc_tabs[6], 7, 16,
> +             ff_atrac3p_sf_huff_bits5, 1, 1,
> +             ff_atrac3p_sf_huff_code5, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->sf_vlc_tabs[7], 7, 16,
> +             ff_atrac3p_sf_huff_bits6, 1, 1,
> +             ff_atrac3p_sf_huff_code6, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->ct_vlc_tabs[0], 3, 4,
> +             ff_atrac3p_ct_huff_bits1, 1, 1,
> +             ff_atrac3p_ct_huff_code1, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->ct_vlc_tabs[1], 4, 8,
> +             ff_atrac3p_ct_huff_bits2, 1, 1,
> +             ff_atrac3p_ct_huff_code2, 1, 1, 0);
> +
> +    ff_init_vlc_sparse(&vlc_tabs->ct_vlc_tabs[2], 4, 8,
> +                       ff_atrac3p_ct_huff_bits2, 1, 1,
> +                       ff_atrac3p_ct_huff_code2, 1, 1,
> +                       ff_atrac3p_ct_huff_xlat1, 1, 1, 0);
> +
> +    init_vlc(&vlc_tabs->ct_vlc_tabs[3], 4, 8,
> +             ff_atrac3p_ct_huff_bits3, 1, 1,
> +             ff_atrac3p_ct_huff_code3, 1, 1, 0);
> +
> +    /* build huffman tables for spectrum decoding */
> +    for (i = 0; i < 112; i++) {
> +        if (ff_atrac3p_spectra_tabs[i].cb)
> +            build_canonical_huff(ff_atrac3p_spectra_tabs[i].cb,
> +                                 ff_atrac3p_spectra_tabs[i].xlat,
> +                                 &vlc_tabs->spec_vlc_tabs[i]);
> +        else
> +            vlc_tabs->spec_vlc_tabs[i].table = 0;
> +    }
> +
> +    /* build huffman tables for gain data decoding */
> +    build_canonical_huff(ff_atrac3p_huff_gain_npoints1_cb,
> +                         0,
> +                         &vlc_tabs->gain_vlc_tabs[0]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_npoints1_cb,
> +                         ff_atrac3p_huff_gain_npoints2_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[1]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_lev1_cb,
> +                         ff_atrac3p_huff_gain_lev1_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[2]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_lev2_cb,
> +                         ff_atrac3p_huff_gain_lev2_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[3]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_lev3_cb,
> +                         ff_atrac3p_huff_gain_lev3_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[4]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_lev4_cb,
> +                         ff_atrac3p_huff_gain_lev4_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[5]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_loc3_cb,
> +                         ff_atrac3p_huff_gain_loc3_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[6]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_loc1_cb,
> +                         ff_atrac3p_huff_gain_loc1_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[7]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_loc4_cb,
> +                         ff_atrac3p_huff_gain_loc4_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[8]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_loc2_cb,
> +                         ff_atrac3p_huff_gain_loc2_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[9]);
> +    build_canonical_huff(ff_atrac3p_huff_gain_loc5_cb,
> +                         ff_atrac3p_huff_gain_loc5_xlat,
> +                         &vlc_tabs->gain_vlc_tabs[10]);
> +
> +    /* build huffman tables for tone decoding */
> +    build_canonical_huff(ff_atrac3p_huff_tonebands_cb,
> +                         0,
> +                         &vlc_tabs->tone_vlc_tabs[0]);
> +    build_canonical_huff(ff_atrac3p_huff_numwavs1_cb,
> +                         0,
> +                         &vlc_tabs->tone_vlc_tabs[1]);
> +    build_canonical_huff(ff_atrac3p_huff_numwavs2_cb,
> +                         ff_atrac3p_huff_numwavs2_xlat,
> +                         &vlc_tabs->tone_vlc_tabs[2]);
> +    build_canonical_huff(ff_atrac3p_huff_wav_ampsf1_cb,
> +                         ff_atrac3p_huff_wav_ampsf1_xlat,
> +                         &vlc_tabs->tone_vlc_tabs[3]);
> +    build_canonical_huff(ff_atrac3p_huff_wav_ampsf2_cb,
> +                         ff_atrac3p_huff_wav_ampsf2_xlat,
> +                         &vlc_tabs->tone_vlc_tabs[4]);
> +    build_canonical_huff(ff_atrac3p_huff_wav_ampsf3_cb,
> +                         ff_atrac3p_huff_wav_ampsf3_xlat,
> +                         &vlc_tabs->tone_vlc_tabs[5]);
> +    build_canonical_huff(ff_atrac3p_huff_freq_cb,
> +                         ff_atrac3p_huff_freq_xlat,
> +                         &vlc_tabs->tone_vlc_tabs[6]);

an error check is needed here too (and freeing on error)

> +}
> +
> +av_cold void ff_atrac3p_free_vlcs(ATRAC3PVlcTabs *vlc_tabs)
> +{
> +    int i;
> +
> +    for (i = 0; i < 4; i++)
> +        if (vlc_tabs->wl_vlc_tabs[i].table)
> +            ff_free_vlc(&vlc_tabs->wl_vlc_tabs[i]);
> +
> +    for (i = 0; i < 8; i++)
> +        if (vlc_tabs->sf_vlc_tabs[i].table)
> +            ff_free_vlc(&vlc_tabs->sf_vlc_tabs[i]);
> +
> +    for (i = 0; i < 4; i++)
> +        if (vlc_tabs->ct_vlc_tabs[i].table)
> +            ff_free_vlc(&vlc_tabs->ct_vlc_tabs[i]);
> +
> +    for (i = 0; i < 112; i++)
> +        if (vlc_tabs->spec_vlc_tabs[i].table)
> +            ff_free_vlc(&vlc_tabs->spec_vlc_tabs[i]);
> +
> +    for (i = 0; i < 11; i++)
> +        if (vlc_tabs->gain_vlc_tabs[i].table)
> +            ff_free_vlc(&vlc_tabs->gain_vlc_tabs[i]);
> +
> +    for (i = 0; i < 7; i++)
> +        if (vlc_tabs->tone_vlc_tabs[i].table)
> +            ff_free_vlc(&vlc_tabs->tone_vlc_tabs[i]);

useless checks - ff_free_vlc() will work on empty VLCs fine too

> +}
> +
[...]
> +
> +/**
> + * 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, ATRAC3PVlcTabs *vlc_tabs,
> +                                  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 = &vlc_tabs->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);

probably we can test for delta < 0 here (i.e. a bitstream error)

> +                    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 + (delta_bits ? get_bits(gb, delta_bits) : 
> 0);
> +            }
> +        }
> +        break;
> +    case 2:
> +        if ((ret = num_coded_units(gb, chan, ctx, avctx)) < 0)
> +            return ret;
> +
> +        if (ch_num && chan->num_coded_vals) {
> +            vlc_tab = &vlc_tabs->wl_vlc_tabs[get_bits(gb, 2)];
> +            delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
> +            chan->qu_wordlen[0] = (ref_chan->qu_wordlen[0] + delta) & 7;
> +
> +            for (i = 1; i < chan->num_coded_vals; i++) {
> +                diff = ref_chan->qu_wordlen[i] - ref_chan->qu_wordlen[i - 1];
> +                delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
> +                chan->qu_wordlen[i] = (chan->qu_wordlen[i - 1] + diff + 
> delta) & 7;
> +            }
> +        } else if (chan->num_coded_vals) {
> +            flag    = get_bits(gb, 1);
> +            vlc_tab = &vlc_tabs->wl_vlc_tabs[get_bits(gb, 1)];
> +
> +            unpack_wordlen_shape(gb, ctx, chan);
> +
> +            if (!flag) {
> +                for (i = 0; i < chan->num_coded_vals; i++) {
> +                    delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
> +                    chan->qu_wordlen[i] = (chan->qu_wordlen[i] + delta) & 7;
> +                }
> +            } else {
> +                for (i = 0; i < (chan->num_coded_vals & - 2); i += 2)
> +                    if (!get_bits1(gb)) {
> +                        chan->qu_wordlen[i]     = (chan->qu_wordlen[i] +
> +                                                   get_vlc2(gb, 
> vlc_tab->table,
> +                                                            vlc_tab->bits, 
> 1)) & 7;
> +                        chan->qu_wordlen[i + 1] = (chan->qu_wordlen[i + 1] +
> +                                                   get_vlc2(gb, 
> vlc_tab->table,
> +                                                            vlc_tab->bits, 
> 1)) & 7;
> +                    }
> +
> +                if (chan->num_coded_vals & 1)
> +                    chan->qu_wordlen[i] = (chan->qu_wordlen[i] +
> +                                           get_vlc2(gb, vlc_tab->table,
> +                                                    vlc_tab->bits, 1)) & 7;
> +            }
> +        }
> +        break;
> +    case 3:
> +        weight_idx = get_bits(gb, 2);
> +        if ((ret = num_coded_units(gb, chan, ctx, avctx)) < 0)
> +            return ret;
> +
> +        if (chan->num_coded_vals) {
> +            vlc_tab = &vlc_tabs->wl_vlc_tabs[get_bits(gb, 2)];
> +
> +            /* first coefficient is coded directly */
> +            chan->qu_wordlen[0] = get_bits(gb, 3);
> +
> +            for (i = 1; i < chan->num_coded_vals; i++) {
> +                delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
> +                chan->qu_wordlen[i] = (chan->qu_wordlen[i - 1] + delta) & 7;
> +            }
> +        }
> +        break;
> +    }
> +
> +    if (chan->fill_mode == 2) {
> +        for (i = chan->num_coded_vals; i < ctx->num_quant_units; i++)
> +            chan->qu_wordlen[i] = ch_num ? get_bits1(gb) : 1;

I'd simply memset it and then chan->qu_wordlen[ch_num] = get_bits1(gb);
(if it's in range of course)

> +    } else if (chan->fill_mode == 3) {
> +        pos = ch_num ? chan->num_coded_vals + chan->split_point
> +                     : ctx->num_quant_units - chan->split_point;
> +        for (i = chan->num_coded_vals; i < pos; i++)
> +            chan->qu_wordlen[i] = 1;
> +    }
> +
> +    if (weight_idx)
> +        add_wordlen_weights(ctx, chan, weight_idx);
> +
> +    return 0;
> +}
> +
> +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];
> +}
> +
> +/**
> + * Decode scale factor indexes for each quant 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_sf_idx(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                                 int ch_num, ATRAC3PVlcTabs *vlc_tabs,
> +                                 AVCodecContext *avctx)
> +{
> +    int i, weight_idx, delta, diff, num_long_vals,
> +        delta_bits, min_val, vlc_sel;
> +    VLC *vlc_tab;
> +    Atrac3pChanParams *chan     = &ctx->channels[ch_num];
> +    Atrac3pChanParams *ref_chan = &ctx->channels[0];
> +
> +    switch (get_bits(gb, 2)) { /* switch according to coding mode */
> +    case 0: /* coded using constant number of bits */
> +        for (i = 0; i < ctx->used_quant_units; i++)
> +            chan->qu_sf_idx[i] = get_bits(gb, 6);
> +        break;
> +    case 1:
> +        if (ch_num) {
> +            vlc_tab = &vlc_tabs->sf_vlc_tabs[get_bits(gb, 2)];
> +
> +            for (i = 0; i < ctx->used_quant_units; i++) {
> +                delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
> +                chan->qu_sf_idx[i] = (ref_chan->qu_sf_idx[i] + delta) & 0x3F;
> +            }
> +        } else {
> +            weight_idx = get_bits(gb, 2);
> +            if (weight_idx == 3) {
> +                unpack_scalefactor_shape(gb, ctx, chan);
> +
> +                num_long_vals = get_bits(gb, 5);
> +                delta_bits    = get_bits(gb, 2);
> +                min_val       = get_bits(gb, 4) - 7;
> +
> +                for (i = 0; i < num_long_vals; i++)
> +                    chan->qu_sf_idx[i] = (chan->qu_sf_idx[i] +
> +                                          get_bits(gb, 4) - 7) & 0x3F;
> +
> +                /* all others are: min_val + delta */
> +                for (i = num_long_vals; i < ctx->used_quant_units; i++)
> +                    chan->qu_sf_idx[i] = (chan->qu_sf_idx[i] + min_val +
> +                                          (delta_bits ? get_bits(gb, 
> delta_bits) : 0)) & 0x3F;

Maybe define a macro for get_bits0() instead of constant checks?
Maybe even in get_bits.h

[...]
> +
> +#define CODING_DIRECT get_bits(gb, num_bits)
> +
> +#define CODING_VLC get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1)
> +
> +#define CODING_VLC_DELTA                                                \
> +    (!i) ? CODING_VLC                                                   \
> +         : (pred + get_vlc2(gb, delta_vlc->table,                       \
> +                            delta_vlc->bits, 1)) & mask;                \
> +    pred = chan->qu_tab_idx[i]
> +
> +#define CODING_VLC_DIFF                                                 \
> +    (ref_chan->qu_tab_idx[i] +                                          \
> +     get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1)) & mask

Looks like it could've been used in the previous functions. Or at least
similar macros, it's full of X = (X + Y + get_vlc2()) & mask for instance.

> +/**
> + * Decode code table indexes for each quant 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_code_tab(GetBitContext *gb, Atrac3pChanUnitCtx 
> *ctx,
> +                                   int ch_num, ATRAC3PVlcTabs *vlc_tabs,
> +                                   AVCodecContext *avctx)
> +{
> +    int i, num_vals, num_bits, pred;
> +    int mask = ctx->use_full_table ? 7 : 3; /* mask for modular arithmetic */
> +    VLC *vlc_tab, *delta_vlc;
> +    Atrac3pChanParams *chan     = &ctx->channels[ch_num];
> +    Atrac3pChanParams *ref_chan = &ctx->channels[0];
> +
> +    chan->table_type = get_bits1(gb);
> +
> +    switch (get_bits(gb, 2)) { /* switch according to coding mode */
> +    case 0: /* directly coded */
> +        num_bits = ctx->use_full_table + 2;
> +        DEC_CT_IDX_COMMON(CODING_DIRECT);
> +        break;
> +    case 1: /* entropy-coded */
> +        vlc_tab = ctx->use_full_table ? &vlc_tabs->ct_vlc_tabs[1]
> +                                      : vlc_tabs->ct_vlc_tabs;
> +        DEC_CT_IDX_COMMON(CODING_VLC);
> +        break;
> +    case 2: /* entropy-coded delta */
> +        if (ctx->use_full_table) {
> +            vlc_tab   = &vlc_tabs->ct_vlc_tabs[1];
> +            delta_vlc = &vlc_tabs->ct_vlc_tabs[2];
> +        } else {
> +            vlc_tab   = vlc_tabs->ct_vlc_tabs;
> +            delta_vlc = vlc_tabs->ct_vlc_tabs;
> +        }
> +        pred = 0;
> +        DEC_CT_IDX_COMMON(CODING_VLC_DELTA);
> +        break;
> +    case 3: /* entropy-coded difference to master */
> +        if (ch_num) {
> +            vlc_tab = ctx->use_full_table ? &vlc_tabs->ct_vlc_tabs[3]
> +                                          : vlc_tabs->ct_vlc_tabs;

maybe you could embed index into macro too, so it just needed to know what
ct_vlc_tabs[] to select with ctx->use_full_table

> +            DEC_CT_IDX_COMMON(CODING_VLC_DIFF);
> +        }
> +        break;
> +    }
> +
> +    return 0;
> +}
> +
[...]
> +static int decode_gainc_levels(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
> +                               int ch_num, int coded_subbands,
> +                               ATRAC3PVlcTabs *vlc_tabs, AVCodecContext 
> *avctx)
> +{
> +    int sb, i, delta, delta_bits, min_val, pred;
> +    Atrac3pChanParams *chan     = &ctx->channels[ch_num];
> +    Atrac3pChanParams *ref_chan = &ctx->channels[0];
> +
> +    switch (get_bits(gb, 2)) { /* switch according to coding mode */
> +    case 0: /* fixed-length coding */
> +        for (sb = 0; sb < coded_subbands; sb++)
> +            for (i = 0; i < chan->gain_data[sb].num_points; i++)
> +                chan->gain_data[sb].lev_code[i] = get_bits(gb, 4);
> +        break;
> +    case 1:
> +        if (ch_num) { /* VLC modulo delta to master channel */
> +            for (sb = 0; sb < coded_subbands; sb++)
> +                for (i = 0; i < chan->gain_data[sb].num_points; i++) {
> +                    delta = get_vlc2(gb, vlc_tabs->gain_vlc_tabs[5].table,
> +                                     vlc_tabs->gain_vlc_tabs[5].bits, 1);
> +                    pred = (i >= ref_chan->gain_data[sb].num_points)
> +                           ? 7 : ref_chan->gain_data[sb].lev_code[i];
> +                    chan->gain_data[sb].lev_code[i] = (pred + delta) & 0xF;
> +                }
> +        } else { /* VLC modulo delta to previous */
> +            for (sb = 0; sb < coded_subbands; sb++)
> +                gainc_level_mode1m(gb, ctx, &chan->gain_data[sb], vlc_tabs);
> +        }
> +        break;
> +    case 2:
> +        if (ch_num) { /* VLC modulo delta to previous or clone master */
> +            for (sb = 0; sb < coded_subbands; sb++)
> +                if (chan->gain_data[sb].num_points > 0) {
> +                    if (get_bits1(gb))
> +                        gainc_level_mode1m(gb, ctx, &chan->gain_data[sb],
> +                                           vlc_tabs);
> +                    else
> +                        gainc_level_mode3s(&chan->gain_data[sb],
> +                                           &ref_chan->gain_data[sb]);
> +                }
> +        } else { /* VLC modulo delta to lev_codes of previous subband */
> +            if (chan->gain_data[0].num_points > 0)
> +                gainc_level_mode1m(gb, ctx, &chan->gain_data[0], vlc_tabs);
> +
> +            for (sb = 1; sb < coded_subbands; sb++)
> +                for (i = 0; i < chan->gain_data[sb].num_points; i++) {
> +                    delta = get_vlc2(gb, vlc_tabs->gain_vlc_tabs[4].table,
> +                                     vlc_tabs->gain_vlc_tabs[4].bits, 1);
> +                    pred = (i >= chan->gain_data[sb - 1].num_points)
> +                           ? 7 : chan->gain_data[sb - 1].lev_code[i];
> +                    chan->gain_data[sb].lev_code[i] = (pred + delta) & 0xF;
> +                }
> +        }
> +        break;
> +    case 3:
> +        if (ch_num) { /* clone master */
> +            for (sb = 0; sb < coded_subbands; sb++)
> +                gainc_level_mode3s(&chan->gain_data[sb],
> +                                   &ref_chan->gain_data[sb]);
> +        } else { /* shorter delta to min */
> +            delta_bits = get_bits(gb, 2);
> +            min_val    = get_bits(gb, 4);
> +
> +            for (sb = 0; sb < coded_subbands; sb++)
> +                for (i = 0; i < chan->gain_data[sb].num_points; i++) {
> +                    delta = delta_bits ? get_bits(gb, delta_bits) : 0;
> +                    chan->gain_data[sb].lev_code[i] = min_val + delta;
> +                    if (chan->gain_data[sb].lev_code[i] > 15)
> +                        return AVERROR_INVALIDDATA;
> +                }

This internal operation looks like it can be made a macro or a standalone
function.

> +        }
> +        break;
> +    }
> +
> +    return 0;
> +}
> +
> +
[...]
> diff --git a/libavcodec/atrac3plusdec.c b/libavcodec/atrac3plusdec.c
> new file mode 100644
> index 0000000..45746f2
> --- /dev/null
> +++ b/libavcodec/atrac3plusdec.c
> @@ -0,0 +1,394 @@
> +/*
> + * ATRAC3+ compatible decoder
> + *
> + * Copyright (c) 2010-2013 Maxim Poliakovski
> + *
> + * This file is part of Libav.
> + *
> + * Libav is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * Libav is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> + */
> +
[...]
> +
> +static int decode_frame(AVCodecContext *avctx, void *data,
> +                        int *got_frame_ptr, AVPacket *avpkt)
> +{
> +    const uint8_t *buf  = avpkt->data;
> +    int buf_size        = avpkt->size;
> +    ATRAC3PContext *ctx = avctx->priv_data;
> +    AVFrame *frame      = data;
> +    int i, ret, ch_unit_id, ch_block = 0, out_ch_index = 0, 
> channels_to_process;
> +    float **samples_p = (float **)frame->extended_data;
> +
> +    frame->nb_samples = ATRAC3P_FRAME_SAMPLES;
> +    if ((ret = ff_get_buffer(avctx, frame, 0)) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return ret;
> +    }
> +
> +    init_get_bits(&ctx->gb, buf, buf_size * 8);
> +
> +    if (get_bits1(&ctx->gb)) {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid start bit!\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    while ((ch_unit_id = get_bits(&ctx->gb, 2)) != CH_UNIT_TERMINATOR) {

I'd also check get_bits_left() > 2

> +        if (ch_unit_id == CH_UNIT_EXTENSION) {
> +            avpriv_report_missing_feature(avctx, "Channel unit extension");
> +            return AVERROR_PATCHWELCOME;
> +        }
> +        if (ch_block >= ctx->num_channel_blocks ||
> +            ctx->channel_blocks[ch_block] != ch_unit_id) {
> +            av_log(avctx, AV_LOG_ERROR,
> +                   "Frame data doesn't match channel configuration!\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        ctx->ch_units[ch_block].unit_type = ch_unit_id;
> +        channels_to_process               = ch_unit_id + 1;
> +
> +        if ((ret = ff_atrac3p_decode_channel_unit(&ctx->gb,
> +                                                  &ctx->ch_units[ch_block],
> +                                                  channels_to_process,
> +                                                  &ctx->glob_vlcs, avctx)) < 
> 0)
> +            return ret;
> +
> +        decode_residual_spectrum(&ctx->ch_units[ch_block], ctx->samples,
> +                                 channels_to_process, avctx);
> +        reconstruct_frame(ctx, &ctx->ch_units[ch_block],
> +                          channels_to_process, avctx);
> +
> +        for (i = 0; i < channels_to_process; i++)
> +            memcpy(samples_p[out_ch_index + i], ctx->outp_buf[i],
> +                   ATRAC3P_FRAME_SAMPLES * sizeof(**samples_p));
> +
> +        ch_block++;
> +        out_ch_index += channels_to_process;
> +    }
> +
> +    *got_frame_ptr = 1;
> +
> +    return avctx->block_align;
> +}
> +
> +static av_cold int decode_close(AVCodecContext *avctx)
> +{
> +    ATRAC3PContext *ctx = avctx->priv_data;
> +
> +    ff_atrac3p_free_vlcs(&ctx->glob_vlcs);
> +
> +    return 0;
> +}
> +
> +AVCodec ff_atrac3p_decoder = {
> +    .name           = "atrac3plus",
> +    .long_name      = NULL_IF_CONFIG_SMALL("ATRAC3+ (Adaptive TRansform 
> Acoustic Coding 3+)"),
> +    .type           = AVMEDIA_TYPE_AUDIO,
> +    .id             = AV_CODEC_ID_ATRAC3P,
> +    .priv_data_size = sizeof(ATRAC3PContext),
> +    .init           = decode_init,
> +    .close          = decode_close,
> +    .decode         = decode_frame,
> +};
[...]

the rest doesn't raise functional remarks, only stylistical ones
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to