diff mbox series

[FFmpeg-devel] avformat/mpegts: Check the next sync byte to avoid incorrectt detected raw packet size

Message ID 1589538218-22814-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/mpegts: Check the next sync byte to avoid incorrectt detected raw packet size | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lance Wang May 15, 2020, 10:23 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
it'll cause many cc errors report and cannot be recovered. So it's better to check
the next sync byte before skip the extra unused bytes.

Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
a master/slave switch serveral times, the raw size can be easily detected by mistake.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/mpegts.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Jun Zhao May 15, 2020, 12:02 p.m. UTC | #1 
On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
> it'll cause many cc errors report and cannot be recovered. So it's better to check
> the next sync byte before skip the extra unused bytes.
>
> Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
> a master/slave switch serveral times, the raw size can be easily detected by mistake.
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/mpegts.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 0833d62..049555c 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
>      return 0;
>  }
>
> -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
> +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
>  {
>      AVIOContext *pb = s->pb;
>      int skip = raw_packet_size - TS_PACKET_SIZE;
> -    if (skip > 0)
> +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
> +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
>          avio_skip(pb, skip);
>  }
>
> @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
>          if (ret != 0)
>              break;
>          ret = handle_packet(ts, data, avio_tell(s->pb));
> -        finished_reading_packet(s, ts->raw_packet_size);
> +        finished_reading_packet(s, data, ts->raw_packet_size);
>          if (ret != 0)
>              break;
>      }
> @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
>              pid = AV_RB16(data + 1) & 0x1fff;
>              if ((pcr_pid == -1 || pcr_pid == pid) &&
>                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
> -                finished_reading_packet(s, ts->raw_packet_size);
> +                finished_reading_packet(s, data, ts->raw_packet_size);
>                  pcr_pid = pid;
>                  packet_count[nb_pcrs] = nb_packets;
>                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
> @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
>                      }
>                  }
>              } else {
> -                finished_reading_packet(s, ts->raw_packet_size);
> +                finished_reading_packet(s, data, ts->raw_packet_size);
>              }
>              nb_packets++;
>          }
> @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
>      }
>      if (data != pkt->data)
>          memcpy(pkt->data, data, ts->raw_packet_size);
> -    finished_reading_packet(s, ts->raw_packet_size);
> +    finished_reading_packet(s, data, ts->raw_packet_size);
>      if (ts->mpeg2ts_compute_pcr) {
>          /* compute exact PCR for each packet */
>          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
> --
> 1.8.3.1
>
Do you have a case to reproduce the cc errors report?
Lance Wang May 15, 2020, 12:47 p.m. UTC | #2 
On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy@gmail.com wrote:
> On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
> > it'll cause many cc errors report and cannot be recovered. So it's better to check
> > the next sync byte before skip the extra unused bytes.
> >
> > Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
> > a master/slave switch serveral times, the raw size can be easily detected by mistake.
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/mpegts.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> > index 0833d62..049555c 100644
> > --- a/libavformat/mpegts.c
> > +++ b/libavformat/mpegts.c
> > @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
> >      return 0;
> >  }
> >
> > -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
> > +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
> >  {
> >      AVIOContext *pb = s->pb;
> >      int skip = raw_packet_size - TS_PACKET_SIZE;
> > -    if (skip > 0)
> > +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
> > +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
> >          avio_skip(pb, skip);
> >  }
> >
> > @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
> >          if (ret != 0)
> >              break;
> >          ret = handle_packet(ts, data, avio_tell(s->pb));
> > -        finished_reading_packet(s, ts->raw_packet_size);
> > +        finished_reading_packet(s, data, ts->raw_packet_size);
> >          if (ret != 0)
> >              break;
> >      }
> > @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
> >              pid = AV_RB16(data + 1) & 0x1fff;
> >              if ((pcr_pid == -1 || pcr_pid == pid) &&
> >                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
> > -                finished_reading_packet(s, ts->raw_packet_size);
> > +                finished_reading_packet(s, data, ts->raw_packet_size);
> >                  pcr_pid = pid;
> >                  packet_count[nb_pcrs] = nb_packets;
> >                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
> > @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
> >                      }
> >                  }
> >              } else {
> > -                finished_reading_packet(s, ts->raw_packet_size);
> > +                finished_reading_packet(s, data, ts->raw_packet_size);
> >              }
> >              nb_packets++;
> >          }
> > @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
> >      }
> >      if (data != pkt->data)
> >          memcpy(pkt->data, data, ts->raw_packet_size);
> > -    finished_reading_packet(s, ts->raw_packet_size);
> > +    finished_reading_packet(s, data, ts->raw_packet_size);
> >      if (ts->mpeg2ts_compute_pcr) {
> >          /* compute exact PCR for each packet */
> >          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
> > --
> > 1.8.3.1
> >
> Do you have a case to reproduce the cc errors report?

Yes, it's real case which can be reproduced in lab.
Marton Balint May 15, 2020, 4:52 p.m. UTC | #3 
On Fri, 15 May 2020, lance.lmwang@gmail.com wrote:

> On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy@gmail.com wrote:
>> On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
>> >
>> > From: Limin Wang <lance.lmwang@gmail.com>
>> >
>> > reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
>> > it'll cause many cc errors report and cannot be recovered. So it's better to check
>> > the next sync byte before skip the extra unused bytes.
>> >
>> > Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
>> > a master/slave switch serveral times, the raw size can be easily detected by mistake.
>> >
>> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > ---
>> >  libavformat/mpegts.c | 13 +++++++------
>> >  1 file changed, 7 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> > index 0833d62..049555c 100644
>> > --- a/libavformat/mpegts.c
>> > +++ b/libavformat/mpegts.c
>> > @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
>> >      return 0;
>> >  }
>> >
>> > -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
>> > +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
>> >  {
>> >      AVIOContext *pb = s->pb;
>> >      int skip = raw_packet_size - TS_PACKET_SIZE;
>> > -    if (skip > 0)
>> > +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
>> > +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
>> >          avio_skip(pb, skip);
>> >  }
>> >
>> > @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
>> >          if (ret != 0)
>> >              break;
>> >          ret = handle_packet(ts, data, avio_tell(s->pb));
>> > -        finished_reading_packet(s, ts->raw_packet_size);
>> > +        finished_reading_packet(s, data, ts->raw_packet_size);
>> >          if (ret != 0)
>> >              break;
>> >      }
>> > @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
>> >              pid = AV_RB16(data + 1) & 0x1fff;
>> >              if ((pcr_pid == -1 || pcr_pid == pid) &&
>> >                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
>> > -                finished_reading_packet(s, ts->raw_packet_size);
>> > +                finished_reading_packet(s, data, ts->raw_packet_size);
>> >                  pcr_pid = pid;
>> >                  packet_count[nb_pcrs] = nb_packets;
>> >                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
>> > @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
>> >                      }
>> >                  }
>> >              } else {
>> > -                finished_reading_packet(s, ts->raw_packet_size);
>> > +                finished_reading_packet(s, data, ts->raw_packet_size);
>> >              }
>> >              nb_packets++;
>> >          }
>> > @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
>> >      }
>> >      if (data != pkt->data)
>> >          memcpy(pkt->data, data, ts->raw_packet_size);
>> > -    finished_reading_packet(s, ts->raw_packet_size);
>> > +    finished_reading_packet(s, data, ts->raw_packet_size);
>> >      if (ts->mpeg2ts_compute_pcr) {
>> >          /* compute exact PCR for each packet */
>> >          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
>> > --
>> > 1.8.3.1
>> >
>> Do you have a case to reproduce the cc errors report?
>
> Yes, it's real case which can be reproduced in lab.

Can you share the sample?

Thanks,
Marton
Lance Wang May 16, 2020, 1:37 a.m. UTC | #4 
On Fri, May 15, 2020 at 06:52:55PM +0200, Marton Balint wrote:
> 
> 
> On Fri, 15 May 2020, lance.lmwang@gmail.com wrote:
> 
> > On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy@gmail.com wrote:
> > > On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
> > > >
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > >
> > > > reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
> > > > it'll cause many cc errors report and cannot be recovered. So it's better to check
> > > > the next sync byte before skip the extra unused bytes.
> > > >
> > > > Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
> > > > a master/slave switch serveral times, the raw size can be easily detected by mistake.
> > > >
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > >  libavformat/mpegts.c | 13 +++++++------
> > > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> > > > index 0833d62..049555c 100644
> > > > --- a/libavformat/mpegts.c
> > > > +++ b/libavformat/mpegts.c
> > > > @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
> > > >      return 0;
> > > >  }
> > > >
> > > > -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
> > > > +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
> > > >  {
> > > >      AVIOContext *pb = s->pb;
> > > >      int skip = raw_packet_size - TS_PACKET_SIZE;
> > > > -    if (skip > 0)
> > > > +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
> > > > +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
> > > >          avio_skip(pb, skip);
> > > >  }
> > > >
> > > > @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
> > > >          if (ret != 0)
> > > >              break;
> > > >          ret = handle_packet(ts, data, avio_tell(s->pb));
> > > > -        finished_reading_packet(s, ts->raw_packet_size);
> > > > +        finished_reading_packet(s, data, ts->raw_packet_size);
> > > >          if (ret != 0)
> > > >              break;
> > > >      }
> > > > @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
> > > >              pid = AV_RB16(data + 1) & 0x1fff;
> > > >              if ((pcr_pid == -1 || pcr_pid == pid) &&
> > > >                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
> > > > -                finished_reading_packet(s, ts->raw_packet_size);
> > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
> > > >                  pcr_pid = pid;
> > > >                  packet_count[nb_pcrs] = nb_packets;
> > > >                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
> > > > @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
> > > >                      }
> > > >                  }
> > > >              } else {
> > > > -                finished_reading_packet(s, ts->raw_packet_size);
> > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
> > > >              }
> > > >              nb_packets++;
> > > >          }
> > > > @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > >      }
> > > >      if (data != pkt->data)
> > > >          memcpy(pkt->data, data, ts->raw_packet_size);
> > > > -    finished_reading_packet(s, ts->raw_packet_size);
> > > > +    finished_reading_packet(s, data, ts->raw_packet_size);
> > > >      if (ts->mpeg2ts_compute_pcr) {
> > > >          /* compute exact PCR for each packet */
> > > >          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
> > > > --
> > > > 1.8.3.1
> > > >
> > > Do you have a case to reproduce the cc errors report?
> > 
> > Yes, it's real case which can be reproduced in lab.
> 
> Can you share the sample?

No, tested with master and slave udp ts stream which switch between the master and slave stream
more than five times, it'll cause tons of cc error report and can't be recovered.

> 
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint May 16, 2020, 9:44 a.m. UTC | #5 
On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:

> On Fri, May 15, 2020 at 06:52:55PM +0200, Marton Balint wrote:
>> 
>> 
>> On Fri, 15 May 2020, lance.lmwang@gmail.com wrote:
>> 
>> > On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy@gmail.com wrote:
>> > > On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
>> > > >
>> > > > From: Limin Wang <lance.lmwang@gmail.com>
>> > > >
>> > > > reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
>> > > > it'll cause many cc errors report and cannot be recovered. So it's better to check
>> > > > the next sync byte before skip the extra unused bytes.
>> > > >
>> > > > Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
>> > > > a master/slave switch serveral times, the raw size can be easily detected by mistake.
>> > > >
>> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > > > ---
>> > > >  libavformat/mpegts.c | 13 +++++++------
>> > > >  1 file changed, 7 insertions(+), 6 deletions(-)
>> > > >
>> > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> > > > index 0833d62..049555c 100644
>> > > > --- a/libavformat/mpegts.c
>> > > > +++ b/libavformat/mpegts.c
>> > > > @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
>> > > >      return 0;
>> > > >  }
>> > > >
>> > > > -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
>> > > > +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
>> > > >  {
>> > > >      AVIOContext *pb = s->pb;
>> > > >      int skip = raw_packet_size - TS_PACKET_SIZE;
>> > > > -    if (skip > 0)
>> > > > +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
>> > > > +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
>> > > >          avio_skip(pb, skip);
>> > > >  }
>> > > >
>> > > > @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
>> > > >          if (ret != 0)
>> > > >              break;
>> > > >          ret = handle_packet(ts, data, avio_tell(s->pb));
>> > > > -        finished_reading_packet(s, ts->raw_packet_size);
>> > > > +        finished_reading_packet(s, data, ts->raw_packet_size);
>> > > >          if (ret != 0)
>> > > >              break;
>> > > >      }
>> > > > @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
>> > > >              pid = AV_RB16(data + 1) & 0x1fff;
>> > > >              if ((pcr_pid == -1 || pcr_pid == pid) &&
>> > > >                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
>> > > > -                finished_reading_packet(s, ts->raw_packet_size);
>> > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
>> > > >                  pcr_pid = pid;
>> > > >                  packet_count[nb_pcrs] = nb_packets;
>> > > >                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
>> > > > @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
>> > > >                      }
>> > > >                  }
>> > > >              } else {
>> > > > -                finished_reading_packet(s, ts->raw_packet_size);
>> > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
>> > > >              }
>> > > >              nb_packets++;
>> > > >          }
>> > > > @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
>> > > >      }
>> > > >      if (data != pkt->data)
>> > > >          memcpy(pkt->data, data, ts->raw_packet_size);
>> > > > -    finished_reading_packet(s, ts->raw_packet_size);
>> > > > +    finished_reading_packet(s, data, ts->raw_packet_size);
>> > > >      if (ts->mpeg2ts_compute_pcr) {
>> > > >          /* compute exact PCR for each packet */
>> > > >          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
>> > > > --
>> > > > 1.8.3.1
>> > > >
>> > > Do you have a case to reproduce the cc errors report?
>> > 
>> > Yes, it's real case which can be reproduced in lab.
>> 
>> Can you share the sample?
>
> No, tested with master and slave udp ts stream which switch between the master and slave stream
> more than five times, it'll cause tons of cc error report and can't be recovered.

I suggest you capture the input, so this issue can be properly reproduced 
and share it. It may even make sense to create a fate test from it.

Anyway, your patch does not seem right, because it seems to revert back 
the packet size as soon as the first byte to be skipped is 0x47. That will 
surely cause issues for normal streams having a packet size > 188.

Regards,
Marton
Lance Wang May 16, 2020, 2:38 p.m. UTC | #6 
On Sat, May 16, 2020 at 11:44:01AM +0200, Marton Balint wrote:
> 
> 
> On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
> 
> > On Fri, May 15, 2020 at 06:52:55PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Fri, 15 May 2020, lance.lmwang@gmail.com wrote:
> > > 
> > > > On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy@gmail.com wrote:
> > > > > On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
> > > > > >
> > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > >
> > > > > > reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
> > > > > > it'll cause many cc errors report and cannot be recovered. So it's better to check
> > > > > > the next sync byte before skip the extra unused bytes.
> > > > > >
> > > > > > Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
> > > > > > a master/slave switch serveral times, the raw size can be easily detected by mistake.
> > > > > >
> > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > > ---
> > > > > >  libavformat/mpegts.c | 13 +++++++------
> > > > > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> > > > > > index 0833d62..049555c 100644
> > > > > > --- a/libavformat/mpegts.c
> > > > > > +++ b/libavformat/mpegts.c
> > > > > > @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
> > > > > >      return 0;
> > > > > >  }
> > > > > >
> > > > > > -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
> > > > > > +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
> > > > > >  {
> > > > > >      AVIOContext *pb = s->pb;
> > > > > >      int skip = raw_packet_size - TS_PACKET_SIZE;
> > > > > > -    if (skip > 0)
> > > > > > +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
> > > > > > +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
> > > > > >          avio_skip(pb, skip);
> > > > > >  }
> > > > > >
> > > > > > @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
> > > > > >          if (ret != 0)
> > > > > >              break;
> > > > > >          ret = handle_packet(ts, data, avio_tell(s->pb));
> > > > > > -        finished_reading_packet(s, ts->raw_packet_size);
> > > > > > +        finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > >          if (ret != 0)
> > > > > >              break;
> > > > > >      }
> > > > > > @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
> > > > > >              pid = AV_RB16(data + 1) & 0x1fff;
> > > > > >              if ((pcr_pid == -1 || pcr_pid == pid) &&
> > > > > >                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
> > > > > > -                finished_reading_packet(s, ts->raw_packet_size);
> > > > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > >                  pcr_pid = pid;
> > > > > >                  packet_count[nb_pcrs] = nb_packets;
> > > > > >                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
> > > > > > @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
> > > > > >                      }
> > > > > >                  }
> > > > > >              } else {
> > > > > > -                finished_reading_packet(s, ts->raw_packet_size);
> > > > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > >              }
> > > > > >              nb_packets++;
> > > > > >          }
> > > > > > @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > >      }
> > > > > >      if (data != pkt->data)
> > > > > >          memcpy(pkt->data, data, ts->raw_packet_size);
> > > > > > -    finished_reading_packet(s, ts->raw_packet_size);
> > > > > > +    finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > >      if (ts->mpeg2ts_compute_pcr) {
> > > > > >          /* compute exact PCR for each packet */
> > > > > >          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
> > > > > > --
> > > > > > 1.8.3.1
> > > > > >
> > > > > Do you have a case to reproduce the cc errors report?
> > > > > Yes, it's real case which can be reproduced in lab.
> > > 
> > > Can you share the sample?
> > 
> > No, tested with master and slave udp ts stream which switch between the master and slave stream
> > more than five times, it'll cause tons of cc error report and can't be recovered.
> 
> I suggest you capture the input, so this issue can be properly reproduced
> and share it. It may even make sense to create a fate test from it.
Have tried, the issue can't be reproduced by capture offset file. I guess it's
caused by live stream seek isn't same as off line file.

> 
> Anyway, your patch does not seem right, because it seems to revert back the
> packet size as soon as the first byte to be skipped is 0x47. That will
> surely cause issues for normal streams having a packet size > 188.

I haven't sample to test such case, I think 0x47 is sync byte, if the next 188
byte is 0x47, it's more possible it's sync byte as sync byte checking is always
make sure the next 188/192/204 payload is 0x47.

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint May 16, 2020, 5:36 p.m. UTC | #7 
On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:

> On Sat, May 16, 2020 at 11:44:01AM +0200, Marton Balint wrote:
>> 
>> 
>> On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
>> 
>> > On Fri, May 15, 2020 at 06:52:55PM +0200, Marton Balint wrote:
>> > > 
>> > > 
>> > > On Fri, 15 May 2020, lance.lmwang@gmail.com wrote:
>> > > 
>> > > > On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy@gmail.com wrote:
>> > > > > On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
>> > > > > >
>> > > > > > From: Limin Wang <lance.lmwang@gmail.com>
>> > > > > >
>> > > > > > reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
>> > > > > > it'll cause many cc errors report and cannot be recovered. So it's better to check
>> > > > > > the next sync byte before skip the extra unused bytes.
>> > > > > >
>> > > > > > Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
>> > > > > > a master/slave switch serveral times, the raw size can be easily detected by mistake.
>> > > > > >
>> > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > > > > > ---
>> > > > > >  libavformat/mpegts.c | 13 +++++++------
>> > > > > >  1 file changed, 7 insertions(+), 6 deletions(-)
>> > > > > >
>> > > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> > > > > > index 0833d62..049555c 100644
>> > > > > > --- a/libavformat/mpegts.c
>> > > > > > +++ b/libavformat/mpegts.c
>> > > > > > @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
>> > > > > >      return 0;
>> > > > > >  }
>> > > > > >
>> > > > > > -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
>> > > > > > +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
>> > > > > >  {
>> > > > > >      AVIOContext *pb = s->pb;
>> > > > > >      int skip = raw_packet_size - TS_PACKET_SIZE;
>> > > > > > -    if (skip > 0)
>> > > > > > +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
>> > > > > > +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
>> > > > > >          avio_skip(pb, skip);
>> > > > > >  }
>> > > > > >
>> > > > > > @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
>> > > > > >          if (ret != 0)
>> > > > > >              break;
>> > > > > >          ret = handle_packet(ts, data, avio_tell(s->pb));
>> > > > > > -        finished_reading_packet(s, ts->raw_packet_size);
>> > > > > > +        finished_reading_packet(s, data, ts->raw_packet_size);
>> > > > > >          if (ret != 0)
>> > > > > >              break;
>> > > > > >      }
>> > > > > > @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
>> > > > > >              pid = AV_RB16(data + 1) & 0x1fff;
>> > > > > >              if ((pcr_pid == -1 || pcr_pid == pid) &&
>> > > > > >                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
>> > > > > > -                finished_reading_packet(s, ts->raw_packet_size);
>> > > > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
>> > > > > >                  pcr_pid = pid;
>> > > > > >                  packet_count[nb_pcrs] = nb_packets;
>> > > > > >                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
>> > > > > > @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
>> > > > > >                      }
>> > > > > >                  }
>> > > > > >              } else {
>> > > > > > -                finished_reading_packet(s, ts->raw_packet_size);
>> > > > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
>> > > > > >              }
>> > > > > >              nb_packets++;
>> > > > > >          }
>> > > > > > @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
>> > > > > >      }
>> > > > > >      if (data != pkt->data)
>> > > > > >          memcpy(pkt->data, data, ts->raw_packet_size);
>> > > > > > -    finished_reading_packet(s, ts->raw_packet_size);
>> > > > > > +    finished_reading_packet(s, data, ts->raw_packet_size);
>> > > > > >      if (ts->mpeg2ts_compute_pcr) {
>> > > > > >          /* compute exact PCR for each packet */
>> > > > > >          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
>> > > > > > --
>> > > > > > 1.8.3.1
>> > > > > >
>> > > > > Do you have a case to reproduce the cc errors report?
>> > > > > Yes, it's real case which can be reproduced in lab.
>> > > 
>> > > Can you share the sample?
>> > 
>> > No, tested with master and slave udp ts stream which switch between the master and slave stream
>> > more than five times, it'll cause tons of cc error report and can't be recovered.
>> 
>> I suggest you capture the input, so this issue can be properly reproduced
>> and share it. It may even make sense to create a fate test from it.
>
> Have tried, the issue can't be reproduced by capture offset file. I guess it's
> caused by live stream seek isn't same as off line file.

If that is the case then you can override the seekability of files by 
using -seekable 0 option. Alternatively you might find some other means to 
replay the recorded ts, anyhow this should be reproduced, because I have a 
feeling we don't have the whole picture of what is going on.

>
>> 
>> Anyway, your patch does not seem right, because it seems to revert back the
>> packet size as soon as the first byte to be skipped is 0x47. That will
>> surely cause issues for normal streams having a packet size > 188.
>
> I haven't sample to test such case, I think 0x47 is sync byte, if the next 188
> byte is 0x47, it's more possible it's sync byte as sync byte checking is always
> make sure the next 188/192/204 payload is 0x47.

No, you can only involve any resync logic if you came across a sync byte 
which is not 0x47. You cannot make assumptions about packet size by 
checking bytes of packet data as long as you are in sync, because that 
would mean that packet data can affect sync decisions even for 
perfectly synced streams.

Regards,
Marton
Lance Wang May 17, 2020, 10:13 a.m. UTC | #8 
On Sat, May 16, 2020 at 07:36:45PM +0200, Marton Balint wrote:
> 
> 
> On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
> 
> > On Sat, May 16, 2020 at 11:44:01AM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
> > > 
> > > > On Fri, May 15, 2020 at 06:52:55PM +0200, Marton Balint wrote:
> > > > > > > > > On Fri, 15 May 2020, lance.lmwang@gmail.com wrote:
> > > > > > > > On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy@gmail.com
> > > wrote:
> > > > > > > On Fri, May 15, 2020 at 6:23 PM <lance.lmwang@gmail.com> wrote:
> > > > > > > >
> > > > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > >
> > > > > > > > reanalyze() may misdetect the new packet size as 204, but it's 188 still actualy,
> > > > > > > > it'll cause many cc errors report and cannot be recovered. So it's better to check
> > > > > > > > the next sync byte before skip the extra unused bytes.
> > > > > > > >
> > > > > > > > Also, it is best to change SIZE_STAT_THRESHOLD from 10 to 100? If the input stream has
> > > > > > > > a master/slave switch serveral times, the raw size can be easily detected by mistake.
> > > > > > > >
> > > > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > > ---
> > > > > > > >  libavformat/mpegts.c | 13 +++++++------
> > > > > > > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> > > > > > > > index 0833d62..049555c 100644
> > > > > > > > --- a/libavformat/mpegts.c
> > > > > > > > +++ b/libavformat/mpegts.c
> > > > > > > > @@ -2932,11 +2932,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
> > > > > > > >      return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > -static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
> > > > > > > > +static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
> > > > > > > >  {
> > > > > > > >      AVIOContext *pb = s->pb;
> > > > > > > >      int skip = raw_packet_size - TS_PACKET_SIZE;
> > > > > > > > -    if (skip > 0)
> > > > > > > > +    /* Check the next sync byte to avoid incorrectt detected raw packet size */
> > > > > > > > +    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
> > > > > > > >          avio_skip(pb, skip);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -2985,7 +2986,7 @@ static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
> > > > > > > >          if (ret != 0)
> > > > > > > >              break;
> > > > > > > >          ret = handle_packet(ts, data, avio_tell(s->pb));
> > > > > > > > -        finished_reading_packet(s, ts->raw_packet_size);
> > > > > > > > +        finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > > > >          if (ret != 0)
> > > > > > > >              break;
> > > > > > > >      }
> > > > > > > > @@ -3137,7 +3138,7 @@ static int mpegts_read_header(AVFormatContext *s)
> > > > > > > >              pid = AV_RB16(data + 1) & 0x1fff;
> > > > > > > >              if ((pcr_pid == -1 || pcr_pid == pid) &&
> > > > > > > >                  parse_pcr(&pcr_h, &pcr_l, data) == 0) {
> > > > > > > > -                finished_reading_packet(s, ts->raw_packet_size);
> > > > > > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > > > >                  pcr_pid = pid;
> > > > > > > >                  packet_count[nb_pcrs] = nb_packets;
> > > > > > > >                  pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
> > > > > > > > @@ -3154,7 +3155,7 @@ static int mpegts_read_header(AVFormatContext *s)
> > > > > > > >                      }
> > > > > > > >                  }
> > > > > > > >              } else {
> > > > > > > > -                finished_reading_packet(s, ts->raw_packet_size);
> > > > > > > > +                finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > > > >              }
> > > > > > > >              nb_packets++;
> > > > > > > >          }
> > > > > > > > @@ -3194,7 +3195,7 @@ static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > > > >      }
> > > > > > > >      if (data != pkt->data)
> > > > > > > >          memcpy(pkt->data, data, ts->raw_packet_size);
> > > > > > > > -    finished_reading_packet(s, ts->raw_packet_size);
> > > > > > > > +    finished_reading_packet(s, data, ts->raw_packet_size);
> > > > > > > >      if (ts->mpeg2ts_compute_pcr) {
> > > > > > > >          /* compute exact PCR for each packet */
> > > > > > > >          if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {
> > > > > > > > --
> > > > > > > > 1.8.3.1
> > > > > > > >
> > > > > > > Do you have a case to reproduce the cc errors report?
> > > > > > > Yes, it's real case which can be reproduced in lab.
> > > > > > > Can you share the sample?
> > > > > No, tested with master and slave udp ts stream which switch
> > > between the master and slave stream
> > > > more than five times, it'll cause tons of cc error report and can't be recovered.
> > > 
> > > I suggest you capture the input, so this issue can be properly reproduced
> > > and share it. It may even make sense to create a fate test from it.
> > 
> > Have tried, the issue can't be reproduced by capture offset file. I guess it's
> > caused by live stream seek isn't same as off line file.
> 
> If that is the case then you can override the seekability of files by using
> -seekable 0 option. Alternatively you might find some other means to replay
> the recorded ts, anyhow this should be reproduced, because I have a feeling
> we don't have the whole picture of what is going on.
I have spend more time to investigate it, I have try to capture a pcap file to
reproduce the issue, with tcprelay, it can be reproduced every time, however
if export to ts, I failed to reproduce it.

> 
> > 
> > > 
> > > Anyway, your patch does not seem right, because it seems to revert back the
> > > packet size as soon as the first byte to be skipped is 0x47. That will
> > > surely cause issues for normal streams having a packet size > 188.
> > 
> > I haven't sample to test such case, I think 0x47 is sync byte, if the next 188
> > byte is 0x47, it's more possible it's sync byte as sync byte checking is always
> > make sure the next 188/192/204 payload is 0x47.
> 
> No, you can only involve any resync logic if you came across a sync byte
> which is not 0x47. You cannot make assumptions about packet size by checking
> bytes of packet data as long as you are in sync, because that would mean
> that packet data can affect sync decisions even for perfectly synced
> streams.
Thanks for the advices, I spend more time to investigate and  try to fix it in the
resync logic, with more debug info, I notice the old reanalyze() has one drawback,
the pos isn't one of 188/192/204, so it'll cause the stat will reset all stats
ahead of one of stats[i] which cause normal 188 failed to detected anymore. I
think it's better to return if pos isn't one of 188/192/204, it'll fix my issue
by testing, please help to review whether it's reasonable or not. 
 

my debug log change:
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 0833d62..9a13146 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -2855,6 +2855,8 @@ static void reanalyze(MpegTSContext *ts) {
     }

     ts->size_stat_count ++;
+    av_log(ts->stream, AV_LOG_INFO, "reanalyze: stat: %d, stat[0]: %d, stat[1]: %d, stat[2]: %d, pos: %lld\n",
+        ts->size_stat_count, ts->size_stat[0], ts->size_stat[1], ts->size_stat[2], pos);
     if (ts->size_stat_count > SIZE_STAT_THRESHOLD) {
         int newsize = 0;
         if (ts->size_stat[0] > SIZE_STAT_THRESHOLD) {

The bug trigger log:
[mpegts @ 0x3ef0180] reanalyze: stat: 0, stat[0]: 0, stat[1]: 1, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] PES packet size mismatch
[mpegts @ 0x3ef0180] Packet corrupt (stream = 0, dts = 1320433).
[mpegts @ 0x3ef0180] DTS 8590011941 < 8591255025 out of orderN/A speed=1.11x
[mpegts @ 0x3ef0180] reanalyze: stat: 1, stat[0]: 0, stat[1]: 2, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] reanalyze: stat: 2, stat[0]: 0, stat[1]: 3, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] reanalyze: stat: 3, stat[0]: 0, stat[1]: 4, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] PES packet size mismatch
[mpegts @ 0x3ef0180] Packet corrupt (stream = 0, dts = 1229349).
[mpegts @ 0x3ef0180] DTS 8590128272 < 8591163941 out of orderN/A speed=1.07x
[mpegts @ 0x3ef0180] reanalyze: stat: 4, stat[0]: 0, stat[1]: 5, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] reanalyze: stat: 5, stat[0]: 0, stat[1]: 6, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] reanalyze: stat: 6, stat[0]: 0, stat[1]: 7, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] PES packet size mismatch
[mpegts @ 0x3ef0180] Packet corrupt (stream = 0, dts = 1464480).
[mpegts @ 0x3ef0180] DTS 8590009421 < 8591399072 out of orderN/A speed=1.06x
[mpegts @ 0x3ef0180] reanalyze: stat: 7, stat[0]: 0, stat[1]: 8, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] reanalyze: stat: 8, stat[0]: 0, stat[1]: 9, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] reanalyze: stat: 9, stat[0]: 0, stat[1]: 10, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] PES packet size mismatch
[mpegts @ 0x3ef0180] Packet corrupt (stream = 0, dts = 1828029).
[mpegts @ 0x3ef0180] DTS 8590030653 < 8591762621 out of orderN/A speed=1.02x
[mpegts @ 0x3ef0180] reanalyze: stat: 10, stat[0]: 0, stat[1]: 11, stat[2]: 0, raw_packet_size: 188, pos: 192
[mpegts @ 0x3ef0180] changing packet size to 192
[mpegts @ 0x3ef0180] reanalyze: stat: 0, stat[0]: 1, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] PES packet size mismatch
[mpegts @ 0x3ef0180] Packet corrupt (stream = 0, dts = 1442461).
[mpegts @ 0x3ef0180] reanalyze: stat: 1, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 2, stat[0]: 3, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 3, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 4, stat[0]: 5, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188


