[FFmpeg-devel] [PATCH] ALS decoder

[Thilo Borgmann]

>> +    // allocate quantized parcor coefficient buffer
>> +    if (!(ctx->quant_cof = av_malloc(sizeof(int64_t) * sconf->max_order)) ||
>> +        !(ctx->lpc_cof = av_malloc(sizeof(int64_t) * sconf->max_order))) {
> 
> sizeof(*ctx->lpc_cof)
Ok and some others also.


>> +static void parse_bs_info(uint32_t bs_info, unsigned int n, unsigned int div,
>> +                          unsigned int **div_blocks, unsigned int *num_blocks)
>> +{
> 
>> +    if (n < 31 && ((bs_info >> (30 - n)) & 1)) {
>> +        // if the level is valid and the investigated bit n is set
>> +        // then recursively check both children at bits (2n+1) and (2n+2)
>> +        n   *= 2;
>> +        div += 1;
> 
> this sounds like it could use get_bits()
Have I missed get_bits() supports random access?
As far as I can see, using get_bits() for the desired parsing would
require a copy of the GetBitContext for each iterative call an this
would be way too much overhead, I think. Am I wrong?


>> +/** Reads and decodes a Rice codeword.
>> + */
>> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
>> +{
>> +    int     max = gb->size_in_bits - get_bits_count(gb) - k;
>> +
>> +    if (k > 1) {
> [...]
>> +        q         = get_unary(gb, 0, max);
> [...]
>> +    } else {
>> +        int q;
>> +        q = get_unary(gb, 0, max);
> [...]
> 
> please factorize this out, also please factorize anything else out
> if there is something that can be factored out
I've already had a look at this function looking for things to be
factored out. If there is anything left, please tell me explicitly. In
the case of q - it is of different type in the branches. How to factor
that out?


>> +/** Converts PARCOR coefficient k to direct filter coefficient.
>> + */
>> +static void parcor_to_lpc(unsigned int k, int64_t *par, int64_t *cof)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < (k+1) >> 1; i++) {
>> +        int32_t tmp1, tmp2;
>> +
>> +        tmp1 = cof[    i    ] + ((par[k] * cof[k - i - 1] + (1 << 19)) >> 20);
>> +        tmp2 = cof[k - i - 1] + ((par[k] * cof[    i    ] + (1 << 19)) >> 20);
>> +        cof[k - i - 1] = tmp2;
>> +        cof[    i    ] = tmp1;
> 
> as the intermediate tmp* are 32bit so likely can be cof, making
> multiplications faster
Justin just noted that the multiplication has to be 64-bit and I share
his opinion - if there is something beyond 32-bit here and the stream is
not broken, it is within "((par[k] * cof[...] + (1 << 19))" before it is
shifted right for 20 bits.


>> +static void reconstruct_block_sizes(ALSDecContext *ctx, uint32_t *div_blocks)
>> +{
>> ...
>> +    if (ctx->cur_frame_length == ctx->last_frame_length) {
>> +        unsigned int remaining = ctx->cur_frame_length;
>> +
>> +        for (b = 0; b < ctx->num_blocks; b++) {
>> +            div_blocks[b] = ctx->sconf.frame_length >> div_blocks[b];
> [...]
>> +        }
>> +    } else {
>> +        for (b = 0; b < ctx->num_blocks; b++)
>> +            div_blocks[b] = ctx->sconf.frame_length >> div_blocks[b];
> 
> duplicate
Done.



>> +/** Reads the block data for a non-constant block
>> + */
>> +static int read_var_block(ALSDecContext *ctx, unsigned int ra_block,
>> +                          int64_t *raw_samples, unsigned int block_length,
>> +                          unsigned int *js_blocks, int64_t *raw_other,
>> +                          unsigned int *shift_lsbs)
>> +{
>> ...
>> +            } else {
>> +                int offset, rice_param, k_max;
>> +
> 
>> +                // read coefficient 0
>> +                offset       = parcor_rice_table[sconf->coef_table][0][0];
>> +                rice_param   = parcor_rice_table[sconf->coef_table][0][1];
>> +                quant_index  = decode_rice(gb, rice_param) + offset;
>> +                quant_cof[0] = parcor_scaled_values[quant_index + 64];
>> +
>> +                // read coefficient 1
>> +                offset       = parcor_rice_table[sconf->coef_table][1][0];
>> +                rice_param   = parcor_rice_table[sconf->coef_table][1][1];
>> +                quant_index  = decode_rice(gb, rice_param) + offset;
>> +                quant_cof[1] = -parcor_scaled_values[quant_index + 64];
>> +
>> +                // read coefficients 2 to 19
>> +                k_max = FFMIN(20, opt_order);
>> +                for (k = 2; k < k_max; k++) {
>> +                    offset       = parcor_rice_table[sconf->coef_table][k][0];
>> +                    rice_param   = parcor_rice_table[sconf->coef_table][k][1];
>> +                    quant_index  = decode_rice(gb, rice_param) + offset;
>> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);
>> +                }
> 
> the 3 first lines of these 3 blocks of code are duplicated and can be factorized
Done.



>> +static int read_block_data(ALSDecContext *ctx, unsigned int ra_block,
>> +                            int64_t *raw_samples, unsigned int block_length,
>> +                            unsigned int *js_blocks, int64_t *raw_other)
>> +{
>> ...
>> +    unsigned int block_type;
>> +    unsigned int k;
>> +
>> +    block_type = get_bits1(gb);
>> +
>> +    if (block_type == 0) {
> 
> the temporary variable block_type is unneeded
Done.



>> +/** Decodes blocks independently.
>> + */
>> +static int decode_blocks_ind(ALSDecContext *ctx, unsigned int ra_frame,
>> +                             unsigned int c, unsigned int *div_blocks,
>> +                             unsigned int *js_blocks)
>> +{
>> +    ALSSpecificConfig *sconf = &ctx->sconf;
>> +    int64_t *raw_sample;
>> +    unsigned int b, ra_block;
>> +    raw_sample = ctx->raw_samples[c];
>> +
>> +    for (b = 0; b < ctx->num_blocks; b++) {
>> +        ra_block = !b && ra_frame;
> 
> id use ra_frame rename it to ra_block and set it to 0 at the end of the loop
> this simplification can likely be done for more code
Done here and in decode_blocks().




>> +static int decode_blocks(ALSDecContext *ctx, unsigned int ra_frame,
>> +                         unsigned int c, unsigned int *div_blocks,
>> +                         unsigned int *js_blocks)
>> +{
>> ...
>> +    // store carryover raw samples
>> +    memmove((ctx->raw_samples[c]) - sconf->max_order,
>> +            (ctx->raw_samples[c]) - sconf->max_order + sconf->frame_length,
>> +            sizeof(int64_t) * sconf->max_order);
>> +
>> +    memmove((ctx->raw_samples[c + 1]) - sconf->max_order,
>> +            (ctx->raw_samples[c + 1]) - sconf->max_order + sconf->frame_length,
>> +            sizeof(int64_t) * sconf->max_order);
>> +
>> +    return 0;
>> +}
> 
> this looks similar to decode_blocks_ind() i guess some parts could be
> factored
The "c++;" in the else branch of the calling function prevents this to
be done elegantly. Thus it would require another (duplicated) if to
factor the memmove()'s out of the functions.



>> +static av_cold int decode_init(AVCodecContext *avctx)
>> ...
>> +    if (sconf->floating) {
>> +        avctx->sample_fmt          = SAMPLE_FMT_FLT;
> 
>> +        avctx->bits_per_raw_sample = 32;
> 
> why is this not set to 24 for simplifying that if(), i dont think
> bits_per_raw_sample has a meaning currently for floats but maybe i
> forgot something
I dont't care. Just consolidate yourselves.


All changes will be part of revision 7.

Thanks for that review!

-Thilo