Opened 11 years ago

Closed 9 years ago

#11399 closed Patch - Bug Fix (Fixed)

EIT scanner causes unnecessary reschedules and prevents idle shutdown

Reported by: Roger James <roger@…> Owned by: Stuart Auchterlonie
Priority: minor Milestone: 0.28
Component: MythTV - EIT Version: 0.26-fixes
Severity: medium Keywords: eit idle
Cc: Ticket locked: no

Description

The handling of DVB table version numbers in eitcache.cpp is incorrect. This causes multiple unnecessary reschedules and as a consequence also prevents idle shutdown from occurring.

Apologies to those who are familiar with this stuff for the simple summary that follows.

DVB EIT specifies two types of tables "now next" tables and "schedule" tables. The "now next" table contains events referring to program content that is currently being transmitted or is to be transmitted immediately following the current program content. The "schedule" table contains events that refer to program content sometime in the future after the "now next" content. The tables do not overlap.

The tables are identified by tableids. The "now next" table is identified by two tableids (0x4e and 0x4f) although there are two ids they both refer conceptually to the same single table. The "schedule" table is identified by 32 tableids (0x50 - 0x6f) again these all refer conceptually to the same single table.

The current code in IsNewEIT (eitchache.cpp) treats every change of tableid to a lower number as a table change. I believe this is incorrect. The only thing the denotes a change to a table is a change of its version number.

I have attached a patch that fixes this. To avoid changing the database schema the patch reuses the tableid field in the eitcache table to store the now next table version number. The patch also contains a patch to eithelper.cpp that causes now next table updates to be ignored if they do not relate to recording rules. The patch also stops tables that are not "current" being actioned.

Roger

Attachments (3)

eitscannerpatch (7.9 KB) - added by Roger James <roger@…> 11 years ago.
eitscannerpatch.v2 (18.3 KB) - added by Roger James <roger@…> 11 years ago.
version 2 of eit scanner patch
eitscannerpatch.v3 (22.8 KB) - added by Roger James <roger@…> 11 years ago.
cleaned up version

Download all attachments as: .zip

Change History (21)

Changed 11 years ago by Roger James <roger@…>

Attachment: eitscannerpatch added

comment:1 Changed 11 years ago by dekarl@…

looking at your patch I noticed that you use literal values instead of the enum

class MTV_PUBLIC TableID
{
  enum
  {
    PF_EIT = 0x4e, // always on pid 0x12
    PF_EITo = 0x4f, // always on pid 0x12
    SC_EITbeg = 0x50, // always on pid 0x12
    SC_EITend = 0x5f, // always on pid 0x12
    SC_EITbego = 0x60, // always on pid 0x12
    SC_EITendo = 0x6f, // always on pid 0x12
  }
}

I have to look up the other points in the standards as most are new for me. (e.g. how the programmes get distributed into the different "TableIDs")

comment:2 Changed 11 years ago by dekarl@…

In the past I have successfully setup a backend with idle shutdown and EIT scanning by tweaking the timeouts for

  1. idle time between recordings that prohibits shutdown
  2. idle time before starting the EIT scan
  3. and the idle time after which the backend shuts down

by setting a > b > c the backend would scan EIT between recordings but shut down when the time until the next recording exceeded c. But I was not really satisfied with the result. The real question is, why does a reschedule reset the idle timer. (I remember a change where it would not reset the timer but bump it up to a minimum value of a minute or so to let the reschedule finish)

Now on to the patch.

According to ETR 211 4.1.4.2.1 L) "segments that correspond to events in the past may be replaced by empty segments" which is in contrast to "The tables do not overlap."

After reading ETR 211 and EN 300 468 it is my understanding that the version_number is per sub-table. With the EIT sub-table being identified by table_id + service_id. (aka table_id_extension, signaled by section_syntax_indicator being 1)

As the service_id (by looking up the corresponding chanid) is used as part of the key we can leave that out and restrict the value to table_id + version_number.

As the table_id of an event only changes once every 4 days but the segment it is in changes every 24 hours, thus changing the version_number, we can leave it (the table_id) out of the value. (3 hours per segment of 8 sections with 256 sections per table_id => 256/8*3 hours of guide per table_id)

Thus I do think you can safely re-purpose the table_id field of the EIT Cache. The field should be renamed in a database update in master.

As the event_map is per channel it should be safe to leave out the table_id/table_id_extension there, too.

Up to here the changes appear to be safe, but I'm having a hard time identifying the benefit.

What I can identify is the comparison of events against recording rules when they are the following or present event. That should avoid lots of reschedules. I have gone a similar way by ignoring updates to present/following for other transports, but I like your solution better.