the log which explain why it failed to detect back to 188:

[mpegts @ 0x3ef0180] reanalyze: stat: 1, stat[0]: 1, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 2, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 3, stat[0]: 3, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 4, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 5, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 564
[mpegts @ 0x3ef0180] reanalyze: stat: 6, stat[0]: 5, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 7, stat[0]: 6, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 8, stat[0]: 6, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 376
[mpegts @ 0x3ef0180] reanalyze: stat: 9, stat[0]: 7, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 10, stat[0]: 7, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 474
[mpegts @ 0x3ef0180] reanalyze: stat: 0, stat[0]: 0, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 752
[mpegts @ 0x3ef0180] reanalyze: stat: 1, stat[0]: 0, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 940
[mpegts @ 0x3ef0180] reanalyze: stat: 2, stat[0]: 1, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 3, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 4, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 737
[mpegts @ 0x3ef0180] reanalyze: stat: 5, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 203
[mpegts @ 0x3ef0180] reanalyze: stat: 6, stat[0]: 3, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 7, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 8, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 376
[mpegts @ 0x3ef0180] reanalyze: stat: 9, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 564
[mpegts @ 0x3ef0180] reanalyze: stat: 10, stat[0]: 5, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 0, stat[0]: 1, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 1, stat[0]: 1, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 376
[mpegts @ 0x3ef0180] reanalyze: stat: 2, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 3, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 564
[mpegts @ 0x3ef0180] reanalyze: stat: 4, stat[0]: 3, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 5, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 6, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 376
[mpegts @ 0x3ef0180] reanalyze: stat: 7, stat[0]: 5, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
[mpegts @ 0x3ef0180] reanalyze: stat: 8, stat[0]: 5, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 564
[mpegts @ 0x3ef0180] reanalyze: stat: 9, stat[0]: 6, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
....

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint May 17, 2020, 12:41 p.m. UTC | #9 
On Sun, 17 May 2020, lance.lmwang@gmail.com wrote:

