[FFmpeg-devel] [PATCH] avformat/mpegts: correctly skip TP_extra_header in m2ts

[Hendrik Leppkes]

On Sat, Nov 30, 2024 at 8:30 PM llyyr <llyyr.public at gmail.com> wrote:
>
> On 8/8/24 1:44 AM, llyyr wrote:
> > On 6/7/24 1:29 PM, Hendrik Leppkes wrote:
> >> On Fri, Jun 7, 2024 at 9:47 AM Hendrik Leppkes <h.leppkes at gmail.com>
> >> wrote:
> >>>
> >>> On Mon, May 27, 2024 at 3:47 PM llyyr <llyyr.public at 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 at 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
>
> ping
>
> I don't think handling it in mpegts_resync is the right fix because it's
> not a "special case" for specific files when m2ts is a standard and
> commonly found in blurays and HDV cameras. I don't think there should be
> any fallout from this. I've already tested the patch and it passes FATE,
> along with other ts/m2ts samples I had.
>
> if it must be done in mpegts_resync, then it would need to accompany
> other changes in read_packet as well and would result in a much larger
> patch anyway.

It does not necessarily have to be done in resync, however it must
cooperate with resync in general, if its triggered for whatever reason
(a corrupt file or bad cut).

I'll try to test this next week.

- Hendrik