diff mbox series

[FFmpeg-devel] avformat/mpegts: fix resync logic stuck in 192 bytes

Message ID 20200519190659.11906-1-cus@passwd.hu
State New
Headers show
Series [FFmpeg-devel] avformat/mpegts: fix resync logic stuck in 192 bytes
Related show

Checks

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

Commit Message

Marton Balint May 19, 2020, 7:06 p.m. UTC
pos47_full is not updated for every packet, and for unseekable inputs the
resync logic might simply skip some 0x47 sync bytes. In order to detect these
let's check for modulo instead of exact value.

Also skip unrecognized sync byte distances instead of considering them as a
failure of detection. It only delays the detection of the new packet size.

Also note that AVIO only buffers a single packet (for UDP/mpegts, that usually
means 1316 bytes), so among every ten consecutive 188-byte MPEGTS packets there
will always be a seek failure, and that caused the old code to not find the 188
byte pattern across 10 consecutive packets.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mpegts.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Limin Wang May 19, 2020, 11:18 p.m. UTC | #1
On Tue, May 19, 2020 at 09:06:59PM +0200, Marton Balint wrote:
> pos47_full is not updated for every packet, and for unseekable inputs the
> resync logic might simply skip some 0x47 sync bytes. In order to detect these
> let's check for modulo instead of exact value.

Before modifying and returning directly, I considered whether pos is a multiple 
of raw_packet_size, by the debug information, it's not true. Also it's possible
for the pos of two sync byte > 188/192/204 * n. If that's true, it's not correct
to look it as stats hit by the modulo logic.

