GNOME Bugzilla – Bug 786111
tsdemux: incorrectly parsing non-timestamp byte sequence in PES header as PTS time stamp
Last modified: 2017-09-03 13:16:37 UTC
Test file: http://s3.amazonaws.com/fox_misc/aperi_tr01_1s_pkts-C.cap.gz Unzip and rename to test.cap. Test pipeline: GST_DEBUG=3 gst-launch-1.0 -v filesrc location=~/test.cap ! pcapparse ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! imagefreeze ! ximagesink Warnings: 0:00:01.070779503 8815 0x1831050 WARN pesparser pesparse.c:416:mpegts_parse_pes_header: bad PTS value 0:00:01.070781019 8815 0x1831050 WARN tsdemux tsdemux.c:2253:gst_ts_demux_parse_pes_header: Error parsing PES header. pid: 0x45 stream_type: 0x6 0:00:01.070788151 8815 0x1831050 DEBUG pesparser pesparse.c:140:mpegts_parse_pes_header: header_size : 14 0:00:01.070821307 8815 0x1831050 WARN pesparser pesparse.c:416:mpegts_parse_pes_header: bad PTS value 0:00:01.070823313 8815 0x1831050 WARN tsdemux tsdemux.c:2253:gst_ts_demux_parse_pes_header: Error parsing PES header. pid: 0x42 stream_type: 0x6 Reading the beginning of the PES header in method mpegts_parse_pes_header, I get the following bytes: 0x80 0x0a 0x1b 0xd2 0xbd 0xfe 0x80 is a flag indicating a PTS, but the next byte should have its low bit set. Since it doesn't, the PTS is rejected. This same byte pattern appears for each PES packet, so it doesn't look like corruption, as it is the same pattern. This can't be a PTS time stamp, because it doesn't change. So, it looks like tsdemux is incorrectly parsing these bytes as a PTS time stamp. Please note that the steam type is 0x6, indicating a private stream. Also #define ST_PS_VIDEO_MPEG2_DCII 0x80 So, perhaps this private stream is mistaken for a PTS.
error: File is not a libpcap file, magic is A1B23C4D
Nevermind, it's just that you included the ts stream itself and not a pcap dump. Those PTS are indeed invalid. Taking an example encoded PTS from the stream of 0x0a 0x1b 0xcc, it's already wrong at the first byte (0x0a): * Since this is PTS only, the first 4 bits should be 0010 (here it's 0000) * Then 3 bits of data (here 011) * Then a marker bit '1' (here 0) Where does this stream come from ?
> Those PTS are indeed invalid. > > Taking an example encoded PTS from the stream of 0x0a 0x1b 0xcc, it's > already wrong at the first byte (0x0a): > > * Since this is PTS only, the first 4 bits should be 0010 (here it's 0000) > * Then 3 bits of data (here 011) > * Then a marker bit '1' (here 0) > > Where does this stream come from ? I'm not quite sure how this was captured. Is it possible that this is a private sub-stream mistaken as a PTS ? Are there valid PTS in the stream ?
This stream is from an Aperi SDMP microserver (see http://www.apericorp.com/platform/ ) The format of the attached file is mpeg ts wrapped as pcap (with nanosecond time stamps)
Current master can handle nanosecond time stamps.
Indeed, when using master pcapparse it does fix some minor issues, but: * It still has PTS without marker bits (temporarily disabled it to move forward). * There is a failure parsing the jp2k access unit This is the beginning of the au it fails to parse: p2k_access_unit: 00000000: 65 6c 73 6d 66 72 61 74 03 e9 ea 60 62 72 61 74 elsmfrat...`brat p2k_access_unit: 00000010: 00 1e 84 80 00 04 f4 00 00 00 00 00 74 63 6f 64 ............tcod p2k_access_unit: 00000020: aa bb cc ab 62 63 6f 6c 02 ff 4f ff 51 00 2f 01 ....bcol..O.Q./. p2k_access_unit: 00000030: 02 00 00 05 00 00 00 02 d0 00 00 00 00 00 00 00 ................ p2k_access_unit: 00000040: 00 00 00 05 00 00 00 02 d0 00 00 00 00 00 00 00 ................ p2k_access_unit: 00000050: 00 00 03 09 01 01 09 02 01 09 02 01 ff 52 00 12 .............R.. This seems to be an interlaced stream ... which isn't properly advertised in the j2k_video_descriptor (let alone properly handled in the code). Talk about streams not respecting the standards ...
Created attachment 358661 [details] [review] tsdemux: Handle quirk in jp2k es header handling The jp2k specification (ITU-T T.800) specifies that the 'brat' box has two fields and the second one (AUF2) can be set to 0 for progressive streams. The problem is that the mpeg-ts specification (ITU-T H.222.0 06/2012) says that the AUF2 field is only present if the stream is interlaced In order to cope with both situation, accept those next 32bit if the stream is marked as progressive and those bits contain 0
Nice catch. So, with this patch, and ignoring the messed-up PTS, does this stream play on master ?
Comment on attachment 358661 [details] [review] tsdemux: Handle quirk in jp2k es header handling >From 8f88683929d40135a8ea0c5e985a9ba9981b0d07 Mon Sep 17 00:00:00 2001 >From: Edward Hervey <edward@centricular.com> >Date: Tue, 29 Aug 2017 11:14:59 +0200 >Subject: [PATCH] tsdemux: Handle quirk in jp2k es header handling > >The jp2k specification (ITU-T T.800) specifies that the 'brat' box >has two fields and the second one (AUF2) can be set to 0 for progressive >streams. > >The problem is that the mpeg-ts specification (ITU-T H.222.0 06/2012) >says that the AUF2 field is only present if the stream is interlaced > >In order to cope with both situation, accept those next 32bit if the >stream is marked as progressive and those bits contain 0 > >https://bugzilla.gnome.org/show_bug.cgi?id=786111 >--- > gst/mpegtsdemux/tsdemux.c | 6 ++++++ > 1 file changed, 6 insertions(+) > >diff --git a/gst/mpegtsdemux/tsdemux.c b/gst/mpegtsdemux/tsdemux.c >index f802ae0ff..67501fe8d 100644 >--- a/gst/mpegtsdemux/tsdemux.c >+++ b/gst/mpegtsdemux/tsdemux.c >@@ -2730,8 +2730,14 @@ parse_jp2k_access_unit (TSDemuxStream * stream) > goto error; > #endif > } >+ > /* Time Code Box 'tcod' == 0x74636f64 */ >+ /* Some progressive streams might have a AUF[1] of value 0 present */ > header_tag = gst_byte_reader_get_uint32_be_unchecked (&reader); >+ if (header_tag == 0 && !stream->jp2kInfos.interlace) { >+ AUF[1] = header_tag; Would it be clearer to set AUF[1] to 0 ? >+ header_tag = gst_byte_reader_get_uint32_be_unchecked (&reader); We should check that there is enough data to read this extra 32 bits for progressive. >+ } > if (header_tag != 0x74636f64) { > GST_ERROR_OBJECT (stream->pad, > "Expected Time code box but found %d box instead", header_tag); >-- >2.13.5
(In reply to Aaron Boxer from comment #8) > Nice catch. So, with this patch, and ignoring the messed-up PTS, does this > stream play on master ? No
The reason it's failing afterwards is because ... the header is actually 41 bytes (a bastard mix of expected 38 or 48 bytes). The last 8 bits (reserved) from the 'bcol' box are not present. Therefore we alway drop the first byte (0xff from the magic 0xff4fff51), resulting in unplayable streams.
Created attachment 358736 [details] [review] tsdemux: Handle quirk in jp2k es header handling The jp2k specification (ITU-T T.800) specifies that the 'brat' box has two fields and the second one (AUF2) can be set to 0 for progressive streams. The problem is that the mpeg-ts specification (ITU-T H.222.0 06/2012) says that the AUF2 field is only present if the stream is interlaced In order to cope with both situation, accept those next 32bit if the stream is marked as progressive and those bits contain 0
Created attachment 358737 [details] [review] tsdemux: Make jp2k handling more robust and efficient * Avoid copying the pending data and instead create a buffer directly from that data with the appropriate offset. * Locate the jp2k magic to determine the exact location of the (first) frame data instead of assuming that the header is of an expected size
With those two patches and PTS validation removed, I can get the video decoded. Please contact the vendor to have them fix the PTS magic issues, I'm not removing those checks.
Changes look good. Thanks.
So, this is interesting: From the H.222.0 spec: interlaced_video – This 1-bit field indicates whether the J2K video stream contains interlaced video. When this flag is set to '1' the J2K access unit elementary stream header (see Table S.1) shall include the syntax elements Auf2, fiel_box_code, fic and fio. When this flag is set to '0', these syntax elements shall not be present in the J2K access unit elementary stream header. But, from the table: // j2k_brat brat_box_code = '0x6272 6174' Maxbr Auf1 // If (interlaced_video == 1) { Auf2 } The table has a commented out if statement, which seems to imply that Auf2 is present for progressive. And this is what we see in this stream. Since the second brace is not commented out, my guess is that this is a typo in the table. I don't read standards documents that often :) so perhaps someone else can comment on this ?
Yes, Annex S of H.222.0 is indeed confusing. But other sources (VSF_TR-01 for ex) seem to confirm that the intent is for AUF2 to *not* be present with progressive sources. That being said ... the information in that header isn't useful/blocking for being able to handle jp2k-in-mpeg-ts. The only thing we need to validate is having an actual jp2k frame in there (which is located with the jp2k magic in my patches).
commit 52d0ef2665e0c620e503903d6eebe0771f92c5ba (HEAD -> master, origin/master, origin/HEAD) Author: Edward Hervey <edward@centricular.com> Date: Wed Aug 30 08:37:04 2017 +0200 tsdemux: Make jp2k handling more robust and efficient * Avoid copying the pending data and instead create a buffer directly from that data with the appropriate offset. * Locate the jp2k magic to determine the exact location of the (first) frame data instead of assuming that the header is of an expected size https://bugzilla.gnome.org/show_bug.cgi?id=786111 commit c393f0d768a5c86dade48d991fb7d890f6dc5221 Author: Edward Hervey <edward@centricular.com> Date: Tue Aug 29 11:14:59 2017 +0200 tsdemux: Handle quirk in jp2k es header handling The jp2k specification (ITU-T T.800) specifies that the 'brat' box has two fields and the second one (AUF2) can be set to 0 for progressive streams. The problem is that the mpeg-ts specification (ITU-T H.222.0 06/2012) says that the AUF2 field is only present if the stream is interlaced In order to cope with both situation, accept those next 32bit if the stream is marked as progressive and those bits contain 0 https://bugzilla.gnome.org/show_bug.cgi?id=786111
Great. Thanks for these patches.
Since the stream doesn't conform to spec, the problem is not with GStreamer, so I am going to close this.