Patch to EIT table version handling in DVBStreamData

[Roger James <roger@…>]

DVBStreamData current uses code from MPEGStreamData to handle redundancy (duplicate checking) and version checking. This is fine where the tables concerned only use the mpeg PSIPTable TableIDExtension field to uniquely identify instances of sub tables. This is not the case for all DVB tables. In particular the Service Desctiption Table (SDT) and the Event Information Table (see ETSI EN 300 468 V1.15.1, section 3.1 Definitions, definition of sub_table). This patch currently deals with the EIT only. In the EIT case a unique instance of a subtable is identified by the TableID, the TableIDExtension(service_id), the transport stream id, and the original network id.

This patch implements a 64 bit key constructed from the above 4 values as an index instead of the 32 bit key used in the current code.

I have also implemented the following two items.

A function to check EIT table completeness in line with some of the other tables. I think it would be a good idea to pass this flag on to the EIT handling code where it can be used to delay handling of tables sections until the table is complete thus saving unnecessary scheduler calls. It would be even betwwen if the caching necessary to acheive this was also implemented in DVBStreamData in line with the other tables that do this.

Code to handle the "severe" (their words) derogation from the ETSI standard regarding the original network id field here in the UK. As an aside I suspect that the EIT handling only worked here in the UK because the EIT table version handling is broken.

Roger

[Stuart Auchterlonie]

Roger,

Have you seen the code on #11098 and do you think this would be useful to what you have been implementing?

Regards Stuart

[Roger James <roger@…>]

Please ignore all the files I have attached. I took them from the wrong tree. Can someone with admin privilege please remove them (including the original ones) and I will attach the correct ones.

[Roger James <roger@…>]

Ok I think I got it right this time. Thanks Stuart.

Ignore the original dvbstreamdata files (Stuart, you can remove those too if you want)

I have used #11098 as a basis for this change. This version has moved the robust version handling down into dvbstreamdata and mpegstreamdata. Consider this a beta version. I have tested it against UK DVB-T and it seems OK. I would be great if people could test this code. Especially for regressions against the current dvb and atsc handling with other broadcasters. It signficantly reduces the (hopefully eliminates) the duplicate sections currently passed up to the EIT code. In the future I would like to further reduce the load on the EIT code by passing up complete tables when a version change is complete. I have put the code to check completeness in but it is inactive at the moment. The next step would be to implement caching of EIT sections in the same way that NIT and SDT sections are currently cached. Comments please on that :-)

I have not yet addressed the accurate scheduling stuff. I will return to that once I am sure this stuff is robust.

[Roger James <roger@…>]

A weakness of this implementation is that it cannot detect version number wrap round. This happens when a table version change is left incomplete, usually caused by loss or interruption of a transport stream. If when the stream is retuned sufficient time has elapsed for the version number to have rotated all the way round (modulo 32) the table may be incorrectly marked as complete using some segments with old data.

I have been thinking about algorithms to address this, and would like some help! Think of this as a competition where the only prize is a better mythtv. Here are some thoughts.

Time stamping the version change.

Using the ETSI minimum table repetition rate standards.

I am getting far too old to be doing this stuff!

Roger

[Roger James <roger@…>]

Add

Time stamp last table section seen.

To the above list.

[Roger James <roger@…>]

Just attached a zip of the latest changes.

I really appreciate if anyone could review and test my current rework of DVB handlng of EI and SD tables. I have attached an archive of the changed files. This rework fixes the identification of EI and SD sub-tables and adds caching of EI sub-tables. The identification now complies with ETSI EN 300 468 V1.15.1 (2016-03), as below.

sub_table: collection of sections with the same value of table_id and:

• for a NIT: the same table_id_extension (network_id) and version_number;
• for a BAT: the same table_id_extension (bouquet_id) and version_number;
• for a SDT: the same table_id_extension (transport_stream_id), the same original_network_id and version_number;
• for a EIT: the same table_id_extension (service_id), the same transport_stream_id, the same original_network_id and version_number.

