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

[Nedeljko Babic]

Hi again,

>>          if (downmix_output) {
>> +#if CONFIG_AC3_FIXED
>> +            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 
correctly.

>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.

-Nedeljko