Opened 11 years ago

Closed 11 years ago

#11197 closed Bug Report - General (fixed)

Fix event times in MythZoneMinder broken by the UTC changes

Reported by: mythtv@… Owned by: danielk
Priority: minor Milestone: 0.26.1
Component: Plugin - MythZoneminder Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

Add MythDate::kOverrideUTC to stop the date formating function converting the event times to local time when they are already in local time.

Attachments (2)

0001-mythzoneminder-Fix-the-event-times-broken-by-the-UTC.patch (1.7 KB) - added by paulh <mythtv@…> 11 years ago.
11197-v2.patch (13.7 KB) - added by danielk 11 years ago.

Download all attachments as: .zip

Change History (13)

Changed 11 years ago by paulh <mythtv@…>

comment:1 Changed 11 years ago by dekarl@…

Wouldn't the correct fix be to convert mythzoneminder to explicit time offsets (or all UTC) instead of adding new ways to work with floating time?

Telling MythDate? to convert presumed UTC to UTC when it really way localtime is a hack.

kOverrideUTC   = 0x100000,      // Present date/time in UTC

How about adding the conversion here: http://code.mythtv.org/trac/browser/mythtv/mythplugins/mythzoneminder/mythzoneminder/zmclient.cpp#L338 maybe replacing

item->startTime = MythDate::fromString(sDate);

with a prettified variant of

item->startTime = MythDate::fromString(sDate);
item->startTime.setTimeSpec(Qt::LocalTime);
item->startTime = item->startTime.toUTC();

is all it takes

comment:2 Changed 11 years ago by danielk

Milestone: unknown0.26.1
Owner: set to danielk
Status: newaccepted

comment:3 Changed 11 years ago by danielk

Is the date we're getting here ISODate formatted? Perhaps we could just use this?

-        QString sDate = *it++;
-        item->startTime = MythDate::fromString(sDate);
+        item->startTime = QDateTime::fromString(*it++, Qt::ISODate).toUTC();

comment:4 in reply to:  3 Changed 11 years ago by paulh <mythtv@…>

Replying to danielk:

Is the date we're getting here ISODate formatted? Perhaps we could just use this?

-        QString sDate = *it++;
-        item->startTime = MythDate::fromString(sDate);
+        item->startTime = QDateTime::fromString(*it++, Qt::ISODate).toUTC();

Yes the dates are in ISODate format so that should work.

Seems a bit pointless converting the dates to UTC to only have to change it back later especially when you think we could be dealing with 20,000+ events but so long as the dates displayed match the timestamps on the images I'm not bothered :)

comment:5 Changed 11 years ago by danielk

paulh, the standard is now for all dates to be UTC unless there is a good reason for them not to be. This means we don't need to be checking what timezone a date time is in, if the variable name contains "localTime" it is local time, otherwise it is not. If there is a real performance problem we can of course rename the variable and use local time internally. Is it 20k event per second, per hour, per day?

BTW We're ironically doing a lot fewer utc<->localtime conversions overall now. We used to be doing a few hundred conversions per second in the frontend because QDateTime::currentDateTime() uses it. Now with QDateTime::currentDateTimeUtc() we avoid converting to local time most of the time and it has a measurable positive effect on both CPU usage and responsiveness.

comment:6 Changed 11 years ago by Daniel Thor Kristjansson <danielk@…>

Resolution: fixed
Status: acceptedclosed

In 6122c1bd42b0b8df80a797347daf613eafd8da46/mythtv:

Fixes #11197. mythzoneminder event date is in localtime not utc..

comment:7 Changed 11 years ago by Daniel Kristjansson <danielk@…>

In 9c95f5aabfe779eecb9a3c5f7938d4408e74272c/mythtv:

Revert "Fixes #11197. mythzoneminder event date is in localtime not utc.."

This reverts commit 6122c1bd42b0b8df80a797347daf613eafd8da46.

Reverting based on report from Paul H:

Daniel, this is causing more problems that it solves. If ZM is
set to use it's deep filesytem storage the event date is used
to find the various frames. Because the event start time is
now in UTC MythZM is now failing to find the frames. I could
add more hacks to correct this but it's going to have a big
performance hit because there would have to be a UTC to local
time conversion for each frame.

comment:8 Changed 11 years ago by danielk

Resolution: fixed
Status: closednew
Type: Patch - Bug FixBug Report - General

Changed 11 years ago by danielk

Attachment: 11197-v2.patch added

comment:9 Changed 11 years ago by danielk

Paul, I've attached a patch which encapsulates the Qt:TimeSpec of the event's start time and keeps it internally in local time for efficiency. I also noticed that there is no checking of the length of QStringList's returned from ZMClient::sendReceiveStringList() so it is fairly easy to SEGFAULT the frontend with bad zmserver data. I've added sanity checking for those API calls. I would commit this separately but it would be nice to get it tested at the same time.

comment:10 Changed 11 years ago by paulh <mythtv@…>

Thanks for the patch. I'm not really a good test case for it since I don't use the deep filesystem support in ZM and we are now back to GMT here so no time offsets required. There was one problem I noticed while testing I'm getting this error

ZMClient got a mismatch between the number of cameras and the expected number of stringlist items in getCameraList()

The problem is because of the way the reply is constructed by the server and is then parsed by the client you always get an empty list item tagged on the end so the check in getCameraList() would have to be

if ((int)(strList.size() - 3) != cameraCount)

or since the last item isn't used this would be a better way?

if (strList.size() < cameraCount + 2)

comment:11 Changed 11 years ago by Daniel Kristjansson <danielk@…>

Resolution: fixed
Status: newclosed

In 2e11c6aa842d14d1e79b3c9e403059f6a39d9b66/mythtv:

Fixes #11197. Improve ZoneMinder? Event class encapsulation and use local time internally for efficiency.

Note: See TracTickets for help on using tickets.