If we can find a way to update the guide with present/following information but avoid reschedules when there is no matching rules, that would be perfect.

See [c99b2967] and [b7bab70a] for my attempts to avoid needless reschedules. I'd like to backport them to fixes/0.26 but would prefer if someone can verify they don't break anything.

comment:3 Changed 11 years ago by Roger James <roger@…>

I am currently working on a new version of the patch. I will make sure I use the enum for tableids. I plan to change IsNewEit? to handle the migration of an eventid down through the range of schedule table ids more thoroughly. This means reverting to the original layout of the cache. The code now looks something like this.

    if (it != eventMap->end())
    {
        uint cached_tableid = extract_table_id(*it);
        uint cached_version = extract_version(*it);
        uint cached_endtime = extract_endtime(*it);
        if ((TableID::PF_EIT == tableid) || (TableID::PF_EITo == tableid))
        {
            // tableid is for PF table
            if ((TableID::PF_EIT == cached_tableid) || (TableID::PF_EITo == cached_tableid))
            {
                // Already seen a PF table so check version
                // and update if it has changed
                if ((kVersionInvalid == cached_version) ||
                       (cached_version < version) ||
                       ((kVersionMax == cached_version) && (version < kVersionMax)))
                {
                    verChgCnt++;
                    if (cached_endtime != endtime)
                        bEventMayCauseReschedule = true;
                }
                else
                {
                    // EIT data previously seen
                    hitCnt++;
                    return false;
                }
            }
        }
        else
        {
            // tableid is for SC table
            if ((cached_tableid & 0x0f) == (tableid & 0x0f))
            {
                // Already seen this segment of the table so check version
                // and update if it has changed
                if ((kVersionInvalid == cached_version) ||
                       (cached_version < version) ||
                       ((kVersionMax == cached_version) && (version < kVersionMax)))
                {
                    verChgCnt++;
                    if (cached_endtime != endtime)
                        bEventMayCauseReschedule = true;
                }
                else
                {
                    // EIT data previously seen
                    hitCnt++;
                    return false;
                }
            }
        }
    }
    else
        bEventMayCauseReschedule = true;
 
    eventMap->insert(eventid, construct_sig(tableid, version, endtime, true));
    entryCnt++;

    return true;

I am assuming that the same table version number will be used when the table is broadcast in an "other transport" table. I wonder if anyone can confirm this.

The IsNewEIT function now returns an "Event may cause schedule change flag" in its parameters. This flag will be set to true for new events and events where the endtime has changed from the one stored in the map. Of course this will miss events where the event has been shortened by changing the start time, to catch this we would need to store starttime and duration.

Any comments would be most welcome.

I have looked at the changesets done on head and they look fine.

For selfish reasons I will stick with 0.26-fixes for the time being. After I have tested the new version of eitcache.cpp the next thing will be to look at the MayCauseScheduleChange? flag I am now returning and see if I can extend the check I do for relevant recording rules so that we always update the database but only do reschedules when the flag has been set and a rule is relevant.

Roger

Roger

comment:4 Changed 11 years ago by Roger James <roger@…>

