After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 786111 - tsdemux: incorrectly parsing non-timestamp byte sequence in PES header as PTS time stamp
tsdemux: incorrectly parsing non-timestamp byte sequence in PES header as PTS...
Status: RESOLVED NOTABUG
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Windows
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-08-10 13:50 UTC by Aaron Boxer
Modified: 2017-09-03 13:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tsdemux: Handle quirk in jp2k es header handling (1.49 KB, patch)
2017-08-29 09:18 UTC, Edward Hervey
none Details | Review
tsdemux: Handle quirk in jp2k es header handling (1.70 KB, patch)
2017-08-30 06:39 UTC, Edward Hervey
committed Details | Review
tsdemux: Make jp2k handling more robust and efficient (3.45 KB, patch)
2017-08-30 06:39 UTC, Edward Hervey
committed Details | Review

Description Aaron Boxer 2017-08-10 13:50: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.
Comment 1 Edward Hervey 2017-08-28 07:33:50 UTC
error: File is not a libpcap file, magic is A1B23C4D
Comment 2 Edward Hervey 2017-08-28 07:59:08 UTC
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 ?
Comment 3 Aaron Boxer 2017-08-28 16:59:14 UTC
> 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 ?
Comment 4 Aaron Boxer 2017-08-29 01:06:06 UTC
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)
Comment 5 Aaron Boxer 2017-08-29 01:06:51 UTC
Current master can handle nanosecond time stamps.
Comment 6 Edward Hervey 2017-08-29 08:34:18 UTC
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 ...
Comment 7 Edward Hervey 2017-08-29 09:18:15 UTC
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
Comment 8 Aaron Boxer 2017-08-29 14:19:06 UTC
Nice catch. So, with this patch, and ignoring the messed-up PTS, does this stream play on master ?
Comment 9 Aaron Boxer 2017-08-29 14:20:21 UTC
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
Comment 10 Edward Hervey 2017-08-29 16:01:42 UTC
(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
Comment 11 Edward Hervey 2017-08-30 06:38:53 UTC
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.
Comment 12 Edward Hervey 2017-08-30 06:39:28 UTC
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
Comment 13 Edward Hervey 2017-08-30 06:39:34 UTC
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
Comment 14 Edward Hervey 2017-08-30 06:41:51 UTC
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.
Comment 15 Aaron Boxer 2017-08-30 14:39:14 UTC
Changes look good. Thanks.
Comment 16 Aaron Boxer 2017-08-31 21:01:39 UTC
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 ?
Comment 17 Edward Hervey 2017-09-01 08:51:24 UTC
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).
Comment 18 Edward Hervey 2017-09-01 08:56:57 UTC
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
Comment 19 Aaron Boxer 2017-09-01 12:01:39 UTC
Great. Thanks for these patches.
Comment 20 Aaron Boxer 2017-09-03 13:16:37 UTC
Since the stream doesn't conform to spec, the problem is not with GStreamer, so I am going to close this.