CDateTime: use std::chrono and std::date libraries #18727

basilgello · 2020-11-04T19:00:27Z

Description

Rework CDateTime core component using C++ libraries.

Motivation and Context

This is a continuation of @lrusak cdatetime-std-chrono topic. I would like to have it merged in v19 because it fixes Debian builds on i386 (it still does NOT address Y2038 problem, but the discussion with colleagues in Debian Multimedia Team revealed that it is not necessary at this point).

How Has This Been Tested?

Kodi trunk builds with this PR on Debian Linux i386/amd64/x32 passing all the tests
Casual operation for >1 week shows no issues with date and time conversions
PVR times are correctly shown as local time
JSON-RPC times are correctly shown as UTC
PVR search dialog returns correct results with start/end dates and times as search criteria

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

basilgello · 2020-11-04T19:05:53Z

I am now evaluating the possibility to ise zoneinfo.dat (one-file binary representation of tzdata compiled with zic - the official tinezone info compiler bundled in tzcode by Paul Eggert) as a cross-platform source of timezone data for resource.timezone add-on. In Debian, I am forced to use Debian-packaged tzdata but I will do other platforms builds to verify it works.

basilgello · 2020-11-04T19:18:10Z

What I need now is maximal testing on OSX, FreeBSD, Android and non-Debian Linux.

The CDateTime::m_time field used to store local time (which was converted to system / UTC time by CDateTime::GetAsUTCDateTime() where needed). Now as std::chrono::time_point it stores system / UTC time instead, so I had to remove CDateTime::SetFromUTCDateTime(), CDateTime::GetAsUTCDateTime() and add CDateTime::GetAsLocalDateTime() instead and use this new helper function in all places where CDateTime::SetFromUTCDateTime() was previously used.

To make sure I did not miss anything, testing is a must.

lrusak · 2020-11-04T20:15:42Z