NOTE: The table_id_extension field is equivalent to the fourth and fifth byte of a section when the section_syntax_indicator is set to a value of "1".

The code has only been tested against DVB-T here in the UK. It needs testing against other broadcast providers. I have not fully tested the changes in the SD table handling. These are only used when scanning for channels.

Roger

[Stuart Auchterlonie]

Hi Roger,

Looking to try and get this tested and merged in over the holidays.

Regards Stuart

[Stuart Auchterlonie]

Just doing some testing on this "as is" to see if there are any regressions, and i'm reliably crashing this within about 15mins from startup. It's crashing here

Thread 5 "TVRecEvent" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f97b67db700 (LWP 28463)]
0x00007f97d5aae366 in QGenericAtomicOps<QBasicAtomicOps<4> >::load<int> (
    _q_value=@0x25: <error reading variable>)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qgenericatomic.h:90
90              return _q_value;

Up the stack a bit

#10 0x00007f97d5cd2989 in QMap<unsigned short, ServiceDescriptionTable*>::end (this=0x7f96f801f6b8)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qmap.h:530
No locals.
#11 0x00007f97d5cc80d1 in DVBStreamData::Reset (this=0x7f97ac18ef30, desired_netid=9018, desired_tsid=24640,
    desired_serviceid=25664) at mpeg/dvbstreamdata.cpp:285
        section = {i = 0x7f96f801f660}
        table = {i = 0x7f96f801f698}
        stream = {i = 0x7f96f801f1a0}
        network = {i = 0x7f96f801f770}
        nit_sections = std::vector of length 3, capacity 4 = {0x7f96f83493a0, 0x7f96f81b0300, 0x7f96f8985b60}

Some digging

#11 0x00007f97d5cc80d1 in DVBStreamData::Reset (this=0x7f97ac18ef30, desired_netid=9018, desired_tsid=24640, 
    desired_serviceid=25664) at mpeg/dvbstreamdata.cpp:285
285                                 section != (*table).sections.end(); ++table)

(gdb) print *section
$12 = (ServiceDescriptionTable *&) @0x7f96f801f680: 0x0

So by my reading section is pointing to a null object. It's the calls to end() in the iterator that are causing it to blow up.

The contents of (*table).sections also look fishy...

(gdb) print (*table).sections
$16 = {d = 0x25}

looking at the table object itself

