[FFmpeg-devel] [PATCH] ALS decoder
Thilo Borgmann
thilo.borgmann
Sat Aug 29 01:04:27 CEST 2009
Michael Niedermayer schrieb:
> On Wed, Aug 26, 2009 at 08:28:43PM +0200, Thilo Borgmann wrote:
>> Revision 11 attached.
> [...]
>> +/** Reads an ALSSpecificConfig from a buffer into the output struct.
>> + */
>> +static av_cold int read_specific_config(ALSDecContext *ctx)
>> +{
>> + GetBitContext gb;
>> + uint64_t ht_size;
>> + int i, config_offset, crc_enabled;
>> + MPEG4AudioConfig m4ac;
>> + ALSSpecificConfig *sconf = &ctx->sconf;
>> + AVCodecContext *avctx = ctx->avctx;
>> + const uint8_t *buffer = avctx->extradata;
>> + uint32_t samples, als_id;
>> +
>> + init_get_bits(&gb, buffer, avctx->extradata_size * 8);
>> +
>> + config_offset = ff_mpeg4audio_get_config(&m4ac, buffer, avctx->extradata_size);
>> +
>> + if (config_offset < 0)
>> + return -1;
>> +
>> + skip_bits_long(&gb, config_offset);
>> +
>> + if (get_bits_left(&gb) < (22 << 3))
>> + return -1;
>
> is that the smallest size a valid frame can have? i would have suspect it
> to be larger
I don't know. Is the AudioSpecificConfig embedded in something like a
frame? I don't think there is a minimum size for AudioSpecificConfig and
determining this by the smallest possible *SpecificConfig would be
error-prone and would have to be reconsidered whenever 14496-3 changes.
> Now if it where larger, several of the following checks could be removed
These also prevent from reading beyond a broken stream and are
one-timers for decoding a whole stream and therefore speed is not
relevant, I think.
>
>
> [...]
>> +/** Reads and decodes a Rice codeword.
>> + */
>> +static int32_t decode_rice(GetBitContext *gb, unsigned int k)
>> +{
>> + int max = gb->size_in_bits - get_bits_count(gb) - k;
>> + int32_t q = get_unary(gb, 0, max);
>
> i suspect int is fine for these 2 instead of int32_t ?
Is int at least 32 bits long? k <= 32 and therefore q might have to hold
up to 32 bits.
>> +{
>> + int i;
>> +
>> + for (i = 0; i < (k + 1) >> 1; i++) {
>
>> + int64_t tmp1 = cof[ i ] + ((MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20);
>> + int64_t tmp2 = cof[k - i - 1] + ((MUL64(par[k], cof[ i ]) + (1 << 19)) >> 20);
>> +
>> + if (tmp1 < -INT32_MAX || tmp1 > INT32_MAX ||
>> + tmp2 < -INT32_MAX || tmp2 > INT32_MAX)
>> + return -1;
>> +
>> + cof[k - i - 1] = tmp2;
>> + cof[ i ] = tmp1;
>
> if we didnt do overflow checks ...
>
> int tmp1 = (MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20;
> cof[k - i - 1] += (MUL64(par[k], cof[ i ]) + (1 << 19)) >> 20;
> cof[ i ] += tmp1;
I agree mathematically, but this doesn't work - most likely because of
the += operator. Used the previous no-check version (no tmp2).
>
> also, you could try: (maybe its faster
>
> int64_t tmp1 = cof[ i ] + ((MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20);
> int64_t tmp2 = cof[k - i - 1] + ((MUL64(par[k], cof[ i ]) + (1 << 19)) >> 20);
>
> cof[k - i - 1] = tmp2;
> cof[ i ] = tmp1;
>
> if( cof[k - i - 1] != tmp2
> || cof[ i ] != tmp1)
> return -1;
Benchmarked to need only 75% of the decizycles as before. But as we all
think the check is unneeded in the decoder...
>> +/** Reads the block data for a non-constant block
>> + */
>> +static int read_var_block(ALSDecContext *ctx, unsigned int ra_block,
>> + int32_t *raw_samples, unsigned int block_length,
>> + unsigned int *js_blocks, int32_t *raw_other,
>> + unsigned int *shift_lsbs)
>> +{
>> ...
>> + if (opt_order) {
>> + if (sconf->coef_table == 3) {
>> + // read coefficient 0
>> + quant_cof[0] = parcor_scaled_values[get_bits(gb, 7)];
>> +
>> + // read coefficient 1
>> + quant_cof[1] = -parcor_scaled_values[get_bits(gb, 7)];
>> +
>> + // read coefficients 2 to opt_order
>> + for (k = 2; k < opt_order; k++)
>> + quant_cof[k] = (get_bits(gb, 7) << 14) - (0x7F << 13);
>> + } else {
>> + int offset, rice_param, k_max;
>> + int64_t quant_index;
>> +
>> +
>> + // read coefficient 0 to 19
>> + k_max = FFMIN(20, opt_order);
>> + for (k = 0; k < k_max; k++) {
>> + offset = parcor_rice_table[sconf->coef_table][k][0];
>> + rice_param = parcor_rice_table[sconf->coef_table][k][1];
>> + quant_cof[k] = decode_rice(gb, rice_param) + offset;
>> + }
>> +
>> + quant_cof[0] = parcor_scaled_values[quant_cof[0] + 64];
>> + quant_cof[1] = -parcor_scaled_values[quant_cof[1] + 64];
>> +
>> + for (k = 2; k < k_max; k++)
>> + quant_cof[k] = (quant_cof[k] << 14) + (1 << 13);
>> +
>> + // read coefficients 20 to 126
>> + k_max = FFMIN(127, opt_order);
>> + for (; k < k_max; k++) {
>> + offset = k & 1;
>> + rice_param = 2;
>> + quant_index = decode_rice(gb, rice_param) + offset;
>> + quant_cof[k] = (quant_index << 14) + (1 << 13);
>> + }
>> +
>> + // read coefficients 127 to opt_order
>> + for (; k < opt_order; k++) {
>> + offset = 0;
>> + rice_param = 1;
>> + quant_index = decode_rice(gb, rice_param) + offset;
>> + quant_cof[k] = (quant_index << 14) + (1 << 13);
>> + }
>
> // read coefficient 0 to 19
> k_max = FFMIN( 20, opt_order);
> for (k = 0; k < k_max; k++) {
> int offset = parcor_rice_table[sconf->coef_table][k][0];
> int rice_param = parcor_rice_table[sconf->coef_table][k][1];
> quant_cof[k] = decode_rice(gb, rice_param) + offset;
> }
>
> // read coefficients 20 to 126
> k_max = FFMIN(127, opt_order);
> for (; k < k_max; k++)
> quant_cof[k] = decode_rice(gb, 2) + (k&1);
>
> // read coefficients 127 to opt_order
> for (; k < opt_order; k++)
> quant_cof[k] = decode_rice(gb, 1);
>
> quant_cof[0] = parcor_scaled_values[quant_cof[0] + 64];
> quant_cof[1] = -parcor_scaled_values[quant_cof[1] + 64];
>
> for (k = 2; k < opt_order; k++)
> quant_cof[k] = (quant_cof[k] << 14) + (1 << 13);
It has to be for(..; k < k_max; ...) with k_max = FFMIN(20, opt_order)...
>
>
> and this likely can be factored with the sconf->coef_table == 3 case
... thus it cannot be merged with sconf->... == 3 which uses the whole k
< opt_oder range.
Just shifting it behind the read coefficients xx to yy would require an
extra call to FFMIN().
>
> [...]
>> +/** Computes the bytes left to decode for the current frame
>> + */
>> +static void zero_remaining(unsigned int b, unsigned int b_max,
>> + unsigned int *div_blocks, int32_t *buf)
>> +{
>
> div_blocks should be const, also this likel applies to other arguments of
> other functions
Are there some common FFmpeg-wide rules to declare something const?
I've done what I think might be useful...
>> + for (b = 0; b < ctx->num_blocks; b++) {
>> + ra_block = !b && ra_frame;
>
> ra_block= ra_frame;
> for(){
> ...
> ra_block= 0;
> }
Shame on me! Done.
Thanks!
-Thilo
More information about the ffmpeg-devel
mailing list