> On Sat, May 16, 2020 at 07:36:45PM +0200, Marton Balint wrote:
>> 
>> 
>> On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
>> 
>> > On Sat, May 16, 2020 at 11:44:01AM +0200, Marton Balint wrote:
>> > > 
>> > > 
>> > > On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
>> > > 
>> > > I suggest you capture the input, so this issue can be properly reproduced
>> > > and share it. It may even make sense to create a fate test from it.
>> > 
>> > Have tried, the issue can't be reproduced by capture offset file. I guess it's
>> > caused by live stream seek isn't same as off line file.
>> 
>> If that is the case then you can override the seekability of files by using
>> -seekable 0 option. Alternatively you might find some other means to replay
>> the recorded ts, anyhow this should be reproduced, because I have a feeling
>> we don't have the whole picture of what is going on.
>
> I have spend more time to investigate it, I have try to capture a pcap file to
> reproduce the issue, with tcprelay, it can be reproduced every time, however
> if export to ts, I failed to reproduce it.

Great. Can you share the pcap file? You can may send it to me privately if 
you don't want it to be made public.

[...]

> Thanks for the advices, I spend more time to investigate and  try to fix it in the
> resync logic, with more debug info, I notice the old reanalyze() has one drawback,
> the pos isn't one of 188/192/204, so it'll cause the stat will reset all stats
> ahead of one of stats[i] which cause normal 188 failed to detected anymore. I
> think it's better to return if pos isn't one of 188/192/204, it'll fix my issue
> by testing, please help to review whether it's reasonable or not.

