diff mbox

[FFmpeg-devel,V2] avformat/concat: Fix wrong wrapped timestamp

Message ID 1513312085-31436-1-git-send-email-mymoeyard@gmail.com
State Superseded
Headers show

Commit Message

mymoeyard@gmail.com Dec. 15, 2017, 4:28 a.m. UTC 
From: Wu Zhiqiang <mymoeyard@gmail.com>

When using concat protocol, start from middle of file will generate non-zero wrap reference.
If seek to time before the wrap reference, wrap control will generate wrong wrapped timestamp.
Copy wrap related stream properties when reading header can fix this problem.

Signed-off-by: Wu Zhiqiang <mymoeyard@gmail.com>
---
 libavformat/concatdec.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Nicolas George Dec. 17, 2017, 2:55 p.m. UTC | #1 
mymoeyard@gmail.com (2017-12-14):
> From: Wu Zhiqiang <mymoeyard@gmail.com>
> 
> When using concat protocol, start from middle of file will generate non-zero wrap reference.
> If seek to time before the wrap reference, wrap control will generate wrong wrapped timestamp.
> Copy wrap related stream properties when reading header can fix this problem.
> 
> Signed-off-by: Wu Zhiqiang <mymoeyard@gmail.com>
> ---
>  libavformat/concatdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
> index 0e189012ad..8dae2737df 100644
> --- a/libavformat/concatdec.c
> +++ b/libavformat/concatdec.c
> @@ -188,6 +188,11 @@ static int copy_stream_props(AVStream *st, AVStream *source_st)
>      st->time_base           = source_st->time_base;
>      st->sample_aspect_ratio = source_st->sample_aspect_ratio;
>  
> +    /* Fix wrap control problem */
> +    avpriv_set_pts_info(st, source_st->pts_wrap_bits, source_st->time_base.num, source_st->time_base.den);
> +    st->pts_wrap_behavior   = source_st->pts_wrap_behavior;
> +    st->pts_wrap_reference  = source_st->pts_wrap_reference;
> +
>      av_dict_copy(&st->metadata, source_st->metadata, 0);
>      return 0;
>  }

The concat demuxer is mine to maintain, but I am not very familiar with
the wrapped timestamps handling, and the commit message does not
enlighten me.

(By the way, please wrap it at 60-70 characters.)

The way I see it, if the library takes care of de-wrapping timestamps,
then the concat demuxer will see de-wrapped timestamps from the
underlying demuxer, and the timestamps do not need to be de-wrapped a
second time: setting the wrap information is unnecessary and even wrong.

Please explain what I am missing.

Regards,
mymoeyard@gmail.com Dec. 18, 2017, 7:17 a.m. UTC | #2 
Thanks for your replay.

I see wrap timestamp control is enable by pts_wrap_bits(default 33),
but mp4 demuxer  will later set this to 64 (means disabled).
Now pts_wrap_bits are always tied to 33 without copy it, which seems
strange.

Normally the pts_wrap_reference  is based on first packet of a file in
avformat_find_stream_info.
Without copy the value,  the value is based on first packet of read_packet,
will be non-zero after seeking.

2017-12-17 22:55 GMT+08:00 Nicolas George <george@nsup.org>:

