[FFmpeg-devel] [PATCH] ALS decoder
Thilo Borgmann
thilo.borgmann
Sun Aug 23 23:31:32 CEST 2009
>> + // 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
More information about the ffmpeg-devel
mailing list