Alpha patch for fix for Bug #590: IceTV timer description issues

[prl]

Here's a combined alpha patch of the fix for Bug #590: IceTV timer description issues. and Bug #520: Start / End bookmarks missing on recordings

The patch has been tested with firmware version 2017-04-22 (beta), but it should also work with the official 2017-03-10 release.

This patch is for the T4 firmware. Using it on another model may not work correctly (in ways unrelated to the intended fix).

If you installed the earlier version of this patch, you must first uninstall the patch with that version's uninstaller.

To apply the patches, download the linked .ZIP file, and extract it. It will create a new directory/folder called icetvdesc-installer, which contains two files, installer.sh and uninstaller.sh.

Copy the two files somewhere convenient on a T series box (like /home/root), then log into the box using telnet or ssh, change directory to the place you put the installer.sh/uninstaller.sh files. If you put the files in /home/root you'll be in the right place as soon as you log in.

To install the patches run:

sh installer.sh

and restart the GUI (or reboot).

To uninstall the patches, log in and go to the directory as you did to install, and run

sh uninstaller.sh

Make sure you uninstall before doing an upgrade. Don't run the installer if you've already installed & don't run the uninstaller if you've already uninstalled.

You can check whether the patches are installed by logging in and running this on the box:

find /usr/lib/enigma2 /usr/bin /usr/share/enigma2 -name \*.bak

It should print nothing if the patch isn't installed, and it should print

/usr/lib/enigma2/python/Screens/InfoBarGenerics.pyo.bak
/usr/lib/enigma2/python/enigma.pyo.bak
/usr/lib/enigma2/python/Plugins/SystemPlugins/IceTV/API.pyo.bak
/usr/lib/enigma2/python/Plugins/SystemPlugins/IceTV/plugin.pyo.bak
/usr/bin/enigma2.bak

if the patch is installed.

Comments welcome!

icetvdesc2-installer.zip

[prl]

The patch will log updates to timer descriptions and the time that the update uses to the IceTV log and to the enigma2 debug log.

The IceTV log entries look like this (the update was forced by modifying timers.xml to contain an incorrect description):

Similar log entries are placed in the debug log:

Code: Select all

root@beyonwizt4:/media/hdd/logs# grep EPGFetcher Enigma2-2017-05-24_10-07-57.log
{1454}<   891.520> [EPGFetcher] updateDescriptions from EPG short description 1:0:19:215:211:1010:EEEE0000:0:0:0: 'Delicious' 'Series 1: Episode 1x' -> 'Series 1: Episode 1'
{1454}<   891.533> [EPGFetcher] updateDescriptions time 16.4ms
root@beyonwizt4:/media/hdd/logs#

Please let me know if you see the update taking more than 100ms more than occasionally, and if it ever takes more than 1000ms.

I intend to remove the debug logging entries and shorten the IceTV log entries by removing the text "from EPG short/extended description" from the IceTV log entry in the production version.

As I mentioned here, this patch is compatible with the patch that fixes Bug #520: Start / End bookmarks missing on recording, but as mentioned in that post, care needs to be taken with the installer/uninstaller files.

[Grumpy_Geoff]

Installed fine -

Code: Select all

root@beyonwizt4:~/icetvdesc#
root@beyonwizt4:~/icetvdesc# sh installer
Installing icetvdesc
... in /usr/lib/enigma2/python
Plugins/SystemPlugins/IceTV/plugin.py
root@beyonwizt4:~/icetvdesc#

I hacked an IceTV timer description, but as it turns out there were other updates anyway -

Code: Select all