(gdb) print (*table)
$15 = (SdtSectionsAndStatus &) @0x7f96f801f6b8: {sections = {d = 0x25}, status = {m_version = -519974397,
    m_sections = std::vector of length -140285996814963, capacity -140285996814850 = {97 'a', 95 '_',
      159 '\237', 146 '\222', 80 'P', 250 '\372', 60 '<', 107 'k', 32 ' ', 193 '\301', 112 'p', 248 '\370',
      96 '`', 64 '@', 35 '#', 58 ':', 112 'p', 81 'Q', 85 'U', 73 'I', 225 '\341', 159 '\237', 24 '\030',
      0 '\000', 0 '\000', 0 '\000', 49 '1', 0 '\000', 1 '\001', 11 '\v', 84 'T', 2 '\002', 80 'P', 0 '\000',
      77 'M', 213 '\325', 101 'e', 110 'n', 103 'g', 31 '\037', 80 'P', 111 'o', 119 'w', 101 'e', 114 'r',
      32 ' ', 82 'R', 97 'a', 110 'n', 103 'g', 101 'e', 114 'r', 115 's', 58 ':', 32 ' ', 68 'D', 105 'i',
      110 'n', 111 'o', 32 ' ', 83 'S', 117 'u', 112 'p', 101 'e', 114 'r', 99 'c', 104 'h', 97 'a', 114 'r',
      103 'g', 101 'e', 177 '\261', 76 'L', 111 'o', 118 'v', 101 'e', 32 ' ', 65 'A', 116 't', 32 ' ', 70 'F',
      105 'i', 114 'r', 203 '\313', 69 'E', 19 '\023', 46 '.', 101 'e', 215 '\327', 169 '\251', 55 '7',
      118 'v', 189 '\275', 185 '\271', 232 '\350', 153 '\231', 105 'i', 45 '-', 187 '\273', 90 'Z', 188 '\274',
      56 '8', 191 '\277', 11 '\v', 149 '\225', 180 '\264', 117 'u', 233 '\351', 78 'N', 226 '\342', 75 'K',
      213 '\325', 240 '\360', 254 '\376', 200 '\310', 61 '=', 100 'd', 109 'm', 251 '\373', 78 'N', 173 '\255',
      150 '\226', 57 '9', 112 'p', 80 'P', 6 '\006', 245 '\365', 11 '\v', 11 '\v', 117 'u', 110 'n', 100 'd',
      80 'P', 6 '\006', 246 '\366', 5 '\005', 12 '\f', 101 'e', 110 'n', 103 'g', 80 'P', 6 '\006', 242 '\362',
      64 '@', 148 '\224', 101 'e', 110 'n', 103 'g', 80 'P', 6 '\006', 243 '\363', 20 '\024', 5 '\005',
      101 'e', 110 'n', 103 'g', 126 '~', 1 '\001', 247 '\367', 84 'T', 2 '\002', 128 '\200', 0 '\000', 95 '_',
      4 '\004', 0 '\000', 0 '\000', 35 '#', 58 ':', 137 '\211', 15 '\017', 0 '\000', 101 'e', 110 'n', 103 'g',
      31 '\037', 1 '\001', 222 '\336', 159 '\237', 164 '\244', 42 '*', 93 ']', 211 '\323', 221 '\335',
      174 '\256', 240 '\360', 118 'v', 12 '\f', 196 '\304', 10 '\n', 47 '/', 54 '6', 51 '3', 57 '9', 55 '7',
      55 '7', 47 '/', 48 '0', 48 '0', 49 '1'...}, static init_bits = "\376\374\370\360\340\300\200"},
  timestamp = {d = {d = 0x7f96f801f9a0}}}

That's the notes so far, work continues....

[Stuart Auchterlonie]

I believe I know what's going wrong, the innermost iterator, should be doing a ++section not a ++table. ie.

                    for (sdt_sections_cache_t::iterator section = (*table).sections.begin();
                            section != (*table).sections.end(); ++section)
                        DeleteCachedTableSection(*section);

[Stuart Auchterlonie]

The above code change stopped it crashing, however there's now a memory leak to track down. backend size grew from ~750Mb RSS (compared to the equivalent 0.28 backend at ~200Mb RSS) up to 6Gb RSS overnight.

Work continues....

[Roger James <roger@…>]

Stuat. Which code base are you working from? Is it the original pull request, or one of the attachments here? If I can identify the point in my commit log you are working from then I can do a diff from there to see if there are any code changes in my tree that might be useful.

I will also start a soak test running to see what I can find this end.

You certainly found that cut and paste problem a lot more quickly then I did!

I would suspect the SDT code for the leak. I did do some soak tests on the EIT stuff and that looked OK. But who knows. This level of change is very risky.

Roger

[Stuart Auchterlonie]

Roger,

I'm working from the tarball on this ticket ​https://code.mythtv.org/trac/attachment/ticket/12932/dvbeitsdt.tbz

[Roger James <roger@…>]

A couple of recent commits

roger@dragon:~/mythdev/mythrfj$ git show f37a341
commit f37a341c89b6c0416faab6d3a3ea06baace6514c
Author: Roger James <roger@beardandsandals.co.uk>
Date:   Thu Dec 29 11:43:03 2016 +0000

    Bug fix in SD table handling