I am not sure yet.

> the log which explain why it failed to detect back to 188:
>
> [mpegts @ 0x3ef0180] reanalyze: stat: 1, stat[0]: 1, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> [mpegts @ 0x3ef0180] reanalyze: stat: 2, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> [mpegts @ 0x3ef0180] reanalyze: stat: 3, stat[0]: 3, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> [mpegts @ 0x3ef0180] reanalyze: stat: 4, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> [mpegts @ 0x3ef0180] reanalyze: stat: 5, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 564
> [mpegts @ 0x3ef0180] reanalyze: stat: 6, stat[0]: 5, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> [mpegts @ 0x3ef0180] reanalyze: stat: 7, stat[0]: 6, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> [mpegts @ 0x3ef0180] reanalyze: stat: 8, stat[0]: 6, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 376
> [mpegts @ 0x3ef0180] reanalyze: stat: 9, stat[0]: 7, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> [mpegts @ 0x3ef0180] reanalyze: stat: 10, stat[0]: 7, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 474
> [mpegts @ 0x3ef0180] reanalyze: stat: 0, stat[0]: 0, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 752

pos as a multiple of 188 and not 188 is very suspicious, and it might be 
because pos47_full is only updated if the packet has a payload. Can you 
try the attached patch and see if it makes a difference?

