You could check section_syntax_indicator instead of table_id, section_syntax_indicator indicates whether table_id_extension etc exist.

I think this is a duplicate of #635281. Can you confirm it?

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.

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.

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).

Hi again, I see the point now. Gonn'a test it later.

(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.

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"

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).

This commit should be reverted too because TOT tables have a CRC:
http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=2611b12970d2c2254ecc3f0a6f02a09ffa82a16a

(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.

I have tested current git again and TDTs are parsed in the current git version.

(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.

Created attachment 178024 [details] [review]
TDT table metadata parsing bug (different filtering for packets that do not have section_syntax_indicator set)

I have attached patch (mpegts_packetizer_parse_section_header function), which uses section_syntax_indicator flag instead of looking at table pid.

So what's the conclusion here?

- do any patches need to be reverted?

- anything that should be committed before the release?

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,

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.

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.

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.

*ping*

Any smallish test stream as requested by Christian ?

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.

Latest mpegts changes in -bad (both in the section packetizer and in the parsing library) have fixed those issues.