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: | 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)
Change History (13)
Changed 11 years ago by
Attachment: | 0001-mythzoneminder-Fix-the-event-times-broken-by-the-UTC.patch added |
---|
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
Milestone: | unknown → 0.26.1 |
---|---|
Owner: | set to danielk |
Status: | new → accepted |
comment:3 follow-up: 4 Changed 11 years ago by
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 Changed 11 years ago by
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
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
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
comment:8 Changed 11 years ago by
Resolution: | fixed |
---|---|
Status: | closed → new |
Type: | Patch - Bug Fix → Bug Report - General |
Changed 11 years ago by
Attachment: | 11197-v2.patch added |
---|
comment:9 Changed 11 years ago by
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
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
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.
How about adding the conversion here: http://code.mythtv.org/trac/browser/mythtv/mythplugins/mythzoneminder/mythzoneminder/zmclient.cpp#L338 maybe replacing
with a prettified variant of
is all it takes