The new version of the patch seems to be working. The code I posted before was not correct. Here is the new logic in IsNewEIT

    if (it != eventMap->end())
    {
        uint cached_tableid = extract_table_id(*it);
        uint cached_version = extract_version(*it);
        uint cached_endtime = extract_endtime(*it);
        if ((TableID::PF_EIT == tableid) || (TableID::PF_EITo == tableid))
        {
            // tableid is for PF table
            if ((TableID::PF_EIT == cached_tableid) || (TableID::PF_EITo == cached_tableid))
            {
                // Already seen a PF table so check version
                // and update if it has changed
                if ((kVersionInvalid == cached_version) ||
                       (cached_version < version) ||
                       ((kVersionMax == cached_version) && (version < kVersionMax)))
                {
                    LOG(VB_EIT, LOG_INFO, LOC + QString("PF table version change - new (%1 %2) old (%3 %4) eventid %5 for chanid %6")
                        .arg(QString().sprintf("%02x", tableid)).arg(QString().sprintf("%02x", version))
                        .arg(QString().sprintf("%02x", cached_tableid)).arg(QString().sprintf("%02x", cached_version))
                        .arg(eventid).arg(chanid));
                    verChgCnt++;
                    bEntryUpdated = true;
                    if (cached_endtime != endtime)
                        bEventMayCauseReschedule = true;
                }
                else
                {
                    // EIT data previously seen
                    hitCnt++;
                    return false;
                }
            }
            else
            {
                // Old entry is in SC table
                LOG(VB_EIT, LOG_INFO, LOC + QString("SC to PF tableid change - new (%1 %2) old (%3 %4) eventid %5 for chanid %6")
                        .arg(QString().sprintf("%02x", tableid)).arg(QString().sprintf("%02x", version))
                        .arg(QString().sprintf("%02x", cached_tableid)).arg(QString().sprintf("%02x", cached_version))
                        .arg(eventid).arg(chanid));
                bEntryUpdated = true;
                if (cached_endtime != endtime)
                    bEventMayCauseReschedule = true;
            }
        }
        else
        {
            // tableid is for SC table
            if ((TableID::PF_EIT == cached_tableid) || (TableID::PF_EITo == cached_tableid))
            {
                // Already seen a PF table
                hitCnt++;
                return false;
            }
            else if ((cached_tableid & 0x0f) == (tableid & 0x0f))
            {
                // Already seen in this segment of the table so check version
                // and update if it has changed
                if ((kVersionInvalid == cached_version) ||
                       (cached_version < version) ||
                       ((kVersionMax == cached_version) && (version < kVersionMax)))
                {
                    LOG(VB_EIT, LOG_INFO, LOC + QString("SC table version change - new (%1 %2) old (%3 %4) eventid %5 for chanid %6")
                        .arg(QString().sprintf("%02x", tableid)).arg(QString().sprintf("%02x", version))
                        .arg(QString().sprintf("%02x", cached_tableid)).arg(QString().sprintf("%02x", cached_version))
                        .arg(eventid).arg(chanid));
                    verChgCnt++;
                    bEntryUpdated = true;
                    if (cached_endtime != endtime)
                        bEventMayCauseReschedule = true;
                }
                else
                {
                    // EIT data previously seen
                    hitCnt++;
                    return false;
                }
            }
            else if ((cached_tableid & 0x0f) > (tableid < 0x0f))
            {
                LOG(VB_EIT, LOG_INFO, LOC + QString("SC tableid change - new (%1 %2) old (%3 %4) eventid %5 for chanid %6")
                        .arg(QString().sprintf("%02x", tableid)).arg(QString().sprintf("%02x", version))
                        .arg(QString().sprintf("%02x", cached_tableid)).arg(QString().sprintf("%02x", cached_version))
                        .arg(eventid).arg(chanid));
                bEntryUpdated = true;
                if (cached_endtime != endtime)
                    bEventMayCauseReschedule = true;
            }
            else
            {
                // Already seen this segment of SC table
                hitCnt++;
                return false;
            }
        }
    }
    else
        bEventMayCauseReschedule = true;

    if (bEntryUpdated)
        LOG(VB_EIT, LOG_INFO, LOC + QString("Updated entry - table (%1 %2) eventid %3 for chanid %4 reschedule %5")
            .arg(QString().sprintf("%02x", tableid)).arg(QString().sprintf("%02x", version))
            .arg(eventid).arg(chanid).arg(bEventMayCauseReschedule));
    else
        LOG(VB_EIT, LOG_INFO, LOC + QString("New entry - table (%1 %2) eventid %3 for chanid %4 reschedule %5")
            .arg(QString().sprintf("%02x", tableid)).arg(QString().sprintf("%02x", version))
            .arg(eventid).arg(chanid).arg(bEventMayCauseReschedule));
    eventMap->insert(eventid, construct_sig(tableid, version, endtime, true));
    entryCnt++;

    return true;
}

I will attach the full patch when it has run for a bit.

The new patch always updates the listings database with new data. It will only request a reschedule when the endtime has changed (checked in EITCache::IsNewEIT) and the change is for a programid and seriesid of a rule in the recording rules table (checked in DBEventEIT::UpdateDB) as below.

    if (eventMayCauseReschedule)
    {
        MSqlQuery localquery(MSqlQuery::InitCon());
        localquery.prepare("SELECT EXISTS(SELECT * FROM record WHERE seriesid = :SERIESID AND programid = :PROGRAMID)");
        localquery.bindValue(":SERIESID", seriesId);
        localquery.bindValue(":PROGRAMID", programId);
        if (!localquery.exec())
           MythDB::DBError("AddEIT", query);
        if (query.next())
            if (!query.value(0).toBool())
               RescheduleRequired = true;
        LOG(VB_EIT, LOG_INFO,
            LOC + QString("Checking reschedule - required %1").arg(RescheduleRequired));
    }


Roger

comment:5 Changed 11 years ago by Stuart Auchterlonie

I don't believe this is totally correct.

One of the design decisions behind the EIT table handling was to always treat the data for the current mux as more correct than the data gathered from another mux.

