[FFmpeg-devel] [PATCH 2/2] libavcodec: Implementation of AC3 fixed point decoder

Nedeljko Babic Nedeljko.Babic at imgtec.com
Fri Apr 12 15:01:32 CEST 2013

Hi again,

>>          if (downmix_output) {
>> +            ac3_downmix_c_fixed16(s->outptr, s->downmix_coeffs,
>> +                              s->out_channels, s->fbw_channels, 256);
>> +#else
>This should somehow be part of a dsp context. It's all the more
>disturtbing since below, there is:
>> -            s->ac3dsp.downmix(s->xcfptr + 1, s->downmix_coeffs,
>> +            s->ac3dsp.AC3_RENAME(downmix)(s->xcfptr + 1, s->downmix_coeffs,

I didn't want to clog dsp context with this function because it is used only here
but ok, I'll add it to ac3dsp context in next patch.

>> -                memcpy(((float*)frame->data[ch]) + AC3_BLOCK_SIZE*blk, output[ch], 1024);
>> +                memcpy(((SHORTFLOAT*)frame->data[ch]) + AC3_BLOCK_SIZE*blk, output[ch], 1024);
>I thought SHORTFLOAT was int16_t. How come it is the same copy size?
>I'm surprised this overwrite does not crash the decoder.

Depending on flag CONFIG_AC3_FIXED, SHORTFLOAT is either int16_t (for fixed point decoder) or float
(for floating point decoder). Since output is also defined as SHORTFLOAT, memcpy is working 

>On a side note, I guess this memcpy is long enough to have little
>overhead compared to an inline copy loop using eg AV_COPY128.

You are probably correct, but in original implementation (floating point decoder) here is memcpy and 
I left it here in fixed point implementation because it is easier to merge implementations for these 
two decoders (since this is template file for both decoders).
I can change this if you wish, but the change should be made for original (floating point) decoder also.

>> +static void scale_coefs (
>> +    int32_t *dst,
>> +    const int *src,
>> +    int dynrng,
>> +    int len)
>int* pointer? I think the float context has the same issue, but
>really, this should be int32_t too

OK. I'll change this.

>> +static int ac3_fixed_sqrt(int x)
>This is entirely dependent on the FP format, but that could go into
>the Fixed context header (not the struct itself) as a helper function.

I will move it to header.

>Also, quite a lot less important, but the input is probably 32bits, so
>there may be wasteful sign extension occurring here.

I am not sure on which sign extension you are referring

>> +        dst[i] = av_clip_int16_c(((((int64_t)s0*wj - (int64_t)s1*wi + 0x40000000) >> 31) + 0x80) >> 8);
>> +        dst[j] = av_clip_int16_c(((((int64_t)s0*wi + (int64_t)s1*wj + 0x40000000) >> 31) + 0x80) >> 8);
>I'm stuck on this, not really seeing how useful my remark can be, but...
>Sometimes, you have shuffle insns taking chuncks of 8, 16 or 32bits of
>a SIMD register and reordering them. Had the shift been 32, this would
>have been possible.
>Would it be possible to have 1 more bit of precision on the window
>coeffs (and still be usable and fit on 32bits)?

It is not possible because value would be out of range. This is classical fixed point operation.


More information about the ffmpeg-devel mailing list