[FFmpeg-devel] [PATCH] ALS decoder

[Thilo Borgmann]

>> Revision 4 attached.
> [...]
>> +
>> +typedef struct {
>> +    uint32_t als_id;          ///< ALS identifier
> 
>> +    uint32_t samp_freq;       ///< sampling frequency in Hz
> 
> i suspect that is redundant with AVCodecContext.sample_rate
Oh yes it is now... as well as sconf->channels.
Using AVCodecContext fields now for both.


>> +    uint32_t samples;         ///< number of samples (per channel), 0xFFFFFFFF if unknown
> 
> doesnt seem to be needed to be stored in the context
Made local.


>> +    int file_type;            ///< not used, provided for debugging
> 
> if its just for debugging it can also be a local variable
Done.



>> +    int random_access;        ///< distance between RA frames (in frames, 0...255)
> 
> thats the keyframe distance or gop length i assume, it should be named
> accordingly
Done.



>> +    int bgmc_mode;            ///< BGMC Mode: 1 = on, 0 = off (Rice coding only)
> 
> bgmc?
Done.


>> +    int rlslms;               ///< use RLS-LMS predictor: 1 = on, 0 = off
> 
> RLSMLS?
> no i did not read the spec ...
It is correct as it is.


>> +    int aux_data_enabled;     ///< indicates that auxiliary data is present
> 
>> +    int chan_config_info;     ///< mapping of channels to loudspeaker locations
> 
> never read
But a TODO is set to use it in a future release.

>> +    int *chan_pos;            ///< original channel positions
> 
> same
Same.
Do these hurd so much it has to be reinserted later?


>> +    uint32_t crc;             ///< 32-bit CCITT-32 CRC checksum
> 
> and that one as well
Yes. Removed and crc_enabled made local.


> 
> 
> [...]
>> +    // allocate quantized parcor coefficient buffer
>> +    if (!(ctx->quant_cof = av_malloc(sizeof(int64_t) * sconf->max_order))) {
>> +        av_log(ctx->avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    // allocate LPC coefficients
>> +    if (!(ctx->lpc_cof = av_malloc(sizeof(int64_t) * sconf->max_order))) {
>> +        av_log(ctx->avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
>> +        return AVERROR(ENOMEM);
>> +    }
> 
> duplicate and factorizeable code
Factorized.



>> +        if(!(sconf->chan_pos = av_malloc(sconf->channels * sizeof(int))))
>> +            return -1;
> 
> ENOMEM
Done.


>> +    ht_size = sconf->header_size + sconf->trailer_size;
> 
> overflow
Casted explicitly.


>> +    // skip the header and trailer data
>> +    if (buffer_size < ht_size)
>> +        return -1;
>> +
>> +    ht_size <<= 3;
>> +
>> +    while (ht_size > 0) {
>> +        int len = FFMIN(ht_size, INT32_MAX);
>> +        skip_bits_long(&gb, len);
>> +        ht_size -= len;
>> +    }
> 
> i dont think theres sense in supporting skiping of more than 500mb
> as valid packets should not have such sizes
This is done once per stream. Does the loop hurts so much to limit
ourselves to 32-bit?


>> +/** Parses the bs_info item to extract the block partitioning.
>> + */
> 
> without having read the spec, that says nothing to me ...
> can this be clarified in 1 sentance or is the reader required to have read
> the spec?
Reworded a bit. But I think if block partitioning is unclear, a much
better comment would be beyond one sentence and explains block switching.



>> +/** Reads and decodes a Rice codeword.
>> + */
>> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
>> +{
>> ...
>> +}
> 
> theres some obvious factorizeable code in there
Refactorized as far as I can see.
I rebenchmarked and it still needs around 760 dezicykles.
The skip rate sunk to a fifth, if this indicates something?


>> +/** Converts PARCOR coefficient k to direct filter coefficient.
>> + */
>> +static void parcor_to_lpc(unsigned int k, int64_t *par, int64_t *cof)
>> +{
>> +    int i;
>> +    int64_t tmp1, tmp2;
>> +
>> +    for (i = 0; i < (k+1) >> 1; i++) {
>> +        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;
>> +    }
>> +
>> +    cof[k] = par[k];
>> +}
> 
> doesnt look entirely unfamiliar, dont some of our other audio codecs have
> something that can be used? (no i dont know which, i would have to RTFS too)
RTFS for me too and RTFM about other audio codecs... anyone?



>> +/** Reads the block data for a constant block
>> + */
>> +static void read_const_block(ALSDecContext *ctx, int64_t *raw_samples,
>> +                             unsigned int block_length, unsigned int *js_blocks)
>> +{
>> ...
>> +    // skip 5 reserved bits
>> +    skip_bits(gb, 5);
> 
> if these have a required value like 0 that should be checked, always usefull
> to know early if something unexpected turns up
Not the case here.


> 
> 
>> +
>> +    if (const_block) {
>> +        unsigned int const_val_bits;
>> +
>> +        if (sconf->resolution == 2 || sconf->floating)
>> +            const_val_bits = 24;
>> +        else
>> +            const_val_bits = avctx->bits_per_raw_sample;
> 
> why would const_val_bits != avctx->bits_per_raw_sample ?
bits_per_raw_sample = 32 for floating sconf->floating.



>> +/** 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)
>> +{
>> ...
>> +    // determine the number of sub blocks for entropy decoding
>> +    if (!sconf->bgmc_mode && !sconf->sb_part)
>> +        sub_blocks = 1;
>> +    else if (sconf->bgmc_mode && sconf->sb_part)
>> +        sub_blocks = 1 << get_bits(gb, 2);
>> +    else
>> +        sub_blocks = get_bits1(gb) ? 4 : 1;
> 
> id write 1 << (2*get_bits1(gb))
> because its more simlar to the others
I like that, too. Done.



>> +    if (!sconf->bgmc_mode) {
>> +        s[0] = get_bits(gb, (sconf->resolution > 1) ? 5 : 4);
>> +        for (k = 1; k < sub_blocks; k++)
>> +            s[k] = s[k - 1] + decode_rice(gb, 0);
>> +    } else {
>> +        // TODO: BGMC mode
>> +    }
> 
> if(x)
> else
> 
> is imho clearer than
> 
> if(!x)
> else
Done.



>> +                quant_index = get_bits(gb, 7) - 64;
>> +                quant_cof[0] = parcor_scaled_values[quant_index + 64];
> 
> -64 + 64 ?
>> +                quant_index = get_bits(gb, 7) - 64;
>> +                quant_cof[1] = -parcor_scaled_values[quant_index + 64];
> 
> same
Done.



>> +                // read coefficients 2 to opt_order
>> +                for (k = 2; k < opt_order; k++) {
>> +                    quant_index = get_bits(gb, 7) - 64;
>> +                    quant_cof[k] = (quant_index << 14) + (1 << 13);
> 
> the +- can be combined
Sorry I can't see that..



>> +                k_max = FFMIN(20, opt_order);
>> +                for (k = 2; k < k_max; k++) {
>> ...
>> +                }
>> ...
>> +                k_max = FFMIN(127, opt_order);
>> +                for (k = 20; k < k_max; k++) {
> 
> k=20 is redundant
> 
> 
>> +                // read coefficients 127 to opt_order
>> +                for (k = 127; k < opt_order; k++) {
> 
> so is k=127
No, this is correct.
 for(; k < 20;)
 for(k = 20;;)



>> +            memcpy(ctx->prev_raw_samples, raw_samples - sconf->max_order,
>> +                   sizeof(int64_t) * sconf->max_order);
> 
> instead of duplicating the type (int64_t) the type of the array should be used
Sorry I don't understand this. Please elaborate.


>> +static int decode_frame(AVCodecContext *avctx,
>> +                        void *data, int *data_size,
>> +                        AVPacket *avpkt)
>> +{
>> ...
>> +    ra_frame = sconf->random_access && ((!ctx->frame_id) ||
>> +               !(ctx->frame_id % sconf->random_access));
> 
> the !ctx->frame_id condition seems unneeded
Indeed. Done.



>> +    // allocate previous raw sample buffer
>> +    if (!(ctx->prev_raw_samples = av_malloc(sizeof(int64_t) * sconf->max_order))) {
>> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
>> +        decode_end(avctx);
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    // allocate raw and carried sample buffer
>> +    if (!(ctx->raw_buffer = av_mallocz(sizeof(int64_t) *
>> +                                       avctx->channels * channel_size))) {
>> +        av_log(avctx, AV_LOG_ERROR, "Allocating buffer memory failed.\n");
>> +        decode_end(avctx);
>> +        return AVERROR(ENOMEM);
>> +    }
> 
> factorizeable with a goto
If you don't veto on a or'd if, I would prefer that.



>> Index: libavutil/common.h
>> ===================================================================
>> --- libavutil/common.h	(revision 19673)
>> +++ libavutil/common.h	(working copy)
>> @@ -225,6 +225,14 @@
>>      else               return a;
>>  }
>>  
>> +/** Computes ceil(log2(x)) using av_log2.
>> + * @param x value used to compute ceil(log2(x))
>> + * @param computed ceiling of log2(x)
>> + */
>> +static inline int ceil_log2(int x) {
>> +    return x > 1 ? av_log2((x - 1) << 1) : 0;
>> +}
>> +
>>  #define MKTAG(a,b,c,d) (a | (b << 8) | (c << 16) | (d << 24))
>>  #define MKBETAG(a,b,c,d) (d | (c << 8) | (b << 16) | (a << 24))
>>  
> 
> should be a seperate patch
Ok.


All changes included in revision 5.

Thanks!

-Thilo