On Sat, Nov 30, 2024 at 8:30 PM llyyr <llyyr.pub...@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.lepp...@gmail.com> > >> wrote: > >>> > >>> On Mon, May 27, 2024 at 3:47 PM llyyr <llyyr.pub...@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.pub...@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.
I went over the logic again and cleared up one of my concerns, as well as tested with a variety of samples. Couldn't find any regressions. So LGTM. - Hendrik _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