> mymoeyard@gmail.com (2017-12-14):
> > From: Wu Zhiqiang <mymoeyard@gmail.com>
> >
> > When using concat protocol, start from middle of file will generate
> non-zero wrap reference.
> > If seek to time before the wrap reference, wrap control will generate
> wrong wrapped timestamp.
> > Copy wrap related stream properties when reading header can fix this
> problem.
> >
> > Signed-off-by: Wu Zhiqiang <mymoeyard@gmail.com>
> > ---
> >  libavformat/concatdec.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
> > index 0e189012ad..8dae2737df 100644
> > --- a/libavformat/concatdec.c
> > +++ b/libavformat/concatdec.c
> > @@ -188,6 +188,11 @@ static int copy_stream_props(AVStream *st, AVStream
> *source_st)
> >      st->time_base           = source_st->time_base;
> >      st->sample_aspect_ratio = source_st->sample_aspect_ratio;
> >
> > +    /* Fix wrap control problem */
> > +    avpriv_set_pts_info(st, source_st->pts_wrap_bits,
> source_st->time_base.num, source_st->time_base.den);
> > +    st->pts_wrap_behavior   = source_st->pts_wrap_behavior;
> > +    st->pts_wrap_reference  = source_st->pts_wrap_reference;
> > +
> >      av_dict_copy(&st->metadata, source_st->metadata, 0);
> >      return 0;
> >  }
>
> The concat demuxer is mine to maintain, but I am not very familiar with
> the wrapped timestamps handling, and the commit message does not
> enlighten me.
>
> (By the way, please wrap it at 60-70 characters.)
>
> The way I see it, if the library takes care of de-wrapping timestamps,
> then the concat demuxer will see de-wrapped timestamps from the
> underlying demuxer, and the timestamps do not need to be de-wrapped a
> second time: setting the wrap information is unnecessary and even wrong.
>
> Please explain what I am missing.
>
> Regards,
>
> --
>   Nicolas George
>
Nicolas George Dec. 29, 2017, noon UTC | #3 
Hi. Sorry for having missed your reply earlier.

吴志强 (2017-12-18):
> I see wrap timestamp control is enable by pts_wrap_bits(default 33),
> but mp4 demuxer  will later set this to 64 (means disabled).
> Now pts_wrap_bits are always tied to 33 without copy it, which seems
> strange.

I understand the logic of this patch less and less. Why are you speaking
about the MP4 demuxer?

If I understand correctly how unwrapping timestamp works, concat should
never have to do it.

> 2017-12-17 22:55 GMT+08:00 Nicolas George <george@nsup.org>:

Please remember that top-posting on this list is forbidden. If you do
not know what it means, look it up.

Regards,
mymoeyard@gmail.com Dec. 29, 2017, 4:17 p.m. UTC | #4 
On Fri, Dec 29, 2017 at 8:00 PM, Nicolas George <george@nsup.org> wrote:

> Hi. Sorry for having missed your reply earlier.
>
> 吴志强 (2017-12-18):
> > I see wrap timestamp control is enable by pts_wrap_bits(default 33),
> > but mp4 demuxer  will later set this to 64 (means disabled).
> > Now pts_wrap_bits are always tied to 33 without copy it, which seems
> > strange.
>
> I understand the logic of this patch less and less. Why are you speaking
> about the MP4 demuxer?
>
> If I understand correctly how unwrapping timestamp works, concat should
> never have to do it.
>
> > 2017-12-17 22:55 GMT+08:00 Nicolas George <george@nsup.org>:
>
> Please remember that top-posting on this list is forbidden. If you do
> not know what it means, look it up.
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Sorry for top-posting.

Normally pts_wrap_reference is based on the first packet when calling
ff_read_packet,
which will call  function update_wrap_reference , in libavformat/utils line
734:

    if (ref == AV_NOPTS_VALUE)
        ref = pkt->pts;
    if (st->pts_wrap_reference != AV_NOPTS_VALUE || st->pts_wrap_bits >= 63
|| ref == AV_NOPTS_VALUE || !s->correct_ts_overflow)
        return 0;
    ref &= (1LL << st->pts_wrap_bits)-1;

    // reference time stamp should be 60 s before first time stamp
    pts_wrap_reference = ref - av_rescale(60, st->time_base.den,
st->time_base.num);

Because  concat_read_header  and concat_seek do not call ff_read_packet and
open_file does not copy pts_wrap_reference,
it is constant value during playing.

If  demuxing packets like avformat_open_input => avformat_seek_file =>
av_read_frame and seek time is larger than 60s,
the constant reference time will be positive value and not  indicate the
normal wrap margin.

Then seeking to time before pts_wrap_reference may generate huge timestamp,
which  not expected.