{1287}<  1109.237> [EPGFetcher] updateDescriptions from EPG short description 1:0:19:684:607:1014:EEEE0000:0:0:0: 'MasterChef Australia' '2017 - TBA 25/05/17' -> '2017 - Elimination Blind Taste Test'
{1287}<  1109.243> [EPGFetcher] updateDescriptions from EPG short description 1:0:19:325:320:3202:EEEE0000:0:0:0: 'Wild Thailand' 'ZZZZ[Rpt] A Forgotten And Rarely Seen Wilderness' -> '[Rpt] A Forgotten And Rarely Seen Wilderness'
{1287}<  1109.250> [EPGFetcher] updateDescriptions from EPG extended description 1:0:19:2E5:261:3201:EEEE0000:0:0:0: 'Media Watch' 'Australia's leading forum for media analysis and comment returns for 2017, presented by one of Australia's most respected journalists Paul Barry. #MediaWatch (Return)  ' -> 'Australia's leading forum for media analysis and comment returns for 2017, presented by one of Australia's most respected journalists Paul Barry. #MediaWatch (Return)	 '
{1287}<  1109.644> [EPGFetcher] updateDescriptions time 424.2ms

('Media Watch' had a tab inserted!)

I'll monitor, and keep the T4 in standby when not in use.


Cheers
Geoff

[prl]

Thanks. I'm not liking the look of the 400ms+ on that update, though. I've seen occasional updates take 300ms+. It may just be hitting a garbage collection while it's running, so not an issue unless it's happening too often.

I may have added this to my post after you looked at it: "Please let me know if you see the update taking more than 100ms more than occasionally, and if it ever takes more than 1000ms."

[Grumpy_Geoff]

Yeah, you did but I saw it before posting. That 424.2ms description update time was from a restart, so it would've been procesing a full EPG download.

[prl]

Yeah, you did but I saw it before posting. That 424.2ms description update time was from a restart, so it would've been processing a full EPG download.

That's typically only about 20-30ms on our in-use T4 with ~37 IceTV timers. The description update time for subsequent IceTV updates is faster, typically < 10ms.

The update time is proportional to the number of events in an EPG update that are in services where there are any IceTV recordings. Even if you record across many more different services than we do, it still shouldn't blow out to 400ms+ unless there are other things happening at the same, like a Python garbage collection. The speed is also limited by the preprocessing time of the timer list, which may dominate when there are only a small number of events in the EPG update.

[Grumpy_Geoff]

81 waiting timers, of which 71 were from IceTV, and those timers were for 15 different services.

[prl]

400ms still seems too much. Lets just wait and see whether it's typical for startup on your system.

[Grumpy_Geoff]

400ms still seems too much. Lets just wait and see whether it's typical for startup on your system.

Shouldn't I resist doing reboots/restarts to fully test this patch though?

[prl]

400ms still seems too much. Lets just wait and see whether it's typical for startup on your system.

Shouldn't I resist doing reboots/restarts to fully test this patch though?

That's right, but I'm happy to wait until function has been checked out to have a look at performance issues.

[Grumpy_Geoff]

I've got a timer from IceTV with an empty 'description' field that shouldn't be so.

timers.xml snippet -
<timer begin="1496236800" end="1496242140" serviceref="1:0:19:325:320:3202:EEEE0000:0:0:0:" repeated="0" rename_repeat="1" name="Fargo" description="" afterevent="auto" tags="" disabled="0" justplay="0" always_zap="0" descramble="1" record_ecm="0" isAutoTimer="0" ice_timer_id="16296280767804293935"></timer>
The record time of 1496236800 ==> Wednesday, May 31, 2017 9:20:00 PM GMT+08:00

The timer came in at Thu 25/5 21:31:09 -
<129772.778> [RecordTimer] Record RecordTimerEntry(name=Fargo, begin=Wed May 31 21:20:00 2017, end=Wed May 31 22:49:00 2017, serviceref=1:0:19:325:320:3202:EEEE0000:0:0:0:, justplay=False, isAutoTimer=False, ice_timer_id=16296280767804293935)

The IceTV website shows the episode title is "The Lord Of No Mercy" This is reflected in the EPG on the T4.
The EPG entry came in at time 1495635368 => Wednesday, May 24, 2017 10:16:08 PM GMT+08:00. That time is 23 hours before the timer was created.

