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 639056 - mpeg-ts: TDT tables are not parsed correctly
mpeg-ts: TDT tables are not parsed correctly
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal major
: 1.1.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-01-09 12:01 UTC by Karol Sobczak
Modified: 2013-07-18 07:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
TDT table metadata parsing bug (3.66 KB, patch)
2011-01-09 12:01 UTC, Karol Sobczak
none Details | Review
TDT table metadata parsing bug (different filtering for packets that do not have section_syntax_indicator set) (3.69 KB, patch)
2011-01-11 12:10 UTC, Karol Sobczak
none Details | Review
0001-mpegtsparse-really-fix-handling-of-TDT-and-TOT-table.patch (4.59 KB, patch)
2011-03-21 15:38 UTC, Andoni Morales
none Details | Review

Description Karol Sobczak 2011-01-09 12:01:58 UTC
Created attachment 177868 [details] [review]
TDT table metadata parsing bug

TDT tables are not parsed correctly. Please have a look at the current mpegts_packetizer_parse_section_header function implementation in /gst/mpegdemux/mpegtspacketizer.c source file. In this function MPEGTS tables metadata is extracted. TDT tables does not contain fields (e.g current_next_indicator, service_id) that other tables (e.g EIT tables) have. However, in the current mpegts_packetizer_parse_section_header function implementation, TDT tables are treated as if they had those fields (could have caused memory faults). This causes ignoring of TDT packets. I have attached a patch, that fixes this issue. Please review the patch.
Comment 1 Raimo Järvi 2011-01-09 21:06:58 UTC
You could check section_syntax_indicator instead of table_id, section_syntax_indicator indicates whether table_id_extension etc exist.
Comment 2 Andoni Morales 2011-01-10 11:46:33 UTC
I think this is a duplicate of #635281. Can you confirm it?
Comment 3 Karol Sobczak 2011-01-10 12:31:47 UTC
Bug #635281 is related to the same problem, however patches there do not fix mpegts_packetizer_parse_section_header function, which also have a bug preventing TDT tables from being parsed.
Comment 4 Andoni Morales 2011-01-10 12:42:19 UTC
TDT sections do not extend to several packets, so you don't need to use the gst adapter anymore to wait for the section to be finished, and mpegts_packetizer_parse_section_header will never be called for TDT sections. 

