[FFmpeg-devel] [PATCH] Issue more explicit error messages in compute_pkt_fields2().

[Stefano Sabatini]

On date Thursday 2011-01-06 20:46:28 +0100, Michael Niedermayer encoded:
> On Thu, Jan 06, 2011 at 11:59:59AM +0100, Stefano Sabatini wrote:
> > On date Wednesday 2011-01-05 16:55:14 +0100, Michael Niedermayer encoded:
> > > On Tue, Jan 04, 2011 at 07:50:47PM +0100, Stefano Sabatini wrote:
> > > > ---
> > > >  libavformat/utils.c |    6 ++++--
> > > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > > index fb93e3b..3b1b34e 100644
> > > > --- a/libavformat/utils.c
> > > > +++ b/libavformat/utils.c
> > > > @@ -2917,12 +2917,14 @@ static int compute_pkt_fields2(AVFormatContext *s, AVStream *st, AVPacket *pkt){
> > > >  
> > > >      if(st->cur_dts && st->cur_dts != AV_NOPTS_VALUE && st->cur_dts >= pkt->dts){
> > > >          av_log(s, AV_LOG_ERROR,
> > > > -               "st:%d error, non monotone timestamps %"PRId64" >= %"PRId64"\n",
> > > > +               "Non monotone timestamps in stream number %d: st->cur_dts:%"PRId64" >= pkt->dts:%"PRId64"\n",
> > > >                 st->index, st->cur_dts, pkt->dts);
> > > 
> > > Thats just confusing the user with internal details.
> > > And might make it look like its some problem internal to libav while truth,
> > > plain and simple the libav user (like ffmpeg.c) has provided invalid timestamps
> > > What libavformat internal variables are used to make this check has no relevance
> > 
> > Where the problem lays is secondary, I'd like the log message to
> > convey what was wrong in an intelligible way.
> 
> That would be, either way:
> "Application provided invalid, non monotonly increasing dts to muxer"
> (with the actual values & stream number of course)

Yes that's even better.

> 
> 
> >  
> > > >          return -1;
> > > >      }
> > > >      if(pkt->dts != AV_NOPTS_VALUE && pkt->pts != AV_NOPTS_VALUE && pkt->pts < pkt->dts){
> > > > -        av_log(s, AV_LOG_ERROR, "st:%d error, pts < dts\n", st->index);
> > > > +        av_log(s, AV_LOG_ERROR,
> > > > +               "pts < dts in stream number %d\n: pkt->pts:%"PRId64" < pkt->dts:%"PRId64"\n",
> > > > +               st->index, pkt->pts, pkt->dts);
> > > 
> > > This should be on one line for being grepable saneley and theres no need to
> > > say pts<dts twice.
> > 
> > > Also you loose the word error which is the most fundamental part of this, it
> > > is an error not a warning
> > 
> > Error is implied by the log level of the message, and also from
> > the fact that the function is returning an error code. I can add the
> > string "error" but this looks quite pointless to me.
> 
> The error message is intended for the user, the user doesnt know what a
> function returned without debugger, and without a terminal with color support
> he wont see the log level either.
> If the user doesnt know
> anymore if something is debugging chatter or error thats bad IMHO.

That's basically an application problem, indeed the application may
add a log level tag in its log callback.

Updated.
-- 
FFmpeg = Fanciful and Fostering Meaningful Puristic Elitist God