Thanks.
Nicolas George Dec. 29, 2017, 4:30 p.m. UTC | #5 
Wu Zhiqiang (2017-12-30):
> Normally pts_wrap_reference is based on the first packet when calling
> ff_read_packet,
> which will call  function update_wrap_reference , in libavformat/utils line
> 734:
> 
>     if (ref == AV_NOPTS_VALUE)
>         ref = pkt->pts;
>     if (st->pts_wrap_reference != AV_NOPTS_VALUE || st->pts_wrap_bits >= 63
> || ref == AV_NOPTS_VALUE || !s->correct_ts_overflow)
>         return 0;
>     ref &= (1LL << st->pts_wrap_bits)-1;
> 
>     // reference time stamp should be 60 s before first time stamp
>     pts_wrap_reference = ref - av_rescale(60, st->time_base.den,
> st->time_base.num);
> 
> Because  concat_read_header  and concat_seek do not call ff_read_packet and
> open_file does not copy pts_wrap_reference,
> it is constant value during playing.
> 
> If  demuxing packets like avformat_open_input => avformat_seek_file =>
> av_read_frame and seek time is larger than 60s,
> the constant reference time will be positive value and not  indicate the
> normal wrap margin.
> 
> Then seeking to time before pts_wrap_reference may generate huge timestamp,
> which  not expected.

Thanks for explaining, but it still does not address my concern. I
probably explained it badly. Let me try to rephrase.

When an application is reading directly from a file, if I understand
correctly, here is what happens:

1. file contains wrapped timestamps;
2. demuxer sets wrapping_bits to enable timestamps correction;
3. demuxer outputs packets with non-monotonic timestamps;
4. lavf sees non-monotonic timestamps;
5. lavf detects wrapping reference;
6. lavf fixes non-monotonic timestamps;
7. lavf returns packets with monotonic timestamps;
8. application sees packets with monotonic timestamps;
9. application has nothing to do at all about wrapping.

Is it right?

The concat demuxer behaves like any application. Therefore, I can
copy-paste and rename:

1. file contains wrapped timestamps;
2. slave demuxer sets wrapping_bits to enable timestamps correction;
3. slave demuxer outputs packets with non-monotonic timestamps;
4. lavf for slave demuxer sees non-monotonic timestamps;
5. lavf for slave demuxer detects wrapping reference;
6. lavf for slave demuxer fixes non-monotonic timestamps;
7. lavf for slave demuxer returns packets with monotonic timestamps;
8. concat sees packets with monotonic timestamps;
9. concat has nothing to do at all about wrapping.

And it should still be right. Note the conclusion: concat has nothing to
do at all about wrapping.

I will try to rephrase yet another way:

Wrapped timestamps should be de-wrapped even before concat sees them,
and thus no de-wrapping should be necessary after concat.

Regards,
mymoeyard@gmail.com Dec. 29, 2017, 5:33 p.m. UTC | #6 
2017年12月30日 上午12:30，"Nicolas George" <george@nsup.org>写道：

Wu Zhiqiang (2017-12-30):
> Normally pts_wrap_reference is based on the first packet when calling
> ff_read_packet,
> which will call  function update_wrap_reference , in libavformat/utils
line
> 734:
>
>     if (ref == AV_NOPTS_VALUE)
>         ref = pkt->pts;
>     if (st->pts_wrap_reference != AV_NOPTS_VALUE || st->pts_wrap_bits >=
63
> || ref == AV_NOPTS_VALUE || !s->correct_ts_overflow)
>         return 0;
>     ref &= (1LL << st->pts_wrap_bits)-1;
>
>     // reference time stamp should be 60 s before first time stamp
>     pts_wrap_reference = ref - av_rescale(60, st->time_base.den,
> st->time_base.num);
>
> Because  concat_read_header  and concat_seek do not call ff_read_packet
and
> open_file does not copy pts_wrap_reference,
> it is constant value during playing.
>
> If  demuxing packets like avformat_open_input => avformat_seek_file =>
> av_read_frame and seek time is larger than 60s,
> the constant reference time will be positive value and not  indicate the
> normal wrap margin.
>
> Then seeking to time before pts_wrap_reference may generate huge
timestamp,
> which  not expected.

Thanks for explaining, but it still does not address my concern. I
probably explained it badly. Let me try to rephrase.

When an application is reading directly from a file, if I understand
correctly, here is what happens:

1. file contains wrapped timestamps;
2. demuxer sets wrapping_bits to enable timestamps correction;
3. demuxer outputs packets with non-monotonic timestamps;
4. lavf sees non-monotonic timestamps;
5. lavf detects wrapping reference;
6. lavf fixes non-monotonic timestamps;
7. lavf returns packets with monotonic timestamps;
8. application sees packets with monotonic timestamps;
9. application has nothing to do at all about wrapping.

