[FFmpeg-devel] [PATCH 1/2] avutil/internal: Add ff_elog()

[wm4]

On Thu, 23 Feb 2017 07:57:41 -0500
"Ronald S. Bultje" <rsbultje at gmail.com> wrote:

> Hi,
> 
> On Thu, Feb 23, 2017 at 2:19 AM, wm4 <nfxjfg at googlemail.com> wrote:
> 
> > On Wed, 22 Feb 2017 19:16:46 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >  
> > > This enables the extra error messages in case of DEBUG or high assrtion  
> > levels.  
> > > High assertion levels imply slow checks in inner loops so any extra  
> > error should  
> > > be insignificant.
> > > Is it preferred to have a separate switch for ff_elog() so it doesnt  
> > depend on  
> > > DEBUG/assertion level ?
> > >
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavutil/internal.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libavutil/internal.h b/libavutil/internal.h
> > > index 7780a9a791..208f8f474f 100644
> > > --- a/libavutil/internal.h
> > > +++ b/libavutil/internal.h
> > > @@ -262,6 +262,12 @@ void avpriv_request_sample(void *avc,
> > >  #   define ff_dlog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_DEBUG,  
> > __VA_ARGS__); } while (0)  
> > >  #endif
> > >
> > > +#if defined(DEBUG) || ASSERT_LEVEL > 1
> > > +#   define ff_elog(ctx, ...) av_log(ctx, AV_LOG_ERROR, __VA_ARGS__)
> > > +#else
> > > +#   define ff_elog(ctx, ...) do { if (0) av_log(ctx, AV_LOG_ERROR,  
> > __VA_ARGS__); } while (0)  
> > > +#endif
> > > +
> > >  // For debuging we use signed operations so overflows can be detected  
> > (by ubsan)  
> > >  // For production we use unsigned so there are no undefined operations
> > >  #ifdef CHECKED  
> >
> > Not really a fan of this. (And neither ff_dlog.) But I guess I don't
> > mean to stop you.
> >
> > In my opinion, code that checks for overflow and fuzz issues should be
> > as small and unintrusive as possible, or preferably not exist.  
> 
> 
> This ^^^ +100.
> 
> And to be clear, "code" means binary as well as source code size.

Well, I think the appeal here is that the error message is normally
not compiled by default, because DEBUG is undefined and ASSERT_LEVEL is
0. I guess.

> (Whether that applies to h264_ps specifically is a separate issue; as I
> said on IRC a few days ago, it may well be that out-of-bounds values for
> uv_qp_offset are common outside of fuzzing, e.g. some encoders creating
> non-spec-compliant files. If that is really true, and we have examples of
> such files, then I totally understand that you want a log message to aid
> debugging, and maybe even a way to override them using -flags, as well as
> an indication of this option/flag in the error message.)

Didn't know this... acknowledged.