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

[wm4]

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:  
> >>> On Fri, 16 Dec 2016 03:32:07 +0100
> >>> Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
> >>>     
> >>>> Suggested-by: Rodger Combs <rodger.combs at gmail.com>
> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >>>> ---
> >>>>  libavutil/common.h | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/libavutil/common.h b/libavutil/common.h
> >>>> index 8142b31..00b7504 100644
> >>>> --- a/libavutil/common.h
> >>>> +++ b/libavutil/common.h
> >>>> @@ -99,6 +99,8 @@
> >>>>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
> >>>>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
> >>>>  
> >>>> +#define FF_BAIL_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, "Overflow check failed: " #x"\n"); return AVERROR_INVALIDDATA;}
> >>>> +
> >>>>  /* misc math functions */
> >>>>  
> >>>>  #ifdef HAVE_AV_CONFIG_H    
> >>>
> >>> 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, 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, since overflowing checks are
annoying and there's also a chance to get them wrong. The code gets
less ugly too. 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.

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