ie. prefer EITa to EITo

You patch groups EITa and EITo together. (TableID::PF_EIT & TableID::PF_EITo)

I agree that for the long term data tables

    SC_EITbeg to SC_EITend <----- treat as the same
    SC_EITbego to SC_EITendo <--- treat as the same

we should group them together as above

Cheers Stuart

Changed 11 years ago by Roger James <roger@…>

Attachment: eitscannerpatch.v2 added

version 2 of eit scanner patch

comment:6 Changed 11 years ago by Roger James <roger@…>

Hi Stuart

Just to cover my back :-), I did ask this question earlier on in the thread.

"I am assuming that the same table version number will be used when the table is broadcast in an "other transport" table. I wonder if anyone can confirm this."

What follows are my current personal musings on this :-)

I still have not managed to find an official answer to in EN300468 and TS101211. However looking at the logs for the new version of the patch, it would seem that at least for FreeView? here in the UK the assumption is correct i.e. the version numbers for a particular serviceid are in step on all the multiplexes. I agree that this would seem to contradict a literal interpretation of EN300468 section 5.1.1. However if the version ids did not correlate then you would have to maintain a separate copy of the table for each multiplex you encountered it on. That does not make any sense because you have no idea which one is authoritative. If the "actual" mux version is authoritative then what is the point of broadcasting the "other" mux tables in the first place. Maybe it is solely to indicate that if you see an "actual" update then you are tuned to that mux and may be processing the service(id) and need to do something about, in that case maybe there is only one shared copy of the "schedule" and "present/following" table.

I have attached a new version of the patch.

comment:7 Changed 11 years ago by dekarl@…

Roger, there is an open ticket that suggests that it is possible to receive EIT from multiple unrelated sources/operators for the very same channel, see #9480.

My understanding is that you get a separate, unrelated, independent version_number per TableID/ServiceID combination.

IIRC Freeview has its transports generated by one entity and thus are not representative for most DVB networks. If Freeview does simply collect all EITa from each transport and push it out as EITo with only one bit (in the TableID plus the CRC) changed you will have the same version_number on EITa/o. But then it is by accident and not by design.

When I asked about testing my changes I was thinking of testing only these changes against fixes/0.26 before I backport them. They should improve the "EIT scanner causes lots of reschedules" part of this ticket by reducing the scope and count of reschedules. I was hoping to get a simple "works on 0.26 over here" from someone :)

I tried to split the scope of this ticket into breakage "idle shutdown does not work with the EIT scanner" and optimization "there are lots of reschedules that make the CPU do work that might be unneeded".

In my opinion concentrating on the idle shutdown breakage might be more rewarding due to the complexity of the EIT concepts :-)

comment:8 Changed 11 years ago by Roger James <roger@…>

Guys,

Firstly I agree with what dekarl says. It is easy to split my current patch in the two parts. Virtually all the DVB heavy optimization stuff is confined to eitcache.cpp. If you just leave in there the bit that checks changes to endtime and returns a flag, then all the rest is about reducing the number of reschedules, which does fix the breakage!

Any way back to the heavy DVB stuff.

I think I will have to nail my colours to mast and defend my interpretation.

This is going to sound horribly preachy, sorry.

Here is what EN300468 section 5.1.1 says about version numbers

d)
version_number:
When the characteristics of the TS described in the SI given in the present document change
(e.g. new events start, different composition of elementary streams for a given service),
then new SI data shall be sent containing the updated information. A new version of the
SI data is signalled by sending a sub_table with the same identifiers as the previous
sub_table containing the relevant data, but with the next value of version_number.
For the SI tables specified in the present document, the version_number applies to all
sections of a sub_table.

Here is what EN300468 section 5.2.1 says about transport stream ids

The combination of original_network_id and transport_stream_id allow each TS to be uniquely
identified throughout the application area of the present document.

I think some of the confusion is caused by mixing up what is meant by a SI table as an object and as data encoded into a transport stream for transmission. To me there is one notional EIT table object per original network id/transport stream id/service id combination. As one of its properties it has version number. Copies of this object are then encoded into various transmitted transport streams. If it is encoded into the transport stream to which it belongs it is encoded as an "actual" table. If it is encoded into any other transport stream it is encoded as an "other" table.