Something that is for sure busted is the date entry dialogs. You can test this by creating a new smart playlist, adding a rule for date created and try to set a date. (EDIT: the code in question is here -> https://github.com/xbmc/xbmc/blob/master/xbmc/dialogs/GUIDialogNumeric.cpp)

I'm not sure what @DaveTBlake thinks but I would think this might be too risky (and too late) for v19.

lrusak · 2020-11-04T20:16:43Z

This also won't work on windows as that platform doesn't use the normal depends build system. We would have to get @Paxxi to compile the date lib for windows so it can be added to the build.

DaveTBlake · 2020-11-04T22:51:49Z

Since CDateTime is so fundamental and has broad impact this does seem risky for the timescales we want to push for the release of v19. So please continue to pursue this work @basilgello , but be prepared for it not to merge into v19.

Despite the code not being directly touched, from previous experience changes to datetime can trigger the entire music library to be rescanned on the next library update (can be a slow process on a large library). This is because datetime is used in the formation of hash values used it the folder/file checking process. So can some testing include having a previously populated music library please.

lrusak · 2020-11-05T00:31:19Z

Despite the code not being directly touched, from previous experience changes to datetime can trigger the entire music library to be rescanned on the next library update (can be a slow process on a large library). This is because datetime is used in the formation of hash values used it the folder/file checking process. So can some testing include having a previously populated music library please.

The digest changes so a scan will be forced, see: https://github.com/xbmc/xbmc/pull/18727/files#diff-b9394f09c93c1f12392dcc243a628ecdc2d228671f58d45d14ab2d1175b3d8f3R1250

basilgello · 2020-11-05T08:20:49Z

Something that is for sure busted is the date entry dialogs. You can test this by creating a new smart playlist, adding a rule for date created and try to set a date. (EDIT: the code in question is here -> https://github.com/xbmc/xbmc/blob/master/xbmc/dialogs/GUIDialogNumeric.cpp)

Now I see what you mean, thanks! Will fix it in the next few days.

The digest changes so a scan will be forced, see: https://github.com/xbmc/xbmc/pull/18727/files#diff-b9394f09c93c1f12392dcc243a628ecdc2d228671f58d45d14ab2d1175b3d8f3R1250

Isn't the proper solution here to increase database version and create a migration wrapper function (maybe, keeping one or two functions from the old CDateTime implementation just for the purpose of migration)? I can do that.

lrusak · 2021-04-07T22:33:08Z

@basilgello did you want to continue with this or did you want me to take it over?

lrusak · 2021-04-07T22:38:42Z

The digest changes so a scan will be forced, see: https://github.com/xbmc/xbmc/pull/18727/files#diff-b9394f09c93c1f12392dcc243a628ecdc2d228671f58d45d14ab2d1175b3d8f3R1250

Isn't the proper solution here to increase database version and create a migration wrapper function (maybe, keeping one or two functions from the old CDateTime implementation just for the purpose of migration)? I can do that.

no, re-scanning isn't a problem, it's just good to be aware that hashes will have to be recalculated because of the size of type change. It's a one time thing that will happen and doesn't really have to do anything with the database. If someone downgrades to a version that doesn't use this PR the hashes will just be recalculated again.

basilgello · 2021-04-08T03:45:37Z

@basilgello did you want to continue with this or did you want me to take it over?

I did include this in Debian package of v19 since RC1. So far there is only one unresolved bug with that patch exists in Debian. I am going to hunt the root cause of that one for v19.1. As for v20, I think you should take over my changes and I will keep testing / fixing stuff. We need the DB schema upgrade from @DaveTBlake to resolve the music library issue now.

The 'GetAsStringsWithBias' test logic assumed the timezone on machine bullding Kodi is set to 'UTC'. This is not always the case, plus hour value was declared with a typo. To address this, we compare outputs of two runs of date library, and instead add a 'Tzdata' test case comparing local date and time against computed values from different timezones: find /usr/share/zoneinfo/Etc/ -type f | sort -Vu | sed 's,^/usr/share/zoneinfo/,,' | while read TZ do TMSTR=$(LANG=C TZ="$TZ" date '+%Y-%m-%dT%H:%M:%S%Ez' -d "1991-05-14 12:34:56 UTC" | sed 's/[0-9][0-9]$/:&/') echo " // LANG=C TZ=\"$TZ\" date '+%Y-%m-%dT%H:%M:%S%Ez' -d \"1991-05-14 12:34:56 UTC\" | sed 's/[0-9][0-9]$/:&/'" echo " tps = date::floor<std::chrono::seconds>(dateTime.GetAsTimePoint());" echo " zone = date::make_zoned(\"$TZ\", tps);" echo " EXPECT_EQ(date::format(\"%FT%T%Ez\", zone), \"$TMSTR\") << \"tzdata information not valid for '$TZ'\";" echo "" done

The 'm_time' now represents a UTC time-point, so 'GetAsUTCDateTime' does nothing useful. Remove it fully, and add 'GetAsLocalDateTime' function that we will use later to convert UTC (system) time to the local time needed to get displayed in UI. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

... from local time by default to UTC time by default. Namely, 'CDateTime::SetFromUTCDateTime' converted the UTC time to local time in the implementation of CDateTime before 'std::chrono'. Now this conversion has gone, and to avoid displaying PVR times in UTC, 'GetAsLocalDateTime' is added where necessary. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

* The 'const CDateTime& CPVRRecording::RecordingTimeAsLocalTime() const' method needs 'static' CDateTime instance set explicitly. This manifested in Debian Bug #980038 : the return value of function is explicitly nulled out causing immediate crash when using any PVR addon manipulating PVR recordings. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Fixes pvr.dvbviewer#49 Closes: #984682 Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

This should fix the GUI date dialog issue: xbmc#18727 (comment) xbmc#18727 (comment) Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

This should leave the possibility of time_t truncation on 32-bit architectures only in GetAsTime(), what we can do nothing with. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Fixes broken timezone picker as reported in Debian Bug#1011294. Also fixes TZ-related issue spotted on reproducibility build where TZ envvar specifying timezone as absolute pathname on Linux and FreeBSD screwed date lib up. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

* Apply clang-format fixes on CDateTime * Drop unused "persistence" variable from Scraper Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

yol · 2024-04-01T16:52:43Z

@basilgello using floating point data types for time points and time spans has lots of implications, so I'd rather not go down that route. is it really necessary? we have been fine with integers so far, so I guess there must be some way to stay with that? I have to say though that I didn't read the 300+ comments this PR already has, so I hope you forgive me if this was already discussed.

basilgello · 2024-04-01T17:31:08Z

@yol

Rationale about doing this:

#18727 (comment)
#18727 (comment)
#18727 (comment)

I have this code basically since 2021 in Debian and no user ever complained in Bullseye, Bookworm and now Trixie.

basilgello · 2024-04-01T17:50:47Z

If you insist on integer type AND nanosecond precision, we can use something like https://github.com/int128-libraries/curtint128 to enable 128-bit integers even on 32-bit architectures. GCC14 supports them out of the box on 64-bit architectures only.

yol · 2024-04-01T19:38:58Z

Thanks for providing the context. I don't see any reason why we should need nanosecond precision though (in general as well as in the linked comments)? We didn't have it before as well, after all. Python datetime (which I guess most addons use) is also "limited" to microseconds. This is also only about the default handling in CDateTime. If necessary, anyone can just use std::chrono directly with whatever resolution is provided/necessary.

So I'd definitely throw my vote in the "microseconds as 64-bit integer" basket :-)

basilgello · 2024-04-01T20:04:10Z

TBH I also dont recall why I decided we need ns resolution - I should have noted this somewhere. From the quick search over this repo we use nanoseconds in Starfish codec (audio and video), in Neptune UPnP (but not datetime yet!) and struct FILETIME in Windows is number of 100 nanosecond intervals since Jan 1st 1601. If we really are OK with microseconds - the change is trivial :)

yol · 2024-04-01T20:08:27Z

So wouldn't the safest choice be to replicate FILETIME with the 100 ns resolution? Or does this create other problems?

basilgello · 2024-04-01T20:21:20Z

I did not test but 100ns resolution is easy doable - you need to multiply or divide a factor of 10 to get ms to filetime-ish conversion.

yol · 2024-04-06T15:38:23Z

Then I think defining the "standard" (i.e., what is used for CDateTime) to 100 ns resolution would be the preferred solution from my side 👌 it has sufficient accuracy for the usual operations with dates and will likely cause the least problems in transition

HowardHinnant · 2024-04-06T16:10:46Z

Just in case this helps:

using Duration = std::chrono::duration<std::int64_t, std::ratio<1, 10'000'000>>;
using TimePoint = std::chrono::time_point<std::chrono::system_clock, Duration>;

basilgello requested review from DaveTBlake and ksooo as code owners November 4, 2020 19:00

basilgello mentioned this pull request Nov 4, 2020

[Linux][i686]Failing TestDateTime tests expose Y2038 problem #18594

Open

6 tasks

DaveTBlake added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality v20 Nexus WIP PR that is still being worked on labels Nov 4, 2020

basilgello force-pushed the cdatetime-std-chrono branch 3 times, most recently from 375d729 to 1ac2cdc Compare November 10, 2020 16:05

basilgello force-pushed the cdatetime-std-chrono branch from 1ac2cdc to adf30f8 Compare November 19, 2020 18:07

basilgello force-pushed the cdatetime-std-chrono branch from adf30f8 to c7b701b Compare December 19, 2020 04:35

lrusak added the No Jenkins do not run automatic Jenkins builds on this PR label Dec 19, 2020

basilgello force-pushed the cdatetime-std-chrono branch from c7b701b to b36e06f Compare December 27, 2020 06:07

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Jan 6, 2021

basilgello force-pushed the cdatetime-std-chrono branch from b36e06f to 60420eb Compare January 6, 2021 22:32

basilgello force-pushed the cdatetime-std-chrono branch 2 times, most recently from c2620f5 to f299313 Compare January 19, 2021 03:28

basilgello force-pushed the cdatetime-std-chrono branch from f299313 to ae97c4b Compare February 19, 2021 11:13

lrusak added this to the N* 20.0 Alpha 1 milestone Apr 7, 2021

lrusak and others added 19 commits March 22, 2024 13:45

add resource.timezone

9af15b4

CDateTime: convert to use std::chrono

07f2b46

[test] fix TestDateTime to work with std::chrono

142d39c

cleanup CPosixTimezone

14ba848

utils/XTimeUtils: clean up SystemTime and FileTime functions

eecf78e

Fix one-time assignment of PVR recording times

856c5cc

Fixes pvr.dvbviewer#49 Closes: #984682 Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Move date includes to separate header

c84055f

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

CDateTime: Use 'long double' as underlying storage type for time points

15f62bc

This should fix the GUI date dialog issue: xbmc#18727 (comment) xbmc#18727 (comment) Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Use "long double" type in time_t / tm conversions

ea630c4

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Avoid time_t in tm conversions

70ec67d

This should leave the possibility of time_t truncation on 32-bit architectures only in GetAsTime(), what we can do nothing with. Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Windows patches

26175f7

Apply formatting fixes

094b235

* Apply clang-format fixes on CDateTime * Drop unused "persistence" variable from Scraper Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

Move LoadTimeZoneDatabase to xbmc/utils/DateLib.cpp

efdf519

Signed-off-by: Vasyl Gello <vasek.gello@gmail.com>

WIP: Load Unicode CLDR from github

b0f2d8e

basilgello force-pushed the cdatetime-std-chrono branch from 8e5017f to b0f2d8e Compare March 22, 2024 11:47

lrusak mentioned this pull request Mar 30, 2024

Remove timezone setting within Kodi #24899

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDateTime: use std::chrono and std::date libraries #18727

CDateTime: use std::chrono and std::date libraries #18727

basilgello commented Nov 4, 2020

basilgello commented Nov 4, 2020

basilgello commented Nov 4, 2020

lrusak commented Nov 4, 2020 •

edited

lrusak commented Nov 4, 2020

DaveTBlake commented Nov 4, 2020

lrusak commented Nov 5, 2020

basilgello commented Nov 5, 2020

lrusak commented Apr 7, 2021

lrusak commented Apr 7, 2021

basilgello commented Apr 8, 2021 via email

yol commented Apr 1, 2024

basilgello commented Apr 1, 2024

basilgello commented Apr 1, 2024

yol commented Apr 1, 2024

basilgello commented Apr 1, 2024

yol commented Apr 1, 2024

basilgello commented Apr 1, 2024

yol commented Apr 6, 2024

HowardHinnant commented Apr 6, 2024

CDateTime: use std::chrono and std::date libraries #18727

Are you sure you want to change the base?

CDateTime: use std::chrono and std::date libraries #18727

Conversation

basilgello commented Nov 4, 2020

Description

Motivation and Context

How Has This Been Tested?

Types of change

Checklist:

basilgello commented Nov 4, 2020

basilgello commented Nov 4, 2020

lrusak commented Nov 4, 2020 • edited

lrusak commented Nov 4, 2020

DaveTBlake commented Nov 4, 2020

lrusak commented Nov 5, 2020

basilgello commented Nov 5, 2020

lrusak commented Apr 7, 2021

lrusak commented Apr 7, 2021

basilgello commented Apr 8, 2021 via email

yol commented Apr 1, 2024

basilgello commented Apr 1, 2024

basilgello commented Apr 1, 2024

yol commented Apr 1, 2024

basilgello commented Apr 1, 2024

yol commented Apr 1, 2024

basilgello commented Apr 1, 2024

yol commented Apr 6, 2024

HowardHinnant commented Apr 6, 2024

lrusak commented Nov 4, 2020 •

edited