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

[Michael Niedermayer]

On Tue, Jan 11, 2011 at 01:24:42AM +0100, Stefano Sabatini wrote:
> 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

>  utils.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> dc0808179e4a95d31f65e227525f291f63653ddc  0001-Clarify-timestamps-related-error-messages-in-compute.patch
> From 61145694168a9777040d4f54f8e4acef10499866 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Thu, 6 Jan 2011 11:38:20 +0100
> Subject: [PATCH] Clarify timestamps related error messages in compute_pkt_fields2().

lgtm

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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20110111/7bfb8dca/attachment.pgp>