Lets have a look at the following scenario. You are tuned to a transport stream with an original network id XXXX and a transport stream id YYYY. You a receive an EIT table with a table id of PF_EITa which must according to EN300468 section 5.2.4 contain original network id XXXX and transport stream id YYYY and a service id of ZZZZ. This table has a version of 8 which you store and process. You then tune to a different mux. You then encounter a PF_EITo containing original network id XXXX, transport stream id YYYY, a service id of ZZZZ a version of 9. The service id of course refers to to original mux you were tuned to. If the version numbers "for the same actual snapshot of the data" can be different then you have absolutely no way of knowing whether this is new, old, or identical data to the snapshot you received previously marked version 8. So the only possible thing you can do is discard it. This would negate the whole concept of having "other" tables.

I am off a for pint of beer (or three) in my local pub. My head hurts.

Cheers,

Roger

comment:9 Changed 11 years ago by dekarl@…

Just putting the parts of the spec in that I based my understanding on.

I was referring to EN 300 468 section 5.2.4. "Event Information Table" where they explain the format of the "Event information section".

version_number: This 5-bit field is the version number of the sub_table. The version_number shall be incremented by 1 when a change in the information carried within the sub_table occurs. When it reaches value 31, it wraps around to 0. ... (part about current_next_indicator omitted)

section_syntax_indicator: The section_syntax_indicator is a 1-bit field which shall be set to "1".

from section 3.1 "Definitions" about the sub_table

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

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

and from the same section about table

table: comprised of a number of sub_tables with the same value of table_id

I did not notice the explicit mention of ONID and TSID earlier, but that makes sense as it avoids collisions between tables for the same service_id on different transports in a different original network. (e.g. service_id 1 in the ONID 2 scope colliding with service_id 1 but a different ONID on the same satelite)

comment:10 Changed 11 years ago by Stuart Auchterlonie

I'm going to agree that Roger's interpretation of the specs appears correct. I will however side with Dekarl on the version number field being only a 5 bit field, and thus the part of the patch which touches the bitmask on the version appears incorrect.

Progress \o/ :)

comment:11 Changed 11 years ago by Roger James <roger@…>

I apologise for having left this in limbo.

I added a bit to the version number so I could store an "invalid version" marker in the field to force updates. This mask only applies to the local storage of the value not the the info extracted from the table.

What do I need to do to get this patch accepted? It has been running successfully here for some time now.

Roger.

comment:12 Changed 11 years ago by Roger James <roger@…>

I just realised that the second version of the patch has some carriage returns missing between the diffs for the various files. I have attached a version of the file which I hope is a bit cleaner.

Changed 11 years ago by Roger James <roger@…>

Attachment: eitscannerpatch.v3 added

cleaned up version

comment:13 Changed 11 years ago by Karl Dietz <dekarl@…>

In e73015ba7f6a3ad8c870e46503ff71a1243639e3/mythtv:

Make the idle timeout independent of scheduler runs

As the scheduler runs every other minute when the EIT scanner is
enabled you end up with never being idle, thus breaking shutdown
when idle.

As discussed on IRC at
http://irc.mythtv.org/ircLog/channel/4/2013-03-14:18:52:00

Refs: #11399

comment:14 Changed 11 years ago by Roger James <roger@…>

#dekarl, that will certainly fix the idle shutdown issue!

My old fashioned approach to cpu cycles still makes me think that my patch has merit in correcting the handling of eit serial numbers and stopping unnecessary calls to the scheduler :-).

On of these days I might get round to look at parsing the running status table (RST) and adding handling of last minute schedule changes. Maybe then I can finally fix the occasional clipping of the end of 3 hour dramas!

Roger

comment:15 Changed 11 years ago by Karl Egly

Roger, some random thoughts before you invest additional time. I've been thinking about EIT handling in general and I think that we have to keep more of the EIT tables instead of just a cache.

I was unable to come up with a simple approach to find "vanished" events, which I think may be the source of various silent guide corruption issues. e.g. the "no guide to see, yet, move along" program being painted on top of the programs that are really shown which got signaled later in place of the place holder.

The downside would be that work on the current implementation may become redundant.

comment:16 Changed 11 years ago by roger@…

#dekarl,I think you are entirely correct about the cache. It would be good to have persistent storage of sufficient information to properly track an event through the different tables, sections and versions, even through periods when the back end is not running or not scanning for table data. My patch made some progress in this area but just marked version numbers as invalid on cache reloads, meaning that out of date info could be used.

I have no plans to do any work for at least the next month, as I will not have direct access to my source build system during that period.

Roger

comment:17 Changed 11 years ago by paulh

Milestone: 0.26.10.28

comment:18 Changed 9 years ago by Karl Egly

Resolution: Fixed
Status: newclosed

I'm closing this as fixed as the shutdown issues and lots of unnecessary rescheduling has long been taken care of. I have opened #12329 to keep track of replacing our section / SI handling.

Note: See TracTickets for help on using tickets.