Message ID | D1KH1R9UF2X7.7AQL6DW1P6EV@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avformat/mpegts: correctly skip TP_extra_header in m2ts | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
On Mon, May 27, 2024 at 3:47 PM llyyr <llyyr.public@gmail.com> wrote: > > instead of just resyncing and skipping a bunch of TS packets, leading to > a loss of frames. > > Before this, a stray byte with the value of 0x47 in TP_extra_header > would throw off the detection of where TS packets start. > > A typical file that could cause issues would look like this: > > 00000300: 238f 4780 4750 1110 0000 01e0 0000 84c0 > ^^ ^^ > The first four bytes here are TP_extra_header and the actual TS packet > starts at offset 0x304 > > FFmpeg would try to read a packet at 0x300 but since nothing skips the > 4 byte TP_extra_header, find that the first byte is not 0x47 and > immediately go into mpegts_resync, and incorrectly detect the stray 0x47 > in the TP_extra_header at 0x302 as the new sync byte. > > Fix this by correctly skipping the first 4 bytes if the source packet > is 192 bytes. > > Signed-off-by: llyyr <llyyr.public@gmail.com> > --- > libavformat/mpegts.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index c66a1ea6ede0..0d80ad80f1aa 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -2944,6 +2944,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size, > AVIOContext *pb = s->pb; > int len; > > + // 192 bytes source packet that start with a 4 bytes TP_extra_header > + // followed by 188 bytes of TS packet. The sync byte is at offset 4, so skip > + // the first 4 bytes otherwise we'll end up syncing to the wrong packet. > + if (raw_packet_size == TS_DVHS_PACKET_SIZE) > + avio_skip(pb, 4); > + I think this doesn't work with mpegts_resync, since it always resyncs for the packet start to be on the 0x47 marker, not 4 bytes before it. So if sync is lost, it would never recover, if I read this right. - Hendrik
On Fri, Jun 7, 2024 at 9:47 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > On Mon, May 27, 2024 at 3:47 PM llyyr <llyyr.public@gmail.com> wrote: > > > > instead of just resyncing and skipping a bunch of TS packets, leading to > > a loss of frames. > > > > Before this, a stray byte with the value of 0x47 in TP_extra_header > > would throw off the detection of where TS packets start. > > > > A typical file that could cause issues would look like this: > > > > 00000300: 238f 4780 4750 1110 0000 01e0 0000 84c0 > > ^^ ^^ > > The first four bytes here are TP_extra_header and the actual TS packet > > starts at offset 0x304 > > > > FFmpeg would try to read a packet at 0x300 but since nothing skips the > > 4 byte TP_extra_header, find that the first byte is not 0x47 and > > immediately go into mpegts_resync, and incorrectly detect the stray 0x47 > > in the TP_extra_header at 0x302 as the new sync byte. > > > > Fix this by correctly skipping the first 4 bytes if the source packet > > is 192 bytes. > > > > Signed-off-by: llyyr <llyyr.public@gmail.com> > > --- > > libavformat/mpegts.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > index c66a1ea6ede0..0d80ad80f1aa 100644 > > --- a/libavformat/mpegts.c > > +++ b/libavformat/mpegts.c > > @@ -2944,6 +2944,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size, > > AVIOContext *pb = s->pb; > > int len; > > > > + // 192 bytes source packet that start with a 4 bytes TP_extra_header > > + // followed by 188 bytes of TS packet. The sync byte is at offset 4, so skip > > + // the first 4 bytes otherwise we'll end up syncing to the wrong packet. > > + if (raw_packet_size == TS_DVHS_PACKET_SIZE) > > + avio_skip(pb, 4); > > + > > I think this doesn't work with mpegts_resync, since it always resyncs > for the packet start to be on the 0x47 marker, not 4 bytes before it. > So if sync is lost, it would never recover, if I read this right. > Since we're dealing with a special case for 192 byte packets here anyway, maybe a special case in mpegts_resync that just checks if raw size = 192 && [4] = 0x47 would handle this with less potential fallout? - Hendrik
On 6/7/24 1:29 PM, Hendrik Leppkes wrote: > On Fri, Jun 7, 2024 at 9:47 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote: >> >> On Mon, May 27, 2024 at 3:47 PM llyyr <llyyr.public@gmail.com> wrote: >>> >>> instead of just resyncing and skipping a bunch of TS packets, leading to >>> a loss of frames. >>> >>> Before this, a stray byte with the value of 0x47 in TP_extra_header >>> would throw off the detection of where TS packets start. >>> >>> A typical file that could cause issues would look like this: >>> >>> 00000300: 238f 4780 4750 1110 0000 01e0 0000 84c0 >>> ^^ ^^ >>> The first four bytes here are TP_extra_header and the actual TS packet >>> starts at offset 0x304 >>> >>> FFmpeg would try to read a packet at 0x300 but since nothing skips the >>> 4 byte TP_extra_header, find that the first byte is not 0x47 and >>> immediately go into mpegts_resync, and incorrectly detect the stray 0x47 >>> in the TP_extra_header at 0x302 as the new sync byte. >>> >>> Fix this by correctly skipping the first 4 bytes if the source packet >>> is 192 bytes. >>> >>> Signed-off-by: llyyr <llyyr.public@gmail.com> >>> --- >>> libavformat/mpegts.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c >>> index c66a1ea6ede0..0d80ad80f1aa 100644 >>> --- a/libavformat/mpegts.c >>> +++ b/libavformat/mpegts.c >>> @@ -2944,6 +2944,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size, >>> AVIOContext *pb = s->pb; >>> int len; >>> >>> + // 192 bytes source packet that start with a 4 bytes TP_extra_header >>> + // followed by 188 bytes of TS packet. The sync byte is at offset 4, so skip >>> + // the first 4 bytes otherwise we'll end up syncing to the wrong packet. >>> + if (raw_packet_size == TS_DVHS_PACKET_SIZE) >>> + avio_skip(pb, 4); >>> + >> >> I think this doesn't work with mpegts_resync, since it always resyncs >> for the packet start to be on the 0x47 marker, not 4 bytes before it. >> So if sync is lost, it would never recover, if I read this right. >> > > Since we're dealing with a special case for 192 byte packets here > anyway, maybe a special case in mpegts_resync that just checks if raw > size = 192 && [4] = 0x47 would handle this with less potential > fallout? I've looked at this again and I don't see a way to gracefully handle it in mpegts_resync. It's much cleaner to just skip the header that's not relevant to us so we line up with the first packet being the sync byte. FWIW this can be reproduced with `ffmpeg -i <file> -c copy out.h264` Sample file: https://0x0.st/XVJi.m2ts
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index c66a1ea6ede0..0d80ad80f1aa 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -2944,6 +2944,12 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size, AVIOContext *pb = s->pb; int len; + // 192 bytes source packet that start with a 4 bytes TP_extra_header + // followed by 188 bytes of TS packet. The sync byte is at offset 4, so skip + // the first 4 bytes otherwise we'll end up syncing to the wrong packet. + if (raw_packet_size == TS_DVHS_PACKET_SIZE) + avio_skip(pb, 4); + for (;;) { len = ffio_read_indirect(pb, buf, TS_PACKET_SIZE, data); if (len != TS_PACKET_SIZE) @@ -2966,7 +2972,11 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size, static void finished_reading_packet(AVFormatContext *s, int raw_packet_size) { AVIOContext *pb = s->pb; - int skip = raw_packet_size - TS_PACKET_SIZE; + int skip; + if (raw_packet_size == TS_DVHS_PACKET_SIZE) + skip = raw_packet_size - TS_DVHS_PACKET_SIZE; + else + skip = raw_packet_size - TS_PACKET_SIZE; if (skip > 0) avio_skip(pb, skip); }
instead of just resyncing and skipping a bunch of TS packets, leading to a loss of frames. Before this, a stray byte with the value of 0x47 in TP_extra_header would throw off the detection of where TS packets start. A typical file that could cause issues would look like this: 00000300: 238f 4780 4750 1110 0000 01e0 0000 84c0 ^^ ^^ The first four bytes here are TP_extra_header and the actual TS packet starts at offset 0x304 FFmpeg would try to read a packet at 0x300 but since nothing skips the 4 byte TP_extra_header, find that the first byte is not 0x47 and immediately go into mpegts_resync, and incorrectly detect the stray 0x47 in the TP_extra_header at 0x302 as the new sync byte. Fix this by correctly skipping the first 4 bytes if the source packet is 192 bytes. Signed-off-by: llyyr <llyyr.public@gmail.com> --- libavformat/mpegts.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) base-commit: e9197db4f7227a341b781c227a0a8f3a99033b5e