Is it right?

The concat demuxer behaves like any application. Therefore, I can
copy-paste and rename:

1. file contains wrapped timestamps;
2. slave demuxer sets wrapping_bits to enable timestamps correction;
3. slave demuxer outputs packets with non-monotonic timestamps;
4. lavf for slave demuxer sees non-monotonic timestamps;
5. lavf for slave demuxer detects wrapping reference;
6. lavf for slave demuxer fixes non-monotonic timestamps;
7. lavf for slave demuxer returns packets with monotonic timestamps;
8. concat sees packets with monotonic timestamps;
9. concat has nothing to do at all about wrapping.

And it should still be right. Note the conclusion: concat has nothing to
do at all about wrapping.

I will try to rephrase yet another way:

Wrapped timestamps should be de-wrapped even before concat sees them,
and thus no de-wrapping should be necessary after concat.

Regards,

--
  Nicolas George
Nicolas George Dec. 29, 2017, 5:38 p.m. UTC | #7 
Wu Zhiqiang (2017-12-30):
> But how to  decide wrap margin that  concat see?
> Though monotonic timestamp is always ok,
> I do not want that sub-demuxer see unwrapped  timestamp but concat see
> wrapped one .
> Or sub-demuxer disable wrap correction but concat enable it.

I am sorry, I do not understand what you are saying at all.

Maybe it would help if you were to give a fully detailed of the simplest
use case you are trying to fix: the files you are trying to concat, with
their format, their timestamps at the beginning, at the end and at
discontinuities, the positions where the application jumps, the
timestamps that are returned by lavf at both levels.

The reason I am insisting on all this is that I fear the problem lies
elsewhere, another bug in timestamps unwrapping. This patch would bury
it deeper instead of fixing it.

Regards,
Carl Eugen Hoyos Dec. 29, 2017, 9:19 p.m. UTC | #8 
2017-12-29 18:38 GMT+01:00 Nicolas George <george@nsup.org>:

> I am sorry, I do not understand what you are saying at all.
>
> Maybe it would help if you were to give a fully detailed of the simplest
> use case you are trying to fix

Maybe ticket #6908?

Carl Eugen
mymoeyard@gmail.com Dec. 30, 2017, 7:16 a.m. UTC | #9 
On Sat, Dec 30, 2017 at 5:19 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:

> 2017-12-29 18:38 GMT+01:00 Nicolas George <george@nsup.org>:
>
> > I am sorry, I do not understand what you are saying at all.
> >
> > Maybe it would help if you were to give a fully detailed of the simplest
> > use case you are trying to fix
>
> Maybe ticket #6908?
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
i am not sure ticket #6908 is the same case.

The command to  generate sample video:

ffmpeg -f lavfi -i testsrc=duration=120  -c:v h264 -profile:v high -level:v
10 -pix_fmt yuv420p -r 30 -g 30 -c:a aac test.flv
echo -e "file test.flv\nduration 120" > playlist
ffplay -f concat playlist -ss 90 -max_ts_probe 0

then seek to time before 30s , the result timestamp  is huge and
print:"Invalid timestamps stream"

ffmpeg version N-89656-g0c78b6a Copyright (c) 2000-2017 the FFmpeg
developers
built with gcc 5.4.0 (Ubuntu 5.4.0-6ubuntu1~16.04.4) 20160609
Thanks
diff mbox

Patch

diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index 0e189012ad..8dae2737df 100644
--- a/libavformat/concatdec.c
+++ b/libavformat/concatdec.c
@@ -188,6 +188,11 @@  static int copy_stream_props(AVStream *st, AVStream *source_st)
     st->time_base           = source_st->time_base;
     st->sample_aspect_ratio = source_st->sample_aspect_ratio;
 
+    /* Fix wrap control problem */
+    avpriv_set_pts_info(st, source_st->pts_wrap_bits, source_st->time_base.num, source_st->time_base.den);
+    st->pts_wrap_behavior   = source_st->pts_wrap_behavior;
+    st->pts_wrap_reference  = source_st->pts_wrap_reference;
+
     av_dict_copy(&st->metadata, source_st->metadata, 0);
     return 0;
 }