These sections are parsed directly in http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/mpegdemux/mpegtspacketizer.c#n2195, and you jump after that  to http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/mpegdemux/mpegtspacketizer.c#n2297, so I doubt you can reproduce this bug with the latest git checkout.
Comment 5 Karol Sobczak 2011-01-10 12:47:49 UTC
From mpegts_packetizer_push_section: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/mpegdemux/mpegtspacketizer.c#n2282, you go to http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/mpegdemux/mpegtspacketizer.c#n264, which parses TDT tables incorrectly (I used git from 09.01.2011 to test parsing of TDT tables).
Comment 6 Karol Sobczak 2011-01-10 12:49:36 UTC
Hi again, I see the point now. Gonn'a test it later.
Comment 7 Raimo Järvi 2011-01-10 17:12:15 UTC
(In reply to comment #4)
> TDT sections do not extend to several packets, so you don't need to use the gst
> adapter anymore to wait for the section to be finished, and
> mpegts_packetizer_parse_section_header will never be called for TDT sections. 

Is it really guaranteed that TDT or especially TOT sections don't extend to
several TS packets? Maximum section_length for TOT is 1023, so it doesn't
necessarily fit in one TS packet.
Comment 8 Andoni Morales 2011-01-10 17:24:54 UTC
I think it's guaranteed, or at least this what I understand from the spec.

For TDT and TOT tables it says:
"The TDT shall consist of a single section using the syntax of table 8"
"The TOT shall consist of a single section using the syntax of table 9"

Instead for other tables, like EIT it says:
"The EIT shall be segmented into event_information_sections using the syntax of table 7"
Comment 9 Andoni Morales 2011-01-10 17:38:22 UTC
But I think you are right, the TOT table can extend to more than one packet depending on the number of Local time offset descriptors. (This is not the case for the TDT table, which has a fixed size).
Comment 10 Andoni Morales 2011-01-10 17:48:21 UTC
This commit should be reverted too because TOT tables have a CRC:
http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=2611b12970d2c2254ecc3f0a6f02a09ffa82a16a
Comment 11 Raimo Järvi 2011-01-10 17:53:27 UTC
(In reply to comment #9)
> But I think you are right, the TOT table can extend to more than one packet
> depending on the number of Local time offset descriptors. (This is not the case
> for the TDT table, which has a fixed size).

Even if section size is less than 184 (like it always is for TDT), it could extend to two TS packets if the first TS packet starts with enough data from another section (e.g. the end of a big TOT section). For TDT this is not very likely, but it's theoretically possible.
Comment 12 Karol Sobczak 2011-01-10 20:32:00 UTC
I have tested current git again and TDTs are parsed in the current git version.
Comment 13 Andoni Morales 2011-01-11 10:04:25 UTC
(In reply to comment #12)
> I have tested current git again and TDTs are parsed in the current git version.

But like Raimo noticed, my approach is not correct and your patch seams the best way to do it.
Comment 14 Karol Sobczak 2011-01-11 12:10:25 UTC
Created attachment 178024 [details] [review]
TDT table metadata parsing bug (different filtering for packets that do not have section_syntax_indicator set)
Comment 15 Karol Sobczak 2011-01-11 12:11:54 UTC
I have attached patch (mpegts_packetizer_parse_section_header function), which uses section_syntax_indicator flag instead of looking at table pid.
Comment 16 Tim-Philipp Müller 2011-01-14 00:44:29 UTC
So what's the conclusion here?

- do any patches need to be reverted?

- anything that should be committed before the release?
Comment 17 Andoni Morales 2011-01-14 09:45:17 UTC
I would revert 2611b12970d2c2254ecc3f0a6f02a09ffa82a16a and include the following the following hunk in the Karol's patch:

index a86ef0e..b61dca4 100644
--- a/gst/mpegdemux/mpegtspacketizer.c
+++ b/gst/mpegdemux/mpegtspacketizer.c
@@ -2192,22 +2192,6 @@ mpegts_packetizer_push_section (MpegTSPacketizer * packetizer,
 
     data += pointer;
   }
-  /* TDT and TOT sections (see ETSI EN 300 468 5.2.5)
-   *  these sections do not extend to several packets so we don't need to use the
-   *  sections filter. */
-  if (packet->pid == 0x14) {
-    table_id = data[0];
-    section->section_length = GST_READ_UINT24_BE (data) & 0x000FFF;
-    section->buffer = gst_buffer_create_sub (packet->buffer,
-        data - GST_BUFFER_DATA (packet->buffer), section->section_length + 3);
-    section->table_id = table_id;
-    section->complete = TRUE;
-    res = TRUE;
-    GST_DEBUG ("TDT section pid:%d table_id:%d section_length: %d\n",
-        packet->pid, table_id, section->section_length);
-    goto out;
-  }
-
   /* create a sub buffer from the start of the section (table_id and
    * section_length included) to the end */
   sub_buf = gst_buffer_create_sub (packet->buffer,
Comment 18 Edward Hervey 2011-01-14 10:00:27 UTC
I will be testing this over the weekend. Managed to reproduce the bug... but it's tricky to reproduce. The overall proposal is sane, but needs double-checking.
Comment 19 Andoni Morales 2011-03-21 15:38:33 UTC
Created attachment 183950 [details] [review]
0001-mpegtsparse-really-fix-handling-of-TDT-and-TOT-table.patch

Attached a patch that applies to current git with a slightly different approach to make the it more readable.
Comment 20 Christian Fredrik Kalager Schaller 2011-05-26 12:49:03 UTC
Hi Andoni, do you happen to have a small testfile which exposes the issues your patch fixes? Since mpegts is such a painful format in general it would just be nice to have that to verify with before committing this patch.
Comment 21 Vincent Penquerc'h 2011-11-01 11:24:09 UTC
*ping*

Any smallish test stream as requested by Christian ?
Comment 22 Andoni Morales 2011-11-01 11:41:35 UTC
Sorry I don't have any. We usually have a disker storing the TS steam, but it was segfaulting before reaching the sink. The only thing I can say is that with bad weather we used to have many segfaults of the same type and we have this patch running on platform for several months and the issue hasn't been reproduced.
Comment 23 Edward Hervey 2013-07-18 07:37:36 UTC
Latest mpegts changes in -bad (both in the section packetizer and in the parsing library) have fixed those issues.