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

[Christophe Gisquet]

Hi

here's a more specific review, that probably looks at very secondary details.

2013/2/28 Nedeljko Babic <nbabic at mips.com>:
>          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,

[...]

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

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.

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

> +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.
Also, quite a lot less important, but the input is probably 32bits, so
there may be wasteful sign extension occurring here.

> +        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)?

--
Christophe