[FFmpeg-devel] [RFC v2 3/3] daaladec: Implement a native Daala decoder

[Andreas Cadhalpun]

Hi,

On 29.12.2015 17:15, Ronald S. Bultje wrote:
> On Tue, Dec 29, 2015 at 11:07 AM, Andreas Cadhalpun <
> andreas.cadhalpun at googlemail.com> wrote:
> 
>> On 29.12.2015 16:58, Ronald S. Bultje wrote:
>>> On Tue, Dec 29, 2015 at 10:55 AM, Andreas Cadhalpun <
>>> andreas.cadhalpun at googlemail.com> wrote:
>>>
>>>>> +static av_always_inline void idct_1D_8(pixel *x, int xstride, const
>>>> pixel y[16])
>>>>> +{
>>>>> +    int t1h, t4h, t6h, t0 = y[0], t1 = y[1], t2 = y[2], t3 = y[3], t4
>> =
>>>> y[4];
>>>>> +    int t5 = y[5], t6 = y[6], t7 = y[7];
>>>>> +    t5 -= (t3*2485 + 4096) >> 13;
>>>>> +    t3 += (t5*18205 + 16384) >> 15;
>>>>> +    t5 -= (t3*2485 + 4096) >> 13;
>>>>> +    t7 -= (t1*3227 + 16384) >> 15;
>>>>> +    t1 += (t7*6393 + 16384) >> 15;
>>>>> +    t7 -= (t1*3227 + 16384) >> 15;
>>>>> +    t1 += t3;
>>>>
>>>> These seven lines can overflow.
>>>
>>>
>>> Why do you believe they can overflow?
>>
>> Because ubsan told me that.
>>
>>> Look at range constraints for the input values.
>>
>> Apparently that is not constrained enough, e.g.:
>> t3 = -1449866
>> t3*2485 = -3602917010 < INT32_MIN
> 
> 
> So what type is a pixel?

#define pixel int32_t

> Isn't a pixel uint8_t?

No.

> And coefs are typically constrained alike. Or are we talking 12bpp content here?

I don't know.

> In that case, you likely need 64bit integers for 15bit math precision (look at
> vp9 code), or daala needs to reduce precision (as does hevc).

Yes, either the intermediate calculation needs to happen with 64bit integers,
or the input has to be sanitized in some way.

Best regards,
Andreas