Thanks,
Marton
Lance Wang May 19, 2020, 10:53 a.m. UTC | #10 
On Sun, May 17, 2020 at 02:41:18PM +0200, Marton Balint wrote:
> 
> 
> On Sun, 17 May 2020, lance.lmwang@gmail.com wrote:
> 
> > On Sat, May 16, 2020 at 07:36:45PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
> > > 
> > > > On Sat, May 16, 2020 at 11:44:01AM +0200, Marton Balint wrote:
> > > > > > > > > On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
> > > > > > > I suggest you capture the input, so this issue can be
> > > properly reproduced
> > > > > and share it. It may even make sense to create a fate test from it.
> > > > > Have tried, the issue can't be reproduced by capture offset
> > > file. I guess it's
> > > > caused by live stream seek isn't same as off line file.
> > > 
> > > If that is the case then you can override the seekability of files by using
> > > -seekable 0 option. Alternatively you might find some other means to replay
> > > the recorded ts, anyhow this should be reproduced, because I have a feeling
> > > we don't have the whole picture of what is going on.
> > 
> > I have spend more time to investigate it, I have try to capture a pcap file to
> > reproduce the issue, with tcprelay, it can be reproduced every time, however
> > if export to ts, I failed to reproduce it.
> 
> Great. Can you share the pcap file? You can may send it to me privately if
> you don't want it to be made public.