diff --git a/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp b/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp
index 5a41b08..c5f010c 100644
--- a/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp
+++ b/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp
@@ -273,7 +273,7 @@ void DVBStreamData::Reset(uint desired_netid, uint desired_tsid,
             DeleteCachedTableSection(*nit);
         _cached_nit.clear();

-        //ValidateSDTCache();
+        ValidateSDTCache();

         for (sdt_tsn_cache_t::iterator network = _cached_sdts.begin();
                 network != _cached_sdts.end(); ++network)
@@ -974,7 +974,7 @@ sdt_const_vec_t DVBStreamData::GetCachedSDTs() const             for (sdt_t_cache_t::iterator table = (*stream).begin(); table != (*stream).end(); ++table)
             {
                 for (sdt_sections_cache_t::iterator section = (*table).sections.begin();
-                        section != (*table).sections.end(); ++table)
+                        section != (*table).sections.end(); ++section)
                     sdts.push_back(*section);
             }
         }
@@ -1164,8 +1164,9 @@ void DVBStreamData::ValidateEITCache()
             }
         }
     }
-    LOG(VB_DVBSICACHE|VB_FLUSH, LOG_DEBUG, LOC + QString(
+    LOG(VB_DVBSICACHE, LOG_DEBUG, LOC + QString(
             "============================================="));
+    LOG(VB_FLUSH, LOG_DEBUG, "");
 }


@@ -1202,8 +1203,9 @@ void DVBStreamData::ValidateSDTCache()
             }
         }
     }
-    LOG(VB_DVBSICACHE|VB_FLUSH, LOG_DEBUG, LOC + QString(
+    LOG(VB_DVBSICACHE, LOG_DEBUG, LOC + QString(
             "============================================="));
+    LOG(VB_FLUSH, LOG_DEBUG, "");
 }


roger@dragon:~/mythdev/mythrfj$ git show f2811df
commit f2811df18874356d201d9dd9eef26222465c8916
Author: Roger James <roger@beardandsandals.co.uk>
Date:   Thu Dec 29 16:17:48 2016 +0000

    Another SD table fix

diff --git a/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp b/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp
index c5f010c..2addcdf 100644
--- a/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp
+++ b/mythtv/libs/libmythtv/mpeg/dvbstreamdata.cpp
@@ -959,8 +959,9 @@ sdt_const_ptr_t DVBStreamData::GetCachedSDTSection(
 sdt_const_vec_t DVBStreamData::GetCachedSDTs() const
 {
     // This function is purely for use by channelscan_sm
-    // It should return all the actual SD table sections
-    // in the cache
+    // It returns a copy of all the actual SD table sections
+    // in the cache. It is the responsibility of the caller
+    // to delete these copies
     QMutexLocker locker(&_cache_lock);


@@ -975,7 +976,7 @@ sdt_const_vec_t DVBStreamData::GetCachedSDTs() const             {
                 for (sdt_sections_cache_t::iterator section = (*table).sections.begin();
                         section != (*table).sections.end(); ++section)
-                    sdts.push_back(*section);
+                    sdts.push_back(new ServiceDescriptionTable(**section));
             }
         }
     }
roger@dragon:~/mythdev/mythrfj$

[Roger James <roger@…>]

According to valgrind the code is leaking DVBEventInformationTable objects allocated at line 488 of dvbstreamdata.cpp.

I tried to get better information by compiling with -fsanitize=leak, but that does not seem to work with the Mythtv build. The flag shows on the compiler output but liblsan is not being linked. That is a rat hole I would rather not go down. I have a horrendous head cold at the moment so do not think I will get much done this weekend.

[Roger James <roger@…>]

I think at some time in the future we should rename the DVBEventInformationTable class to DVBEventInformationTableSection because that is what it is, not necessarily a complete table. The same applies to other related classes.

[Stuart Auchterlonie]

Moving remaining open tickets to 0.28.2 milestone

[Stuart Auchterlonie]

Moving remaining open tickets to 29.2 milestone

[Stuart Auchterlonie]

We have moved all bug tracking to github [1]

If you continue to have this issue, please open a new issue at github, referencing this ticket.

[1] - ​https://github.com/MythTV/mythtv/issues