diff mbox

[FFmpeg-devel] lavf/utils: update stream duration only if it is not set or 0

Message ID 20170703120224.25003-1-matthieu.bouron@gmail.com
State New
Headers show

Commit Message

Matthieu Bouron July 3, 2017, 12:02 p.m. UTC 
---

The following patch makes lavf/utils only update stream duration only if it is
not set or 0 in fill_all_stream_timings (which is called by
avformat_find_stream_info).

In the context of mov demuxing, the patch makes the last packet duration the
same as the one declared in the stts table for example (ie: we trust what the
demuxer reports).

The patch passes fate, however it might not be valid but I would like to
understand what is the purpose / use-case of the update_stream_timing /
fill_all_stream_timings functions which are not too well documented.

Thanks.

---
 libavformat/utils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer July 4, 2017, 10:31 a.m. UTC | #1 
On Mon, Jul 03, 2017 at 02:02:24PM +0200, Matthieu Bouron wrote:
> ---
> 
> The following patch makes lavf/utils only update stream duration only if it is
> not set or 0 in fill_all_stream_timings (which is called by
> avformat_find_stream_info).
> 
> In the context of mov demuxing, the patch makes the last packet duration the
> same as the one declared in the stts table for example (ie: we trust what the
> demuxer reports).
> 
> The patch passes fate, however it might not be valid but I would like to
> understand what is the purpose / use-case of the update_stream_timing /
> fill_all_stream_timings functions which are not too well documented.

I think these functions were mostly for filling in values that
have not been set otherwise
as in stream timings <-> file timings, that way demuxers can just
export what is stored in a file and teh rest is set from that


> 
> Thanks.
> 
> ---
>  libavformat/utils.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 38d247c6cd..00adf026e6 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2654,7 +2654,8 @@ static void fill_all_stream_timings(AVFormatContext *ic)
>              if (ic->start_time != AV_NOPTS_VALUE)
>                  st->start_time = av_rescale_q(ic->start_time, AV_TIME_BASE_Q,
>                                                st->time_base);
> -            if (ic->duration != AV_NOPTS_VALUE)
> +            if (ic->duration != AV_NOPTS_VALUE &&
> +                (st->duration == 0 || st->duration == AV_NOPTS_VALUE))
>                  st->duration = av_rescale_q(ic->duration, AV_TIME_BASE_Q,
>                                              st->time_base);
>          }

can a inconsistency occur from setting start_time and duration from
different sources ?



[...]
Matthieu Bouron July 4, 2017, 3:24 p.m. UTC | #2 
On Tue, Jul 04, 2017 at 12:31:59PM +0200, Michael Niedermayer wrote:
> On Mon, Jul 03, 2017 at 02:02:24PM +0200, Matthieu Bouron wrote:
> > ---
> > 
> > The following patch makes lavf/utils only update stream duration only if it is
> > not set or 0 in fill_all_stream_timings (which is called by
> > avformat_find_stream_info).
> > 
> > In the context of mov demuxing, the patch makes the last packet duration the
> > same as the one declared in the stts table for example (ie: we trust what the
> > demuxer reports).
> > 
> > The patch passes fate, however it might not be valid but I would like to
> > understand what is the purpose / use-case of the update_stream_timing /
> > fill_all_stream_timings functions which are not too well documented.
> 
> I think these functions were mostly for filling in values that
> have not been set otherwise
> as in stream timings <-> file timings, that way demuxers can just
> export what is stored in a file and teh rest is set from that
> 
> 
> > 
> > Thanks.
> > 
> > ---
> >  libavformat/utils.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 38d247c6cd..00adf026e6 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -2654,7 +2654,8 @@ static void fill_all_stream_timings(AVFormatContext *ic)
> >              if (ic->start_time != AV_NOPTS_VALUE)
> >                  st->start_time = av_rescale_q(ic->start_time, AV_TIME_BASE_Q,
> >                                                st->time_base);
> > -            if (ic->duration != AV_NOPTS_VALUE)
> > +            if (ic->duration != AV_NOPTS_VALUE &&
> > +                (st->duration == 0 || st->duration == AV_NOPTS_VALUE))
> >                  st->duration = av_rescale_q(ic->duration, AV_TIME_BASE_Q,
> >                                              st->time_base);
> >          }
> 
> can a inconsistency occur from setting start_time and duration from
> different sources ?

Probably. Maybe the patch should *not* update the timings if both
start_time and duration are already set ?
Matthieu Bouron July 4, 2017, 3:40 p.m. UTC | #3 
On Tue, Jul 04, 2017 at 05:24:45PM +0200, Matthieu Bouron wrote:
> On Tue, Jul 04, 2017 at 12:31:59PM +0200, Michael Niedermayer wrote:
> > On Mon, Jul 03, 2017 at 02:02:24PM +0200, Matthieu Bouron wrote:
> > > ---
> > > 
> > > The following patch makes lavf/utils only update stream duration only if it is
> > > not set or 0 in fill_all_stream_timings (which is called by
> > > avformat_find_stream_info).
> > > 
> > > In the context of mov demuxing, the patch makes the last packet duration the
> > > same as the one declared in the stts table for example (ie: we trust what the
> > > demuxer reports).
> > > 
> > > The patch passes fate, however it might not be valid but I would like to
> > > understand what is the purpose / use-case of the update_stream_timing /
> > > fill_all_stream_timings functions which are not too well documented.
> > 
> > I think these functions were mostly for filling in values that
> > have not been set otherwise
> > as in stream timings <-> file timings, that way demuxers can just
> > export what is stored in a file and teh rest is set from that
> > 
> > 
> > > 
> > > Thanks.
> > > 
> > > ---
> > >  libavformat/utils.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 38d247c6cd..00adf026e6 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -2654,7 +2654,8 @@ static void fill_all_stream_timings(AVFormatContext *ic)
> > >              if (ic->start_time != AV_NOPTS_VALUE)
> > >                  st->start_time = av_rescale_q(ic->start_time, AV_TIME_BASE_Q,
> > >                                                st->time_base);
> > > -            if (ic->duration != AV_NOPTS_VALUE)
> > > +            if (ic->duration != AV_NOPTS_VALUE &&
> > > +                (st->duration == 0 || st->duration == AV_NOPTS_VALUE))
> > >                  st->duration = av_rescale_q(ic->duration, AV_TIME_BASE_Q,
> > >                                              st->time_base);
> > >          }
> > 
> > can a inconsistency occur from setting start_time and duration from
> > different sources ?
> 
> Probably. Maybe the patch should *not* update the timings if both
> start_time and duration are already set ?

Nevermind, we only enter this block of code if st->start_time ==
AV_NOPTS_VALUE.

> 
> -- 
> Matthieu B.
diff mbox

Patch

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 38d247c6cd..00adf026e6 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -2654,7 +2654,8 @@  static void fill_all_stream_timings(AVFormatContext *ic)
             if (ic->start_time != AV_NOPTS_VALUE)
                 st->start_time = av_rescale_q(ic->start_time, AV_TIME_BASE_Q,
                                               st->time_base);
-            if (ic->duration != AV_NOPTS_VALUE)
+            if (ic->duration != AV_NOPTS_VALUE &&
+                (st->duration == 0 || st->duration == AV_NOPTS_VALUE))
                 st->duration = av_rescale_q(ic->duration, AV_TIME_BASE_Q,
                                             st->time_base);
         }