[FFmpeg-devel] [PATCH] ALS decoder
Måns Rullgård
mans
Sat Aug 22 11:30:11 CEST 2009
Thilo Borgmann <thilo.borgmann at googlemail.com> writes:
>>> + // read the fixed items
>>> + sconf->als_id = get_bits_long(&gb, 32);
>>> + sconf->samp_freq = m4ac.sample_rate;
>>> + skip_bits_long(&gb, 32);
>>
>> Maybe comment what is being skipped here and elsewhere.
> Ok, I thought it is obvious. Done.
If you're reading the spec with the other eye, yes.
>>> + sconf->crc_enabled = get_bits1(&gb);
>>> + sconf->rlslms = get_bits1(&gb);
>>> + skip_bits(&gb, 5); // skip 5 reserved bits
>>
>> Please lose some of that whitespace. Compliments on the alignment
>> though. Diego will love you.
> I did this above, but here, not even the 80-characters reason exist.
> Why give up readability?
I was referring to the spaces before the comment on the last line.
The alignment of the assignments is very nice.
>>> +/** Reads and decodes a Rice codeword.
>>> + */
>>> +static int64_t decode_rice(GetBitContext *gb, unsigned int k)
>>> +{
>>> + int64_t value = 0;
>>> + int64_t q = 0;
>>> + int max = gb->size_in_bits - get_bits_count(gb) - k;
>>> +
>>> + if (!k) {
>>> + q = get_unary(gb, 0, max);
>>> + return (q & 1) ? -((q + 1) >> 1) : ((q + 1) >> 1);
>>> + } else if (k == 1) {
>>> + q = get_unary(gb, 0, max);
>>> + return get_bits1(gb) ? q : -(q + 1);
>>> + } else {
>>> + unsigned int r, sub_sign;
>>> +
>>> + q = get_unary(gb, 0, max);
>>> + sub_sign = get_bits1(gb);
>>> + r = get_bits_long(gb, k - 1);
>>> +
>>> + value = (q << (k - 1)) + r;
>>> +
>>> + return sub_sign ? value : -(value + 1);
>>> + }
>>> +}
>>
>> This function looks like it was designed specifically to make gcc fail:
>>
>> 1. 64-bit variables used unnecessarily.
>> 2. GCC hates complex conditionals.
>> 3. Compilers in general are bad at bit-hacks.
>>
>> It seems to me that int is enough for 'q' in the first two cases.
>> get_unary() returns int, so we start with at most that many bits. To
>> avoid overflow in q+1, do this instead:
>>
>> int r = q & 1;
>> return r ? -((q >> 1) + r) : ((q >> 1) + r);
>>
>> Furthermore, gcc does stupid things with that code even with plain int
>> variables. Rewriting in a few more steps helps massively:
>>
>> int r = q & 1;
>> q = (q >> 1) + r;
>> return r ? -q : q;
>>
>> This reduces the number of instructions on ARM from 6 to 4 (from 4 to
>> 3 with armcc). On MIPS it goes from 9 with a branch to 5 branch-free.
>>
>> Finally, -(v + 1) is equivalent to ~v on two's complement machines,
>> which we have previously agreed to assume we have. Hence we should
>> write the return values in the second and third cases like this:
>>
>> return sign ? value : ~value;
>>
>> This prevents 32-bit overflow in the k==1 case and gives better code
>> with several compilers in the final case, where we must resort to
>> 64-bit maths.
> Tricky. Works like a charm. Done.
Did it get any faster?
>>> + // reconstruct difference signal for prediction (joint-stereo)
>>> + if (*js_blocks && raw_other) {
>>> + int i;
>>> + if (raw_other > raw_samples) { // D = R - L
>>> + for (i = -1; i >= -sconf->max_order; i--)
>>> + raw_samples[i] = raw_other[i] - raw_samples[i];
>>> + } else { // D = R - L
>>
>> Are those two comments meant to be the same?
> Yes but semantically different. This is meant to be that way.
> It shows, which channel is represented by which variable.
I guess that subtlety was lost on me, although I did understand the
code...
>>> + // 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);
>>> + }
>>> +
>>> + // allocate raw sample array buffer
>>> + if (!(ctx->raw_samples = av_malloc(sizeof(int64_t*) * avctx->channels))) {
>>> + av_log(avctx, AV_LOG_ERROR, "Allocating buffer array failed.\n");
>>> + decode_end(avctx);
>>> + return AVERROR(ENOMEM);
>>> + }
>>
>> This looks very repetitive...
> Thus.... it is good/bad/crazy?
> The log message depends on the buffer allocated, the size of the buffer
> is never the same...
The messages look the same to me...
>>> + // allocate raw and carried samples buffers
>>> + ctx->raw_samples[0] = ctx->raw_buffer + sconf->max_order;
>>> + for (c = 1; c < avctx->channels; c++) {
>>> ...
>>> +
>>> Index: libavcodec/als_data.h
>>> ===================================================================
>>> --- libavcodec/als_data.h (revision 0)
>>> +++ libavcodec/als_data.h (revision 0)
>>> @@ -0,0 +1,147 @@
>>> +/*
>>> + * ALS header file for common data
>>> + * Copyright (c) 2009 Thilo Borgmann <thilo.borgmann _at_ googlemail.com>
>>> ...
>>> +#ifndef AVCODEC_ALS_DATA_H
>>> +#define AVCODEC_ALS_DATA_H
>>> +
>>> +
>>> +#include <stdint.h>
>>> +
>>> +/** Rice parameters and corresponding index offsets for decoding the
>>> + * indices of scaled PARCOR values. The table choosen is set globally
>>> + * by the encoder and stored in ALSSpecificConfig.
>>> + */
>>> +int8_t parcor_rice_table[3][20][2] = {
>>> + {
>>> + {-52, 4},
>>
>> WTF happened to the indentation here?
I usually do something like this:
int8_t parcor_rice_table[3][20][2] = {
{ { -52, 4 }, { xx, yy }, { zz, ww }, { ... }
/* ... */
{ ... }, { ... }, { ... }, { ... } },
};
>>> +/** Scaled PARCOR values used for the first two PARCOR coefficients.
>>> + * To be indexed by the Rice coded indices.
>>> + * Generated by: parcor_scaled_values[i] = 32 + ((i * (i+1)) << 7) - (1 << 20)
>>> + */
>>> +int32_t parcor_scaled_values[] = {-1048544, -1048288, -1047776, -1047008,
>>> + -1045984, -1044704, -1043168, -1041376,
>>
>> And here.
> Would you like block- or function-like indents?
> No kidding - I ask that before it goes back and forth and I don't care
> which one...
> (Or are there table-like indention rules I've missed?)
Here I'd do like this:
int32_t parcor_scaled_values[] =
-1048544, -1048288, -1047776, -1047008,
};
--
M?ns Rullg?rd
mans at mansr.com
More information about the ffmpeg-devel
mailing list