[FFmpeg-devel] [PATCH] Make av_set_pts_info keep previous time base if new one is invalid.

[Michael Niedermayer]

On Sun, Feb 06, 2011 at 11:37:52AM +0100, Reimar D?ffinger wrote:
> On Sun, Feb 06, 2011 at 02:40:01AM +0100, Michael Niedermayer wrote:
> > On Sat, Feb 05, 2011 at 08:08:01PM -0500, Ronald S. Bultje wrote:
> > > Hi,
> > > 
> > > On Sat, Feb 5, 2011 at 4:35 AM, Reimar D?ffinger
> > > <Reimar.Doeffinger at gmx.de> wrote:
> > > > ---
> > > > ?libavformat/utils.c | ? 15 ++++++++-------
> > > > ?1 files changed, 8 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > > index 4f51c26..c49d8ef 100644
> > > > --- a/libavformat/utils.c
> > > > +++ b/libavformat/utils.c
> > > > @@ -3742,16 +3742,17 @@ int ff_hex_to_data(uint8_t *data, const char *p)
> > > > ?void av_set_pts_info(AVStream *s, int pts_wrap_bits,
> > > > ? ? ? ? ? ? ? ? ? ? ?unsigned int pts_num, unsigned int pts_den)
> > > > ?{
> > > > - ? ?s->pts_wrap_bits = pts_wrap_bits;
> > > > -
> > > > - ? ?if(av_reduce(&s->time_base.num, &s->time_base.den, pts_num, pts_den, INT_MAX)){
> > > > - ? ? ? ?if(s->time_base.num != pts_num)
> > > > - ? ? ? ? ? ?av_log(NULL, AV_LOG_DEBUG, "st:%d removing common factor %d from timebase\n", s->index, pts_num/s->time_base.num);
> > > > + ? ?AVRational new_tb;
> > > > + ? ?if(av_reduce(&new_tb.num, &new_tb.den, pts_num, pts_den, INT_MAX)){
> > > > + ? ? ? ?if(new_tb.num != pts_num)
> > > > + ? ? ? ? ? ?av_log(NULL, AV_LOG_DEBUG, "st:%d removing common factor %d from timebase\n", s->index, pts_num/new_tb.num);
> > > > ? ? }else
> > > > ? ? ? ? av_log(NULL, AV_LOG_WARNING, "st:%d has too large timebase, reducing\n", s->index);
> > > >
> > > > - ? ?if(!s->time_base.num || !s->time_base.den)
> > > > - ? ? ? ?s->time_base.num= s->time_base.den= 0;
> > > > + ? ?if(new_tb.num <= 0 || new_tb.den <= 0)
> > > > + ? ? ? ?return;
> > > > + ? ?s->time_base = new_tb;
> > > > + ? ?s->pts_wrap_bits = pts_wrap_bits;
> > > > ?}
> > > >
> > > > ?int ff_url_join(char *str, int size, const char *proto,
> > > > --
> > > > 1.7.2.3
> > > 
> > > Not wanting to start a bikeshed, but I wonder if it's a good idea to
> > > make av_set_pts_info() return an error code if it didn't set it, like
> > > -1 or AVERROR(EINVAL) or whatever, and then perhaps we can even have
> > 
> > it is a good idea
> 
> It could already fail before and changing it is an API and (at least
> a potential) ABI change.

you could add av_set_pts_info2()


> 
> > > demuxers (optionally, this is not required because it has no security
> > > implications) error out if av_set_pts_info() "fails".
> > > 
> > > At the very least it should be documented in the doxy that it can
> > > "fail" and that it means tb remains unchanged.
> > 
> > it also should print a big error message because its failure is hard to
> > debug otherwise.
> 
> Can be done, even though I'd like to note that I don't see why it has
> to be in one patch, the things you mention were an issue before,
> this fixes just one issue: setting values that make the rest of
> the code crash.

I never said it has to be in one patch.
Just that i prefer a crash over speding hours debuging why AV sync is completely
off
The default timebase is 1/90000 if the first and only case setting it uses an
invalid timebase that will stay without warning,

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

Avoid a single point of failure, be that a person or equipment.
-------------- 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/20110206/1cccb3cf/attachment.pgp>