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

[llyyr]

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.