I have send it the sample to you privately, if you haven't got it, please let me
know.

> 
> [...]
> 
> > Thanks for the advices, I spend more time to investigate and  try to fix it in the
> > resync logic, with more debug info, I notice the old reanalyze() has one drawback,
> > the pos isn't one of 188/192/204, so it'll cause the stat will reset all stats
> > ahead of one of stats[i] which cause normal 188 failed to detected anymore. I
> > think it's better to return if pos isn't one of 188/192/204, it'll fix my issue
> > by testing, please help to review whether it's reasonable or not.
> 
> I am not sure yet.

You can tried with my pcap sample, please update the pcap mac info and use
tcpreplay to get the udp stream.

> 
> > the log which explain why it failed to detect back to 188:
> > 
> > [mpegts @ 0x3ef0180] reanalyze: stat: 1, stat[0]: 1, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> > [mpegts @ 0x3ef0180] reanalyze: stat: 2, stat[0]: 2, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> > [mpegts @ 0x3ef0180] reanalyze: stat: 3, stat[0]: 3, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> > [mpegts @ 0x3ef0180] reanalyze: stat: 4, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> > [mpegts @ 0x3ef0180] reanalyze: stat: 5, stat[0]: 4, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 564
> > [mpegts @ 0x3ef0180] reanalyze: stat: 6, stat[0]: 5, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> > [mpegts @ 0x3ef0180] reanalyze: stat: 7, stat[0]: 6, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> > [mpegts @ 0x3ef0180] reanalyze: stat: 8, stat[0]: 6, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 376
> > [mpegts @ 0x3ef0180] reanalyze: stat: 9, stat[0]: 7, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 188
> > [mpegts @ 0x3ef0180] reanalyze: stat: 10, stat[0]: 7, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 474
> > [mpegts @ 0x3ef0180] reanalyze: stat: 0, stat[0]: 0, stat[1]: 0, stat[2]: 0, raw_packet_size: 192, pos: 752
> 
> pos as a multiple of 188 and not 188 is very suspicious, and it might be
> because pos47_full is only updated if the packet has a payload. Can you try
> the attached patch and see if it makes a difference?