IceTV EPG snippet sent to T4 (I've reformatted it) -
,{"id":"132311939","series_id":"38277","episode_id":"251785","channel_id":"97","date":"2017","season":"3","episode-num":"6"
,"start":"2017-05-31T13:30:00+00:00","stop":"2017-05-31T14:25:00+00:00"
,"title":"Fargo","subtitle":"The Lord Of No Mercy"
,"desc":"Gloria and Winnie get closer to the truth; Emmit tries to make things right; Nikki and Ray prepare for payback; Varga cleans up a mess."
,"category":[{"name":"Drama","eit":"0x10"},{"name":"Crime","eit":"0x11"},{"name":"Comedy","eit":"0x14"}]
,"language":"English","country":"United States"
,"video":{"aspect":"16:9","colour":"YES","quality":"SDTV"}
,"subtitles":{"onscreen":"English"}
,"part_of_series":"Yes","rating":"M"}

IceTV timer (also reformatted) -
,"timers":
[{"id":"16296280767804293935","device_id":"176230","channel_id":"97"
,"name":"Fargo","show_id":"132311939"
,"start_time":"2017-05-31T13:30:00+00:00","duration_minutes":"55"
,"action":"record","state":"waiting"
,"message":"Queued for delivery to device"
,"series_recording_id":"10973931","keyword_id":"0"
}]


What I also noticed is that selecting the EPG program entry that matches this timer doesn't offer GREEN as 'Change timer' as instead it is 'Add timer'
I'm thinking this is because the EIT doesn't match (the timer doesn't have an 'eit={value}' entry at all).

The timer for the earlier Fargo episode showing at 20:30 isn't affected -
<timer begin="1496233200" end="1496238840" serviceref="1:0:19:325:320:3202:EEEE0000:0:0:0:" repeated="0" rename_repeat="1" name="Fargo" description="The House Of Special Purpose" afterevent="auto" eit="12926" tags="" disabled="0" justplay="0" always_zap="0" descramble="1" record_ecm="0" isAutoTimer="0" ice_timer_id="16296280767804293934"></timer>

[prl]

That's odd. When the RecordTimerEntry is created, it does an EPG lookup on the time midpoint of the padded timer. I get that to be at Wed May 31 14:04:30 2017 UTC.

The EPG entry appears to have been 2017-05-31T13:30:00+00:00 .. 2017-05-31T14:25:00+00:00, so the lookup should have found the EPG entry correctly.

The fact that the timer has no eit or description entry strongly suggests that the EPG search failed when the RecordTimerEntry was created. I'm not sure why it would.

BTW: the timer end time suggests that you have post-padding of 24 min (timer finishes at 14:49:00 UTC). Is that correct? It just seems an odd value.

I have correct timer descriptions for both Fargo "The House Of Special Purpose" and "The Lord Of No Mercy" on our in-use T4.

[Grumpy_Geoff]

...
BTW: the timer end time suggests that you have post-padding of 24 min (timer finishes at 14:49:00 UTC). Is that correct? It just seems an odd value.

Yep, 24 mins. It was 25, but I changed a while back to see if it better assisted mid-point program detection for the shorter kids programming.

[prl]

The missing eit when there is no match in the EPG when the timer is created will be an issue. For some reason I thought when I used that test that the eit would be taken from the IceTV timer message. It isn't.

The EPG lookup in the timer updates from the EPG should be on time midpoint, and they should provide an eit as well as the description.

[prl]

The current patch doesn't work as intended. It couldn't cause any additional problems, but it isn't able to fix timers that don't initially get a description.

I'm currently testing a re-written version that should address the problems with the posted version.

[Grumpy_Geoff]

I've two more timers sans description and eit values. Do you need more details, or have you a handle on that already?

[prl]

The patch won't fix the missing descriptions, but it would be useful to know how they got that way.

[Grumpy_Geoff]

See attached. It appears to be similar to previous example - EPG entries came in before timer entries come in from Ice.

[Grumpy_Geoff]

I just noticed that vader1111's "Blindspot" IceTV timer that was posted in the "IceTV Timers still sometimes without titles" thread doesn't have an EIT attribute, so this could either be a similar failed EPG lookup or an absent EPG entry at timer creation time.

<timer begin="1495026900" end="1495032240" serviceref="1:0:1:946:99E:3281:EEEE0000:0:0:0:" repeated="0" rename_repeat="1" name="Blindspot" description="" afterevent="auto" tags="" disabled="0" justplay="0" always_zap="0" descramble="1" record_ecm="0" isAutoTimer="0" ice_timer_id="16292897759185422943">

[prl]

I just noticed that vader1111's "Blindspot" IceTV timer that was posted in the "IceTV Timers still sometimes without titles" thread doesn't have an EIT attribute, so this could either be a similar failed EPG lookup or an absent EPG entry at timer creation time.
...

When a timer is created, the same EPG lookup is used to add the eit and description to the timer if they're not present in the call to construct the object (which for an IceTV timer, they are not).

So I'd expect an IceTV timer to have either no eit nor description, or for it to have both.

[prl]

I've had a look at your posted logs and at the eEPGCache source (lib/dvb/epgcache.cpp) I can't see any reason why the EPG lookup wouldn't work in those cases.

[Grumpy_Geoff]

I've had a look at your posted logs and at the eEPGCache source (lib/dvb/epgcache.cpp) I can't see any reason why the EPG lookup wouldn't work in those cases.

Perhaps something data-related with those specific events?

[prl]

I've had a look at your posted logs and at the eEPGCache source (lib/dvb/epgcache.cpp) I can't see any reason why the EPG lookup wouldn't work in those cases.

Perhaps something data-related with those specific events?

The search point time is within the time span of the Blue Bloods "Drawing Dead" episode. The only thing I can think of is that perhaps there's a problem with the search if the episode being looked up is the last event in the channel at the time of lookup. I'll try experimenting with that.

Once the lookup is done, there's really nothing data-dependent going on apart from the data content of the fields being copied into the timer:

Code: Select all

class RecordTimerEntry(timer.TimerEntry, object):
	def __init__(self, serviceref, begin, end, name, description, eit, disabled=False, justplay=False, afterEvent=AFTEREVENT.AUTO, checkOldTimers=False, dirname=None, tags=None, descramble='notset', record_ecm='notset', isAutoTimer=False, ice_timer_id=None, always_zap=False, rename_repeat=True):
		...
		if not description or not name or not eit:
			evt = self.getEventFromEPG()
			if evt:
				if not description:
					description = evt.getShortDescription()
				if not description:
					description = evt.getExtendedDescription()
				if not name:
					name = evt.getEventName()
				if not eit:
					eit = evt.getEventId()
		self.eit = eit
		self.name = name
		self.description = description
...
	def getEventFromEPG(self):
		epgcache = eEPGCache.getInstance()
		queryTime = self.begin + (self.end - self.begin) / 2
		ref = self.service_ref and self.service_ref.ref
		return epgcache.lookupEventTime(ref, queryTime)

The RecordEventEntry is created in the IceTV plugin as:

Code: Select all

RecordTimerEntry(serviceref, start - config.recording.margin_before.value * 60, start + duration + config.recording.margin_after.value * 60, name, "", None, ice_timer_id=ice_timer_id)

i.e. with name set from the timer request from IceTV, description =="" and eit == None.

[prl]

I just set a series recording on the last event in the VICELAND HD EPG (currently Cycling: Criterium Du Dauphine ‘2017 - Stage 2’), and it found the description and eit without any problem.

[IanB]

You are probably going to need a synchronous copy of the epg.dat as at the time the description-less timer is added.

My beer money is on the epg data not being as required at that exact moment but pretty soon after it is.

[Grumpy_Geoff]

...
The search point time is within the time span of the Blue Bloods "Drawing Dead" episode. The only thing I can think of is that perhaps there's a problem with the search if the episode being looked up is the last event in the channel at the time of lookup. I'll try experimenting with that.

You may still be onto something, as the 'last timer' had been something I'd been thinking about for a while. I don't know why my 'last EPG event for service' timer failures differ from your test though.

When timer for Blue Bloods "Lost and Found" was added, that event was not the last in the EPG for TEN HD (Ice channel_id: 2624) as "Drawing Dead" was.
When timer for Blue Bloods "Drawing Dead" was added, that event _was_ the last in the EPG for TEN HD.

When timer for Bear Grylls "Series 4, Episode 2" was added, that event was the last in the EPG for SBS HD (Ice channel_id: 97)

I've got another timer without EIT and Description values, I'll check on it when I get time, and that of the one I previously reported.

[peteru]

You are probably going to need a synchronous copy of the epg.dat as at the time the description-less timer is added.

My beer money is on the epg data not being as required at that exact moment but pretty soon after it is.

Which could only be as a result of IceTV sending out inconsistent data. The plugin itself populates the EPG before processing the timers and all of this work is done sequentially in a single thread for each IceTV update received. Full IceTV logging should provide enough information about the sequencing.

[Grumpy_Geoff]

..
I've got another timer without EIT and Description values, I'll check on it when I get time, and that of the one I previously reported.

Same, same.

As far as I can tell, when the timer for Fargo episode "The Lord Of No Mercy" came in, that event was the last in the EPG for SBS HD (Ice channel_id: 97).
That wasn't the case for the timer for Fargo episode "The House Of Special Purpose", as its event was the penultimate SBS HD EPG event (episode "The Lord Of No Mercy" followed it).

Again, as far as I can tell from the logs, the Blue Bloods timer for episode "Lost and Found" was the penultimate EPG event for TEN HD. This timer was created with a description and an EIT.
The timer for Blue Bloods episode "Drawing Dead" was the last EPG event for TEN HD, and this timer didn't get a description nor EIT value.

There was one more timer that came in that also has no description nor EIT values, and again as far as I can see from the logs, it was the last event for that service.

[prl]

Hm. I'll try doing some more recordings on the last entry of a channel's EPG again.

If the reason for this can't be found, one of the optimisations in my current version of the fix can't be used

We're also recording Fargo, and we're not seeing any missing descriptions on it, but perhaps the timing of something is different between Canberra and Perth. In fact, I haven't seen any recordings with missing descriptions or EPG. I have seen my current fix update to follow a few IceTV EPG changes, though.

[prl]

You are probably going to need a synchronous copy of the epg.dat as at the time the description-less timer is added.

My beer money is on the epg data not being as required at that exact moment but pretty soon after it is.

Which could only be as a result of IceTV sending out inconsistent data. The plugin itself populates the EPG before processing the timers and all of this work is done sequentially in a single thread for each IceTV update received. Full IceTV logging should provide enough information about the sequencing.

The IceTV plugin EPGFetcher.doWork() does do that, but buried down in the C++ code that gets called from there to update the EPG from IceTV is:

Code: Select all

void eEPGCache::importEvents(ePyObject serviceReferences, ePyObject list)
{
	...
	for (int i = 0; i < numberOfEvents;  ++i)
	{
		...
		Py_BEGIN_ALLOW_THREADS;
		submitEventData(refs, start, duration, title, short_summary, long_description, event_type, eventId);
		Py_END_ALLOW_THREADS;
	}
}

I can't think how that would affect the processing in EPGFetcher.doWork(), but it does mean that the Python code (in general) isn't running completely synchronously with the EPG update, though I think that eEPGCache::importEvents() should still be synchronous within EPGFetcher.doWork(). EPGFetcher.doWork() itself is called directly from twisted.internet.reactor.callInThread() which bypasses the normal enigma2 mainLoop scheduling.

[prl]

OK, I think I've been able to replicate Grumpy_Geoff's problem, and the problem I've found looks like it's an issue with eEPGCache::lookupEventTime() not finding the last event in a service's EPG.

It's slightly complicated to poke this bug this, but at least at this time of the day, the last "Temporary Close" on ABC ME is both the last entry in its EPG and can also be scheduled as an IceTV recording.

If I schedule the last "Temporary Close" as a recording, its timer gets neither an eit not a description. If I add a check on the call of the return value of eEPGCache::lookupEventTime(), then for that timer, the return value is None, even though the lookup time is within the range of the EPG entry.

If I set an IceTV recording for any earlier "Temporary Close", it gets assigned the IceTV description ("Station Close.") as its timer description; it has no IceTV episode information.

This doesn't seem to be due to any asynchronous behaviour, since if I manually update the EPG and timers well after the EPG/timer update that sets the timer, the call of eEPGCache::lookupEventTime() from the timer eit/description update code still returns None.

I'm not certain that this bug is what's causing Grumpy_Geoff's problem, but it probably needs to be fixed anyway.

[Grumpy_Geoff]

Last night (mid-evening), I did the same cycling timer test as prl did. I checked and observed that the last event listed for SBS VICELAND HD on both the IceTV web site and via OpenWebif on the T4 for next Mon 5-Jun was "Cycling: Criterium Du Dauphine" at 23:00. I set a single recording for that via the IceTV web page. I didn't actually check the EPG via the T4 GUI, only via OWIF.

The timer was fetched at 23:01:10, and was created with both description and EIT values. Hmm...
I decided to take a look at the debug log and have a look at the EPG updates that came in before the timer was created to see if any contained events ahead in time of "Criterium Du Dauphine".
I'm guessing you probably know what I found

At log elapsed time <567172.080> the timer from IceTV for "Cycling: Criterium Du Dauphine" was fetched -
"channel_id":"2711","name":"Cycling: Criterium Du Dauphine","show_id":"132435750","start_time":"2017-06-05T15:00:00+00:00" (blah}

Prior to that, at elapsed time <564473.402> there had been an EPG update containg events ahead of 5-Jun 23:00 - thus Criterium Du Dauphine was no longer the last EPG event on the service.
There were events up to the next evening, the last being the first of the two Fargo repeats -
"start":"2017-06-06T13:20:00+00:00","stop":"2017-06-06T14:20:00+00:00","title":"Fargo","subtitle":"[Rpt] The House Of Special Purpose" (blah}
That EPG update contained 583 events, thus it appears to have been the 'daily' update.
So, by the time IceTV released the "Criterium Du Dauphine" timer (i.e. <= 144 hours ahead), the EPG had been populated with events for later times on that service

I think this means I might not be able to test by triggering single timers from IceTV. I'll try on another service though.

[Grumpy_Geoff]

...
It's slightly complicated to poke this bug this, but at least at this time of the day, the last "Temporary Close" on ABC ME is both the last entry in its EPG and can also be scheduled as an IceTV recording.

Okay, I scheduled the Mon 5-Jun 23:00 ABC ME "Temporary Close" as you did, neither description nor EIT value in resultant timer -
<timer begin="1496674200" end="1496697840" serviceref="1:0:1:2E4:261:3201:EEEE0000:0:0:0:" repeated="0" rename_repeat="1" name="Temporary Close" description="" afterevent="auto" tags="" disabled="0" justplay="0" always_zap="0" descramble="1" record_ecm="0" isAutoTimer="0" ice_timer_id="1496198430"></timer>

[prl]

Glad to get confirmation, but is it your bug?

[Grumpy_Geoff]

Well, the scenario matches what I've been able to determine from the debug log for 6 examples - any timer request from IceTV set against the last event in the EPG for that service doesn't get description nor EIT fields in the resultant timer.
Plus the ABC ME "Temporary Close" timer, where thanks to your observations, it was easy to find an event <= 144 hours ahead that was also the last for its service.
Hanging judge, anyone?

[prl]

I've had a closer look at the code for eEPGCache::lookupEventTime() and at the documentation for std::map (most of my active use of C++ predates stdlib!) and I'm now pretty sure that eEPGCache::lookupEventTime() will only return the last event in a service's EPG if it's probed with that event's start time. Any probe times greater than its start time and less than its end time will fail. The probe time in an EPG lookup from either RecordTimerEntry or from the IceTV plugin will almost never probe the event start time.

I'm also having a bit of a problem with working out just how eEPGCache::lookupEventTime() is supposed to work. I think that direction = 0 is supposed to find the event such that event.begin <= queryTime < event.end, direction < 0 is supposed to return the previous event, and direction > 0 is supposed to return the next event.

eEPGCache::lookupEventTime() seems to have a number of other problems that are unrelated to this particular issue.

I've attached a commented-up version of eEPGCache::lookupEventTime() to see whether people agree with my assessment of its problems, or if I've missed any). It's an attachment, because Sucuri, the forum's own paranoid android won't allow the sequence "; <whitespace> if ( something", which makes posting anything but the most trivial code sort of difficult.

[prl]

Also, in eEPGCache::lookupEventTime(), if direction < 0, I'm pretty sure it will return the previous event if event.begin == queryTime, and the event straddling the query time if event.begin < queryTime < event.end. That's one of the reasons why I'm not certain what the direction parameter is supposed to do.

[IanSav]

Hi Prl,

Also, in eEPGCache::lookupEventTime(), if direction < 0, I'm pretty sure it will return the previous event if event.begin == queryTime, and the event straddling the query time if event.begin < queryTime < event.end. That's one of the reasons why I'm not certain what the direction parameter is supposed to do.

What did the documentation and/or program comments say the code should do?



(For the benefit of others who have not looked at the source code, Enigma2 is *very* poorly documented. Even when you do find a comment it often doesn't match the code it documents.)

Regards,
Ian.

[prl]

In this case, it's not helped by the fact that what the code does doesn't seem to make much sense, either.

[IanB]

It appear the map is only of event start times. So yes if t is after the last events start time then you will get the end() sentinal iterator. In which case the code should then be checking the ->second->getDuration() of the last element.

Yuck!

To help refresh peoples std:map usage :-

Return iterator to lower bound
Returns an iterator pointing to the first element in the container whose key is not considered to go before k (i.e., either it is equivalent or goes after).

Return iterator to upper bound
Returns an iterator pointing to the first element in the container whose key is considered to go after k.

Iterator to end
Returns an iterator pointing to the past-the-end element in the sequence:

[prl]

Due to the number of fascinating bugs in eEPGCache::lookupEventTime(), I've just finished completely recoding it (haven't compiled yet).

It should now be both correct and easier to follow. I will add comments about exactly what the direction parameter does.

Some more exercises for the interested reader:

if direction < 0 and t < first_event.begin, what happens?

if direction < 0 and some_event.begin == t, what event gets returned?
if direction < 0 and some_event.begin < t < some_event.end, what event gets returned?

I don't think direction < 0 is used anywhere in the code, and it shows

[prl]

...
Some more exercises for the interested reader:

if direction < 0 and t < first_event.begin, what happens?
...

Curiously in that case, the code doesn't crash, as I thought it would. It returns None which I think is correct. I expected it to crash.

It seems that the behaviour of bidirectional_iterator in some corner cases depends on which version of STL is used.

In the Beyonwiz 16.1 build environment and in Ubuntu 16.04, if you do:

Code: Select all

	std::map<int, std::string> someMap;
	someMap[5] = "2";
	std::map<int, std::string>::iterator x = someMap.begin();
	--x;

then x == someMap.end().

But if you do the same in OS X XCode 7.3, the results are undefined. After the decrement, x != someMap.begin() and x != someMap.end(), and accessing through x gives unpredictable results.

[prl]

This is the comment I've added to to describe what the direction parameter should do.

Code: Select all

// Assuming that all event start times are distinct and events are non-overlapping:
// if direction > 0, return event e with least e.getBeginTime() such that e.getBeginTime() > t else None
// if direction == 0, return event e, where e.getBeginTime() <= t < e.getBeginTime() + e.getDuration() else None
// if direction < 0
//    if event e exists, where e.getBeginTime() <= t < e.getBeginTime() + e.getDuration(),
//        return event e1 with greatest e1.getBeginTime() such that e1.getBeginTime() < e.getBeginTime()
//     else
//        return event e1 with greatest e1.getBeginTime() such that e1.getBeginTime() < t else None

Less formally:
If direction > 0 return the earliest starting event after t.
If direction == 0 return the event that spans t. If t is spanned by a gap in the EPG, return None.
if direction < 0 return the event immediately before the event that spans t. If t is spanned by a gap in the EPG, return the event immediately before the gap.

In the new code that I've written it should be easier to determine that the code does what's on the label.

[prl]

Here's the new version of the eEPGCache::lookupEventTime() code. Comments most welcome.

I've been able to locate the problem* that prevented me posting the original code. You'll find comments in the code to indicate where.

Code: Select all

RESULT eEPGCache::lookupEventTime(const eServiceReference &service, time_t t, const eventData *&result, int direction)
// if t == -1 we search the current event...
// Assuming that all event start times are distinct and events are non-overlapping:
// if direction > 0, return event e with least e.getBeginTime() such that e.getBeginTime() > t else None
// if direction == 0, return event e, where e.getBeginTime() <= t < e.getBeginTime() + e.getDuration() else None
// if direction < 0
//    if event e exists, where e.getBeginTime() <= t < e.getBeginTime() + e.getDuration(),
//        return event e1 with greatest e1.getBeginTime() such that e1.getBeginTime() < e.getBeginTime()
//     else
//        return event e1 with greatest e1.getBeginTime() such that e1.getBeginTime() < t else none
{
	uniqueEPGKey key(handleGroup(service));

	// check if EPG for this service is ready...
	eventCache::iterator It = eventDB.find( key ); // Meaningless comment to placate Sucuri's paranoia
	if ( It != eventDB.end() && !It->second.byEvent.empty() ) // entries cached ?
	{
		if ( t == -1 )
			t = ::time(0);
		timeMap::iterator i = It->second.byTime.upper_bound(t); // find > or equal
		if ( direction > 0 )
		{
			if ( i != It->second.byTime.end() ) {
				result = i->second;
				return 0;
			} else
				return -1;
		}
		if ( i == It->second.byTime.begin() )
			return -1;
		--i;
		time_t start_time = i->first; // Another meaningless comment to placate Sucuri's paranoia
		if ( direction == 0 ) {
			// start_time <= t from map and iterator properties
			if ( t < start_time + i->second->getDuration() ) {
				result = i->second;
				return 0;
			} else
				return -1;
		}
		// direction <= 0
		if ( t >= start_time + i->second->getDuration() ) {
			result = i->second;
			return 0;
		}
		if ( i != It->second.byTime.begin() ) {
			--i;
			result = i->second;
			return 0;
		}
	}
	return -1;
}

* Sucuri, the paranoid android, doesn't like text that contains "stuff ; <whitespace> if ( other stuff".

[IanSav]

Hi Prl,

Can you please post the code block as a text attachment so that those of us who can't see code boxes properly can read your code as intended.

Regards,
Ian.

[peteru]

Comments most welcome.

My comments are mainly about the comments.

Could you please format your comments using the Doxygen style? This is very useful for generating API documentation and it can often be used by other tools, including intelligent IDEs. Have a look at eEPGCache::importEvents for one example of the style. There are a few more instances.

Also, your "less formal" comments are actually a better candidate for the documentation. They communicate the intent clearly and succinctly and are easier to comprehend when scanning the documentation quickly, so please include those in the API documentation.

Otherwise, well done on venturing into the eEPGCache code. There's plenty of scope for improvement! The Beyonwiz version has a number of differences from upstream, because I had to fix some fundamental issue in that code in the past. We also have some enhancements to support better EPG ingestion, which was needed to support IceTV.

[prl]

I was also wondering about the fact that updating M new elements into a service EPG cache with N existing elements was O(MN). I don't think it needs to be. I think it can be O(log(N)M).

[prl]

...
Could you please format your comments using the Doxygen style? This is very useful for generating API documentation and it can often be used by other tools, including intelligent IDEs. Have a look at eEPGCache::importEvents for one example of the style. There are a few more instances.

Also, your "less formal" comments are actually a better candidate for the documentation. They communicate the intent clearly and succinctly and are easier to comprehend when scanning the documentation quickly, so please include those in the API documentation.
...

OK, but I don't have Doxygen installed. I'll post a revised version with the reformatted (and less formal) comments. I'd appreciate it if you could check that I haven't messed up the comment syntax for Doxygen.

I've also made a small change to the code to go back to using map::lower_bound() for direction <= 0, which saves an iterator decrement when t is equal to an event start.

[prl]

...
I've also made a small change to the code to go back to using map::lower_bound() for direction <= 0, which saves an iterator decrement when t is equal to an event start.

I changed it back. The potential small performance benefit when t was an event start time (eliminating a single iterator++; iterator-- pair of operations) was overridden by the loss of code clarity and the extra tests needed to handle all the cases properly. There may have been a tiny remaining performance benefit, but I didn't think it was worth the extra complication.