> 
> Also skip unrecognized sync byte distances instead of considering them as a
> failure of detection. It only delays the detection of the new packet size.
> 
> Also note that AVIO only buffers a single packet (for UDP/mpegts, that usually
> means 1316 bytes), so among every ten consecutive 188-byte MPEGTS packets there
> will always be a seek failure, and that caused the old code to not find the 188
> byte pattern across 10 consecutive packets.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mpegts.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index a065c61c40..f2b2c05d86 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -2846,12 +2846,14 @@ static void reanalyze(MpegTSContext *ts) {
>      if (pos < 0)
>          return;
>      pos -= ts->pos47_full;
> -    if (pos == TS_PACKET_SIZE) {
> +    if (pos % TS_PACKET_SIZE == 0) {
>          ts->size_stat[0] ++;
> -    } else if (pos == TS_DVHS_PACKET_SIZE) {
> +    } if (pos % TS_DVHS_PACKET_SIZE == 0) {
>          ts->size_stat[1] ++;
> -    } else if (pos == TS_FEC_PACKET_SIZE) {
> +    } if (pos % TS_FEC_PACKET_SIZE == 0) {
>          ts->size_stat[2] ++;
> +    } else {
> +        return;
>      }
>  
>      ts->size_stat_count ++;
> -- 
> 2.26.1
> 
> _______________________________________________
> 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 20, 2020, 6:26 a.m. UTC | #2
On Wed, 20 May 2020, lance.lmwang@gmail.com wrote:

> On Tue, May 19, 2020 at 09:06:59PM +0200, Marton Balint wrote:
>> pos47_full is not updated for every packet, and for unseekable inputs the
>> resync logic might simply skip some 0x47 sync bytes. In order to detect these
>> let's check for modulo instead of exact value.
>
> Before modifying and returning directly, I considered whether pos is a multiple 
> of raw_packet_size, by the debug information, it's not true.

Not always, because sometimes there are 0x47 sync bytes in the packet 
payload as well. But we siply ignore that case because of the return later 
originated from your patch. Checking for modulo allows faster detection 
and it also allows detection when UDP packets contain a single MPEGTS 
packet.

> Also it's possible
> for the pos of two sync byte > 188/192/204 * n. If that's true, it's not correct
> to look it as stats hit by the modulo logic.

I don't understand what you mean here.

Regards,
Marton


>
>> 
>> Also skip unrecognized sync byte distances instead of considering them as a
>> failure of detection. It only delays the detection of the new packet size.
>> 
>> Also note that AVIO only buffers a single packet (for UDP/mpegts, that usually
>> means 1316 bytes), so among every ten consecutive 188-byte MPEGTS packets there
>> will always be a seek failure, and that caused the old code to not find the 188
>> byte pattern across 10 consecutive packets.
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mpegts.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> index a065c61c40..f2b2c05d86 100644
>> --- a/libavformat/mpegts.c
>> +++ b/libavformat/mpegts.c
>> @@ -2846,12 +2846,14 @@ static void reanalyze(MpegTSContext *ts) {
>>      if (pos < 0)
>>          return;
>>      pos -= ts->pos47_full;
>> -    if (pos == TS_PACKET_SIZE) {
>> +    if (pos % TS_PACKET_SIZE == 0) {
>>          ts->size_stat[0] ++;
>> -    } else if (pos == TS_DVHS_PACKET_SIZE) {
>> +    } if (pos % TS_DVHS_PACKET_SIZE == 0) {
>>          ts->size_stat[1] ++;
>> -    } else if (pos == TS_FEC_PACKET_SIZE) {
>> +    } if (pos % TS_FEC_PACKET_SIZE == 0) {
>>          ts->size_stat[2] ++;
>> +    } else {
>> +        return;
>>      }
>>
>>      ts->size_stat_count ++;
>> -- 
>> 2.26.1
>> 
>> _______________________________________________
>> 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".
>
> -- 
> Thanks,
> Limin Wang
> _______________________________________________
> 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".
Limin Wang May 20, 2020, 9:37 a.m. UTC | #3
On Wed, May 20, 2020 at 08:26:37AM +0200, Marton Balint wrote:
> 
> 
> On Wed, 20 May 2020, lance.lmwang@gmail.com wrote:
> 
> >On Tue, May 19, 2020 at 09:06:59PM +0200, Marton Balint wrote:
> >>pos47_full is not updated for every packet, and for unseekable inputs the
> >>resync logic might simply skip some 0x47 sync bytes. In order to detect these
> >>let's check for modulo instead of exact value.
> >
> >Before modifying and returning directly, I considered whether pos
> >is a multiple of raw_packet_size, by the debug information, it's
> >not true.
> 
> Not always, because sometimes there are 0x47 sync bytes in the
> packet payload as well. But we siply ignore that case because of the
> return later originated from your patch. Checking for modulo allows
> faster detection and it also allows detection when UDP packets
> contain a single MPEGTS packet.
> 
> >Also it's possible
> >for the pos of two sync byte > 188/192/204 * n. If that's true, it's not correct
> >to look it as stats hit by the modulo logic.
> 
> I don't understand what you mean here.

Let me give one example, the size is 192*2, but the second 192 isn't start by 0x47
sync byte, checking with modulo will not correct for the case


> 
> Regards,
> Marton
> 
> 
> >
> >>
> >>Also skip unrecognized sync byte distances instead of considering them as a
> >>failure of detection. It only delays the detection of the new packet size.
> >>
> >>Also note that AVIO only buffers a single packet (for UDP/mpegts, that usually
> >>means 1316 bytes), so among every ten consecutive 188-byte MPEGTS packets there
> >>will always be a seek failure, and that caused the old code to not find the 188
> >>byte pattern across 10 consecutive packets.
> >>
> >>Signed-off-by: Marton Balint <cus@passwd.hu>
> >>---
> >> libavformat/mpegts.c | 8 +++++---
> >> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> >>index a065c61c40..f2b2c05d86 100644
> >>--- a/libavformat/mpegts.c
> >>+++ b/libavformat/mpegts.c
> >>@@ -2846,12 +2846,14 @@ static void reanalyze(MpegTSContext *ts) {
> >>     if (pos < 0)
> >>         return;
> >>     pos -= ts->pos47_full;
> >>-    if (pos == TS_PACKET_SIZE) {
> >>+    if (pos % TS_PACKET_SIZE == 0) {
> >>         ts->size_stat[0] ++;
> >>-    } else if (pos == TS_DVHS_PACKET_SIZE) {
> >>+    } if (pos % TS_DVHS_PACKET_SIZE == 0) {
> >>         ts->size_stat[1] ++;
> >>-    } else if (pos == TS_FEC_PACKET_SIZE) {
> >>+    } if (pos % TS_FEC_PACKET_SIZE == 0) {
> >>         ts->size_stat[2] ++;
> >>+    } else {
> >>+        return;
> >>     }
> >>
> >>     ts->size_stat_count ++;
> >>-- 
> >>2.26.1
> >>
> >>_______________________________________________
> >>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".
> >
> >-- 
> >Thanks,
> >Limin Wang
> >_______________________________________________
> >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".
> _______________________________________________
> 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".
Michael Niedermayer May 20, 2020, 12:39 p.m. UTC | #4
On Tue, May 19, 2020 at 09:06:59PM +0200, Marton Balint wrote:
> pos47_full is not updated for every packet, and for unseekable inputs the
> resync logic might simply skip some 0x47 sync bytes. In order to detect these
> let's check for modulo instead of exact value.
> 
> Also skip unrecognized sync byte distances instead of considering them as a
> failure of detection. It only delays the detection of the new packet size.
> 
> Also note that AVIO only buffers a single packet (for UDP/mpegts, that usually
> means 1316 bytes), so among every ten consecutive 188-byte MPEGTS packets there
> will always be a seek failure, and that caused the old code to not find the 188
> byte pattern across 10 consecutive packets.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mpegts.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> index a065c61c40..f2b2c05d86 100644
> --- a/libavformat/mpegts.c
> +++ b/libavformat/mpegts.c
> @@ -2846,12 +2846,14 @@ static void reanalyze(MpegTSContext *ts) {
>      if (pos < 0)
>          return;
>      pos -= ts->pos47_full;
> -    if (pos == TS_PACKET_SIZE) {
> +    if (pos % TS_PACKET_SIZE == 0) {
>          ts->size_stat[0] ++;
> -    } else if (pos == TS_DVHS_PACKET_SIZE) {
> +    } if (pos % TS_DVHS_PACKET_SIZE == 0) {
>          ts->size_stat[1] ++;
> -    } else if (pos == TS_FEC_PACKET_SIZE) {
> +    } if (pos % TS_FEC_PACKET_SIZE == 0) {
>          ts->size_stat[2] ++;
> +    } else {
> +        return;
>      }

This patch breaks, or at least changes

./ffmpeg -i tspacket_size_changeback.ts  -vframes 2 -qscale 2 test.avi

ill mail you the file privatly as i dont remember from where this is
and google fails to find any public copy

thx

[...]
Marton Balint May 20, 2020, 6:08 p.m. UTC | #5
On Wed, 20 May 2020, Limin Wang wrote:

> On Wed, May 20, 2020 at 08:26:37AM +0200, Marton Balint wrote:
>> 
>> 
>> On Wed, 20 May 2020, lance.lmwang@gmail.com wrote:
>> 
>> >On Tue, May 19, 2020 at 09:06:59PM +0200, Marton Balint wrote:
>> >>pos47_full is not updated for every packet, and for unseekable inputs the
>> >>resync logic might simply skip some 0x47 sync bytes. In order to detect these
>> >>let's check for modulo instead of exact value.
>> >
>> >Before modifying and returning directly, I considered whether pos
>> >is a multiple of raw_packet_size, by the debug information, it's
>> >not true.
>> 
>> Not always, because sometimes there are 0x47 sync bytes in the
>> packet payload as well. But we siply ignore that case because of the
>> return later originated from your patch. Checking for modulo allows
>> faster detection and it also allows detection when UDP packets
>> contain a single MPEGTS packet.
>> 
>> >Also it's possible
>> >for the pos of two sync byte > 188/192/204 * n. If that's true, it's not correct
>> >to look it as stats hit by the modulo logic.
>> 
>> I don't understand what you mean here.
>
> Let me give one example, the size is 192*2, but the second 192 isn't start by 0x47
> sync byte, checking with modulo will not correct for the case

The mpegts demuxer can only detect 188, 192, or 204 byte packet sizes. 
Arbitrary sizes (like 2*192) is simply not supported, so we don't have to 
detect it.

Regards,
Marton

>
>
>> 
>> Regards,
>> Marton
>> 
>> 
>> >
>> >>
>> >>Also skip unrecognized sync byte distances instead of considering them as a
>> >>failure of detection. It only delays the detection of the new packet size.
>> >>
>> >>Also note that AVIO only buffers a single packet (for UDP/mpegts, that usually
>> >>means 1316 bytes), so among every ten consecutive 188-byte MPEGTS packets there
>> >>will always be a seek failure, and that caused the old code to not find the 188
>> >>byte pattern across 10 consecutive packets.
>> >>
>> >>Signed-off-by: Marton Balint <cus@passwd.hu>
>> >>---
>> >> libavformat/mpegts.c | 8 +++++---
>> >> 1 file changed, 5 insertions(+), 3 deletions(-)
>> >>
>> >>diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> >>index a065c61c40..f2b2c05d86 100644
>> >>--- a/libavformat/mpegts.c
>> >>+++ b/libavformat/mpegts.c
>> >>@@ -2846,12 +2846,14 @@ static void reanalyze(MpegTSContext *ts) {
>> >>     if (pos < 0)
>> >>         return;
>> >>     pos -= ts->pos47_full;
>> >>-    if (pos == TS_PACKET_SIZE) {
>> >>+    if (pos % TS_PACKET_SIZE == 0) {
>> >>         ts->size_stat[0] ++;
>> >>-    } else if (pos == TS_DVHS_PACKET_SIZE) {
>> >>+    } if (pos % TS_DVHS_PACKET_SIZE == 0) {
>> >>         ts->size_stat[1] ++;
>> >>-    } else if (pos == TS_FEC_PACKET_SIZE) {
>> >>+    } if (pos % TS_FEC_PACKET_SIZE == 0) {
>> >>         ts->size_stat[2] ++;
>> >>+    } else {
>> >>+        return;
>> >>     }
>> >>
>> >>     ts->size_stat_count ++;
>> >>-- 
>> >>2.26.1
>> >>
>> >>_______________________________________________
>> >>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".
>> >
>> >-- 
>> >Thanks,
>> >Limin Wang
>> >_______________________________________________
>> >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".
>> _______________________________________________
>> 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".
>
> -- 
> Thanks,
> Limin Wang
> _______________________________________________
> 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 20, 2020, 7:37 p.m. UTC | #6
On Wed, 20 May 2020, Michael Niedermayer wrote:

> On Tue, May 19, 2020 at 09:06:59PM +0200, Marton Balint wrote:
>> pos47_full is not updated for every packet, and for unseekable inputs the
>> resync logic might simply skip some 0x47 sync bytes. In order to detect these
>> let's check for modulo instead of exact value.
>>
>> Also skip unrecognized sync byte distances instead of considering them as a
>> failure of detection. It only delays the detection of the new packet size.
>>
>> Also note that AVIO only buffers a single packet (for UDP/mpegts, that usually
>> means 1316 bytes), so among every ten consecutive 188-byte MPEGTS packets there
>> will always be a seek failure, and that caused the old code to not find the 188
>> byte pattern across 10 consecutive packets.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mpegts.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> index a065c61c40..f2b2c05d86 100644
>> --- a/libavformat/mpegts.c
>> +++ b/libavformat/mpegts.c
>> @@ -2846,12 +2846,14 @@ static void reanalyze(MpegTSContext *ts) {
>>      if (pos < 0)
>>          return;
>>      pos -= ts->pos47_full;
>> -    if (pos == TS_PACKET_SIZE) {
>> +    if (pos % TS_PACKET_SIZE == 0) {
>>          ts->size_stat[0] ++;
>> -    } else if (pos == TS_DVHS_PACKET_SIZE) {
>> +    } if (pos % TS_DVHS_PACKET_SIZE == 0) {
>>          ts->size_stat[1] ++;
>> -    } else if (pos == TS_FEC_PACKET_SIZE) {
>> +    } if (pos % TS_FEC_PACKET_SIZE == 0) {
>>          ts->size_stat[2] ++;
>> +    } else {
>> +        return;
>>      }
>
> This patch breaks, or at least changes
>
> ./ffmpeg -i tspacket_size_changeback.ts  -vframes 2 -qscale 2 test.avi
>
> ill mail you the file privatly as i dont remember from where this is
> and google fails to find any public copy

Yeah, it seems worse now. I don't know, this whole resync logic seems 
bogus to me, and it also duplicates functionality of get_packet_size. I 
will send a new patch which uses that instead of re-implementing it.

Regards,
Marton
diff mbox series

Patch

diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index a065c61c40..f2b2c05d86 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -2846,12 +2846,14 @@  static void reanalyze(MpegTSContext *ts) {
     if (pos < 0)
         return;
     pos -= ts->pos47_full;
-    if (pos == TS_PACKET_SIZE) {
+    if (pos % TS_PACKET_SIZE == 0) {
         ts->size_stat[0] ++;
-    } else if (pos == TS_DVHS_PACKET_SIZE) {
+    } if (pos % TS_DVHS_PACKET_SIZE == 0) {
         ts->size_stat[1] ++;
-    } else if (pos == TS_FEC_PACKET_SIZE) {
+    } if (pos % TS_FEC_PACKET_SIZE == 0) {
         ts->size_stat[2] ++;
+    } else {
+        return;
     }
 
     ts->size_stat_count ++;