Opened 10 years ago

Closed 9 years ago

#9676 closed Patch - Bug Fix (Fixed)

Obtaining program times across daylight savings time change gives inaccurate results

Reported by: mythtv@… Owned by: sphery
Priority: minor Milestone: 0.26
Component: MythTV - Mythfilldatabase Version: 0.24-fixes
Severity: medium Keywords:
Cc: Stuart Auchterlonie Ticket locked: no


This bug affects users having a TimeOffset? set to 'Auto'. I can testify that it affects schedulesdirect data, but it would apply to any listing service that lists using UTC time.

The bug is that in the days leading up to a daylight savings time change, when mythfilldatabase obtains program information for a day after the daylight savings time change will have occurred, the time of that program is calculated incorrectly. By the time the time change actually happens, your program guide has roughly two weeks of incorrect data loaded into it. The workaround is to wait until the time changes, clear out that data, and do a new mythfilldatabase.

The bug is in the MythSecsTo? utility function, because it calculates the time difference independently of the day, and thus can't take into account the hour lost or gained across a daylight savings change.

I will attach a patch that addresses the problem. This simple program demonstrates the problem for CST or other time zones that changed local time on March 13 this year:

#include <iostream>

#include <util.h>

using namespace std;

int main(int argc, char** argv) {
  // this range should only cover a 23-hour period, due to the time change in
  // the middle of it
  QDateTime before = QDateTime(QDate(2011,3,12), QTime(23, 00, 00));
  QDateTime after = QDateTime(QDate(2011,3,13), QTime(23, 00, 00));

  cout << MythSecsTo(before,after) / 60 / 60
       << " vs " << before.secsTo(after) /60 / 60
       << "\n";;

I hope this is not a dup - I searched for tickets, and although some were in the neighborhood none seemed completely like this issue. Apologies if I missed the right ticket.

Attachments (4)

mythsecsto.patch (496 bytes) - added by mythtv@… 10 years ago.
patch that resolves the issue
mythsecsto.2.patch (828 bytes) - added by mythtv@… 10 years ago.
patch that resolves the issue
mythsecsto.3.patch (1.7 KB) - added by mythtv@… 10 years ago.
calc_utc_offset relied on the bug the patch fixes. Update it to match
eithelper.patch (2.8 KB) - added by mythtv@… 10 years ago.
Patch including a fix to EITHelper

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by mythtv@…

Attachment: mythsecsto.patch added

patch that resolves the issue

Changed 10 years ago by mythtv@…

Attachment: mythsecsto.2.patch added

patch that resolves the issue

comment:1 Changed 10 years ago by mythtv@…

Sorry, the first patch applied left out a fix to MythUTCToLocal, where it was using the date of the epoch but in the local time zone.

I considered replacing the entire contents of MythUTCToLocal with QDateTime.toLocalTime(), but I'm not familiar enough with the code to know wether there was some nuance it was trying to work around, or if maybe it was simply written before QDateTime.toLocalTime() came into existence.

While the fixes this patch includes are technically correct, after testing more I'm not 100% confident it resolves the original issue. I will add data once I am sure.

Changed 10 years ago by mythtv@…

Attachment: mythsecsto.3.patch added

calc_utc_offset relied on the bug the patch fixes. Update it to match

comment:2 Changed 10 years ago by mythtv@…

EITHelper gets an offset from UTC in its constructor - if it's long lived (living through a daylight savings time change) that offset will no longer be valid. IIRC, it's not long-lived, though, it is created periodically, so I haven't made any change to it. I mention it in case the myth dev that looks at this ticket knows better than I do.

Sorry to be posting info a little at a time. It seemed like such a simple fix, I thought I had it all ready, and then naturally as soon as I uploaded the patch I realized the rabbit hole went deeper than I thought.

comment:3 Changed 10 years ago by sphery

Component: MythTV - GeneralMythTV - Mythfilldatabase
Owner: set to sphery
Status: newassigned

comment:4 Changed 10 years ago by mythtv@…

After looking further, although these changes fix legit bugs, I'm now thinking the original problem is because EITHelper calculates a utc offset once and uses it after it is no longer valid due to a daylight savings time change. This causes the EIT scanner to go around moving show times to an hour off.

I think EITHelper.CompleteEvent? should not use utc_offset at all, but rather should call startTime.setTimeSpec(Qt::UTC), as in

        starttime.setDate(QDate(result.tm_year + 1900,
                                result.tm_mon + 1,
        starttime.setTime(QTime(result.tm_hour, result.tm_min, result.tm_sec));

However this is deeper in the code than I know how to test properly, and I'm also out of time to look at it right now, so I can't swear that I'm right on this.

Changed 10 years ago by mythtv@…

Attachment: eithelper.patch added

Patch including a fix to EITHelper

comment:5 Changed 10 years ago by mythtv@…

I believe the final patch I attached solves the problem. I did some unit level testing, but it's hard to say 100% just because I can't force the guide data to span a daylight savings time change. I could only rig up some unit tests.

To summarize

  • there were some calculations in the code that may have been written to an older, less complete QtDateTime? API. They didn't handle daylight savings correctly.
  • EITHelper is written to calculate an offset from UTC in its constructor. It used that in its calculations rather than using QtDateTime?'s features. When EITHelper lives across a daylight savings change (which changes the offset from UTC) it starts calculating start times incorrectly, so it updates the schedule to be an hour off over time periods covered by the EIT.

I think this patch solves the problems I've seen discussed in several places, like this thread:

For myself, I happened to leave on vacation right before the time changed. Myth recorded shows off by an hour for more than a week. I cleared out the guide data and redownloaded it (without restarting myth) and it continued to be off by an hour. Restarting myth AND redownloading the data worked around the issue. This has happened to me on multiple previous time changes, and I know from those times that restarting myth only doesn't fully address the issue. In the thread above, the user's data being off for the next two days only would be consistent with about how long EIT data might exist for.

Thanks, mdean, for looking at this patch. The time change thing has gotten me twice a year for years, so I finally decided the best thing to do was try and fix it myself. I hope I got it.

comment:6 Changed 10 years ago by Stuart Auchterlonie

Cc: Stuart Auchterlonie added

comment:7 Changed 10 years ago by Peter D. <0123peter@…>

My apologies if this is a stupid question, but what happens if the data is coming from the TV stations and they dis-agree with each other? Say one is in local time and the other is in UTC.

comment:8 Changed 10 years ago by mythtv@…

A correctly specified time includes the time zone as the last thing in the string (either abbreviated like CST or UTC, or as an offset from UTC like -06), so when it's read in it's simply converted to the system time zone; all time values in memory are kept in system time. If no time zone is specified the local time zone is assumed. If a station doesn't specify the time zone and it's not using local time, it's just plain buggy. Qt's time classes have built-in support to handle all of the conversion.

comment:9 Changed 10 years ago by Peter D. <0123peter@…>

You made a fast response there.

I am in complete ignorance of the code, but I was making a plea for robust behavior when the input data is definitely wrong.

comment:10 Changed 10 years ago by mythtv@…

I can only say the submitted patch make myth neither more nor less robust in that regard.

comment:11 Changed 9 years ago by sphery

Milestone: unknown0.26
Resolution: Fixed
Status: assignedclosed

Fixed by 4f028f388c38c

Note: See TracTickets for help on using tickets.