Modify
Warning Please read the Ticket HowTo before creating or commenting on a ticket. Failure to do so may cause your ticket to be rejected or result in a slower response.

Opened 18 months ago

Closed 17 months 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@…> 18 months ago.
11197-v2.patch (13.7 KB) - added by danielk 18 months ago.

Download all attachments as: .zip

Change History (13)

Changed 18 months ago by paulh <mythtv@…>

comment:1 Changed 18 months 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 18 months ago by danielk

  • Milestone changed from unknown to 0.26.1
  • Owner set to danielk
  • Status changed from new to accepted

comment:3 follow-up: Changed 18 months 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 18 months 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 18 months 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 18 months ago by Daniel Thor Kristjansson <danielk@…>

  • Resolution set to fixed
  • Status changed from accepted to closed

In 6122c1bd42b0b8df80a797347daf613eafd8da46/mythtv:

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

comment:7 Changed 18 months 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 18 months ago by danielk

  • Resolution fixed deleted
  • Status changed from closed to new
  • Type changed from Patch - Bug Fix to Bug Report - General

Changed 18 months ago by danielk

comment:9 Changed 18 months 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 17 months 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 17 months ago by Daniel Kristjansson <danielk@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 2e11c6aa842d14d1e79b3c9e403059f6a39d9b66/mythtv:

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

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'new'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.