Sorry, I'm miss it before.  I have tried with the patch, you can reproduce with
my sample with offline file, below is the log:

[mpegts @ 0x49a9940] reanalyze: stat: 4, stat[0]: 0, stat[1]: 1, stat[2]: 0, pos: 192
[mpegts @ 0x49a9940] reanalyze: stat: 5, stat[0]: 0, stat[1]: 2, stat[2]: 0, pos: 192
[mpegts @ 0x49a9940] reanalyze: stat: 6, stat[0]: 0, stat[1]: 3, stat[2]: 0, pos: 192
[mpegts @ 0x49a9940] reanalyze: stat: 7, stat[0]: 0, stat[1]: 3, stat[2]: 0, pos: 240
[mpegts @ 0x49a9940] reanalyze: stat: 8, stat[0]: 0, stat[1]: 3, stat[2]: 0, pos: 195
[mpegts @ 0x49a9940] reanalyze: stat: 9, stat[0]: 0, stat[1]: 3, stat[2]: 0, pos: 195
[mpegts @ 0x49a9940] reanalyze: stat: 10, stat[0]: 0, stat[1]: 3, stat[2]: 0, pos: 298
[mpegts @ 0x49a9940] Packet corrupt (stream = 0, dts = 446069).
[mpegts @ 0x49a9940] reanalyze: stat: 11, stat[0]: 0, stat[1]: 4, stat[2]: 0, pos: 192
[mpegts @ 0x49a9940] reanalyze: stat: 1, stat[0]: 0, stat[1]: 1, stat[2]: 0, pos: 192
[mpegts @ 0x49a9940] reanalyze: stat: 2, stat[0]: 0, stat[1]: 2, stat[2]: 0, pos: 192
[mpegts @ 0x49a9940] reanalyze: stat: 3, stat[0]: 0, stat[1]: 2, stat[2]: 0, pos: 240
[mpegts @ 0x49a9940] reanalyze: stat: 4, stat[0]: 0, stat[1]: 2, stat[2]: 0, pos: 312


