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

[Michael Niedermayer]

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)


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110106/5f5f7236/attachment.pgp>