[FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Andreas Cadhalpun]

On 21.12.2016 13:46, wm4 wrote:
> On Wed, 21 Dec 2016 01:43:46 +0100
> Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
>> On 20.12.2016 15:22, wm4 wrote:
>>> On Mon, 19 Dec 2016 23:36:11 +0100
>>> Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
>>>> On 16.12.2016 17:22, wm4 wrote:  
>>>>> Are you sure we need the message?    
>>>>
>>>> Yes, since such an overflow could just be a sign of a limitation in our
>>>> framework (think of bit_rate being int32_t) and does not necessarily mean
>>>> that the sample is invalid.
>>>>  
>>>>> It's quite ugly.    
>>>>
>>>> Do you have any suggestions for improving it?  
>>>
>>> I'm pretty much against such macros for rather specific use-cases, and
>>> putting them into a public headers.  
>>
>> It is added to an "internal and external API header".
>> Feel free to send patches separating it into an internal and a public header.
> 
> Macros starting with FF are public API,

No, public macros start with AV.

> so don't put that macro in a public header. Or we're stuck with it forever.
> 
>>> I'm thinking it'd be better to actually provide overflow-checking primitives,  
>>
>> Why?
> 
> Because that would have actual value,

That's a meaningless argument.

> since overflowing checks are annoying

Using overflow-checking primitives is even more annoying.

> and there's also a chance to get them wrong.

That's always the case with anything.

> The code gets less ugly too.

Rather the contrary. Compare
        FF_RETURN_ON_OVERFLOW(s, ea->bytes > INT_MAX / 8 / 2)
        st->codecpar->bits_per_coded_sample = ea->bytes * 8;
        st->codecpar->bit_rate              = (int64_t)st->codecpar->channels *
                                              st->codecpar->sample_rate *
                                              st->codecpar->bits_per_coded_sample / 4;
        st->codecpar->block_align           = st->codecpar->channels *
                                              st->codecpar->bits_per_coded_sample;
with:
        ret = ff_mul_check_overflow(&st->codecpar->bits_per_coded_sample, ea->bytes, 8)
        if (ret < 0)
            return ret;
        st->codecpar->bit_rate              = (int64_t)st->codecpar->channels *
                                              st->codecpar->sample_rate *
                                              st->codecpar->bits_per_coded_sample / 4;
        ret = ff_mul_check_overflow(&st->codecpar->block_align, st->codecpar->channels, st->codecpar->bits_per_coded_sample)
        if (ret < 0)
            return ret;

> If you're going to add such overflow checks to every
> single operation in libavcodec that could overflow, you better come up
> with something that won't add tons of crap to the code.

Code that overflows is "crap", not the checks preventing that.

>>> and I also don't think every overflow has to be logged.  
>>
>> I disagree for the reason I mentioned above.
> 
> Which was? Not going to read the whole thread again.

Reading either of the mails you replied to would have been sufficient.
You've got a third chance with this mail.

Best regards,
Andreas