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

[Nedeljko Babic]

Hi,

>Hi,
>
>I just wanted to mention that I'm not someone of charge of anything
>for ffmpeg: I just wanted to look at a FP implementation.

I am thankful for your comments whether you are in charge or not.

>>>> -                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.
>
>Yes but output[ch] is AC3_BLOCK_SIZE elements, ie 256. The 1024 is
>fine for 256*sizeof(float), but not here: you're overreading and
>overwriting the buffers. This 1024 should be changed to
>AC3_BLOCK_SIZE*sizeof(SHORTFLOAT).
>
>And answering myself, you didn't notice it because this happens only
>on error recovery, and you are probably not using corrupted streams.
>

You are correct and I'll fix it in the next patch.

>>>> +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.
>
>I don't know what naming convention we could have, but maybe naming it
>fp12_fixed_sqrt(x) would be suited to match the format rather than
>where it is used?
>
>Someone else should comment on this FP DSP design.

I don't have a problem with your suggestion, so unless someone gives
a remark on this, I will use it in the next patch


Thanks
-Nedeljko