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

[lance.lmwang at gmail.com]

On Sat, May 16, 2020 at 11:44:01AM +0200, Marton Balint wrote:
> 
> 
> On Sat, 16 May 2020, lance.lmwang at gmail.com wrote:
> 
> > On Fri, May 15, 2020 at 06:52:55PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Fri, 15 May 2020, lance.lmwang at gmail.com wrote:
> > > 
> > > > On Fri, May 15, 2020 at 08:02:44PM +0800, mypopy at gmail.com wrote:
> > > > > On Fri, May 15, 2020 at 6:23 PM <lance.lmwang at gmail.com> wrote:
> > > > > >
> > > > > > From: Limin Wang <lance.lmwang at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Thanks,
Limin Wang