> 
> Thanks,
> Marton

> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 0833d62ea5..921a7ae2db 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -2774,11 +2774,6 @@ static int handle_packet(MpegTSContext *ts, const uint8_t *packet, int64_t pos)
>      if (p >= p_end || !has_payload)
>          return 0;
>  
> -    if (pos >= 0) {
> -        av_assert0(pos >= TS_PACKET_SIZE);
> -        ts->pos47_full = pos - TS_PACKET_SIZE;
> -    }
> -
>      if (tss->type == MPEGTS_SECTION) {
>          if (is_start) {
>              /* pointer field present */
> @@ -2926,6 +2921,8 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
>              else
>                  continue;
>          } else {
> +            MpegTSContext *ts = s->priv_data;
> +            ts->pos47_full = avio_tell(pb) - TS_PACKET_SIZE;
>              break;
>          }
>      }

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint May 19, 2020, 6:29 p.m. UTC | #11 
On Tue, 19 May 2020, lance.lmwang@gmail.com wrote:

> On Sun, May 17, 2020 at 02:41:18PM +0200, Marton Balint wrote:
>> 
>> 
>> On Sun, 17 May 2020, lance.lmwang@gmail.com wrote:
>> 
>> > On Sat, May 16, 2020 at 07:36:45PM +0200, Marton Balint wrote:
>> > > 
>> > > 
>> > > On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
>> > > 
>> > > > On Sat, May 16, 2020 at 11:44:01AM +0200, Marton Balint wrote:
>> > > > > > > > > On Sat, 16 May 2020, lance.lmwang@gmail.com wrote:
>> > > > > > > I suggest you capture the input, so this issue can be
>> > > properly reproduced
>> > > > > and share it. It may even make sense to create a fate test from it.
>> > > > > Have tried, the issue can't be reproduced by capture offset
>> > > file. I guess it's
>> > > > caused by live stream seek isn't same as off line file.
>> > > 
>> > > If that is the case then you can override the seekability of files by using
>> > > -seekable 0 option. Alternatively you might find some other means to replay
>> > > the recorded ts, anyhow this should be reproduced, because I have a feeling
>> > > we don't have the whole picture of what is going on.
>> > 
>> > I have spend more time to investigate it, I have try to capture a pcap file to
>> > reproduce the issue, with tcprelay, it can be reproduced every time, however
>> > if export to ts, I failed to reproduce it.
>> 
>> Great. Can you share the pcap file? You can may send it to me privately if
>> you don't want it to be made public.
>
> I have send it the sample to you privately, if you haven't got it, please let me
> know.

Thanks, got it, and reproduced it, I will reply to your patch.

Regards,
Marton
Marton Balint May 19, 2020, 6:49 p.m. UTC | #12 
On Sun, 17 May 2020, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavformat/mpegts.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index 0833d62..4bca339 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -2852,6 +2852,8 @@ static void reanalyze(MpegTSContext *ts) {
>         ts->size_stat[1] ++;
>     } else if (pos == TS_FEC_PACKET_SIZE) {
>         ts->size_stat[2] ++;
> +    } else {
> +        return;
>     }

Okay, so what this patch does is that it skips unrecognized sync byte 
distances entirely. I guess that is OK, but it still not fixes 
not recognizing skipped 0x47 bytes. I will send an alternative patch with 
some more explanation.

Thanks,
Marton
diff mbox series

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 0833d62..049555c 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -2932,11 +2932,12 @@  static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size,
     return 0;
 }
 
-static void finished_reading_packet(AVFormatContext *s, int raw_packet_size)
+static void finished_reading_packet(AVFormatContext *s, const uint8_t *data, int raw_packet_size)
 {
     AVIOContext *pb = s->pb;
     int skip = raw_packet_size - TS_PACKET_SIZE;
-    if (skip > 0)
+    /* Check the next sync byte to avoid incorrectt detected raw packet size */
+    if (skip > 0 && data[TS_PACKET_SIZE] != 0x47)
         avio_skip(pb, skip);
 }
 
@@ -2985,7 +2986,7 @@  static int handle_packets(MpegTSContext *ts, int64_t nb_packets)
         if (ret != 0)
             break;
         ret = handle_packet(ts, data, avio_tell(s->pb));
-        finished_reading_packet(s, ts->raw_packet_size);
+        finished_reading_packet(s, data, ts->raw_packet_size);
         if (ret != 0)
             break;
     }
@@ -3137,7 +3138,7 @@  static int mpegts_read_header(AVFormatContext *s)
             pid = AV_RB16(data + 1) & 0x1fff;
             if ((pcr_pid == -1 || pcr_pid == pid) &&
                 parse_pcr(&pcr_h, &pcr_l, data) == 0) {
-                finished_reading_packet(s, ts->raw_packet_size);
+                finished_reading_packet(s, data, ts->raw_packet_size);
                 pcr_pid = pid;
                 packet_count[nb_pcrs] = nb_packets;
                 pcrs[nb_pcrs] = pcr_h * 300 + pcr_l;
@@ -3154,7 +3155,7 @@  static int mpegts_read_header(AVFormatContext *s)
                     }
                 }
             } else {
-                finished_reading_packet(s, ts->raw_packet_size);
+                finished_reading_packet(s, data, ts->raw_packet_size);
             }
             nb_packets++;
         }
@@ -3194,7 +3195,7 @@  static int mpegts_raw_read_packet(AVFormatContext *s, AVPacket *pkt)
     }
     if (data != pkt->data)
         memcpy(pkt->data, data, ts->raw_packet_size);
-    finished_reading_packet(s, ts->raw_packet_size);
+    finished_reading_packet(s, data, ts->raw_packet_size);
     if (ts->mpeg2ts_compute_pcr) {
         /* compute exact PCR for each packet */
         if (parse_pcr(&pcr_h, &pcr_l, pkt->data) == 0) {