GNOME Bugzilla – Bug 628408
Use GDateTime that has been released
Last modified: 2012-08-02 09:14:56 UTC
The new glib release (2.25.15) contains GDateTime and we still haven't released a core/base including GstDateTime. We should add an #if/#else and use GDateTime if that is available on the system. If someone has this new version available, he/she will be able to use all GDateTime API, while if someone doesn't, it will fall back to our minimalist GstDateTime API.
Created attachment 171353 [details] [review] datetime: Use GDateTime if available Only use GstDateTime if the glib version still doesn't have GDateTime. This makes us avoid duplicate effort with glib devels
Comment on attachment 171353 [details] [review] datetime: Use GDateTime if available >diff --git a/gst/gstdatetime.h b/gst/gstdatetime.h >index 26b65d4..c3d9574 100644 >--- a/gst/gstdatetime.h >+++ b/gst/gstdatetime.h >@@ -22,19 +22,24 @@ > > #include <time.h> > #include <glib.h> >+#include "glib-compat-private.h" This private header is not installed, so we can't include it in a public header. >+#ifndef GLIB_HAS_GDATETIME > typedef struct _GstDateTime GstDateTime; >+#else >+typedef GDateTime GstDateTime; >+#endif IMHO GstDateTime should *always* be 100% opaque at the API level, even if internally we just map it to GDateTime (more on that below though). We should provide API (a function) to get a GDateTime from a GstDateTime though. If libgstreamer was built against GLib < 2.26 that function would return NULL. gstdatetime.h would define GDateTime to gpointer if we are building against GLib headers < 2.26. Part of the reason I'd like it 100% opaque is that I think we can't just do the 1:1 mapping, not even internally, with the current GDateTime functionality: since the main use case for GstDateTime is to express dates and times from tags, we need to be able to express/set/query the 'granularity' of the datetime provided, so that we can express incomplete date/times properly (ie. "year 2010" or "2010-Aug-08, unknown time"). This is important for transcoding/retagging, so we can, err, "transtag"/rewrite such incomplete tags correctly, without adding bogus information (like making 'year 2010' into 2010-Jan-01, 00:00.000h UTC).
Created attachment 172132 [details] [review] glib-private: Add protection include macro
Created attachment 172133 [details] [review] datetime: Use GDateTime if available Updates with Tim's comments.
Created attachment 172134 [details] [review] gstdatetime: Move doc outside the ifdefs Move the datetime documentation of the functions outside the ifdefs
There are still 2 differences from our API to glib's. 1) They use a double to represent seconds + microseconds. We have 2 separate fields in functions. 2) They use a string for representing a timezone (+0300 or America/Recife or ...) as they can parse tzdata. And for the timezone offset they use microseconds resolution. We are using a double in hours resolution. Item 1 is simple enough to update on our side. For item 2 we could take our timezone handling to microseconds resolution too. glib does it because their datetime math API is based on microseconds, so it makes some sense. For us it doesn't make, but we get consistency with their API. What do you think?
Comment on attachment 172132 [details] [review] glib-private: Add protection include macro > glib-private: Add protection include macro Looks ok (not sure if the glib-compat-private.h stuff is needed here though, see other comment)
Comment on attachment 172133 [details] [review] datetime: Use GDateTime if available >Use GDateTime internally on GstDateTime if glib already >provides it. >--- a/gst/glib-compat-private.h >+++ b/gst/glib-compat-private.h >@@ -8,7 +8,6 @@ > #ifndef __GLIB_COMPAT_PRIVATE_H__ > #define __GLIB_COMPAT_PRIVATE_H__ > >-#include "gst_private.h" /* for g_warning */ > #include <glib.h> > > G_BEGIN_DECLS >@@ -17,6 +16,10 @@ G_BEGIN_DECLS > typedef struct stat GStatBuf; > #endif > >+#if GLIB_CHECK_VERSION(2,26,0) >+#define GLIB_HAS_GDATETIME >+#endif >+ > /* copies */ > > /* adaptations */ >diff --git a/gst/gstdatetime.c b/gst/gstdatetime.c >index e02ea25..00cab13 100644 >--- a/gst/gstdatetime.c >+++ b/gst/gstdatetime.c >@@ -21,8 +21,12 @@ > #include "config.h" > #endif > >+#include "glib-compat-private.h" Since it's not used anywhere else, maybe this define should just go into gstdatetime.c instead?
> There are still 2 differences from our API to glib's. > > 1) They use a double to represent seconds + microseconds. We have 2 separate > fields in functions. Makes sense to change our API to match GLib's here IMHO. Don't think people care too much about micro-seconds in our main usage scenario (tags). > 2) They use a string for representing a timezone (+0300 or America/Recife or > ...) as they can parse tzdata. And for the timezone offset they use > microseconds resolution. We are using a double in hours resolution. > (...) > For item 2 we could take our timezone handling to microseconds resolution too. > glib does it because their datetime math API is based on microseconds, so it > makes some sense. For us it doesn't make, but we get consistency with their > API. > > What do you think? No strong opinion, I could live with both. I think passing timezone offsets in microseconds is a bit weird though.
Created attachment 172219 [details] [review] gstdatetime: Move doc outside the ifdefs
Created attachment 172220 [details] [review] datetime: Use seconds as double Use seconds as double to make API similar to glib's gdatetime. Also move timezone parameter to the first position, just like glib's.
Created attachment 172221 [details] [review] datetime: Handle timezone offset in usecs Handle timezone offsets in microseconds, just like glib API does. This doesn't seem nice and doesn't produce a good looking code, but our goal here is to be as close as possible to glib's API. So... This is the weird one, I prefer to do the same as glib here, as it makes it easier for people to use one or another.
Created attachment 172223 [details] [review] datetime: Use seconds as double Of course I forgot something here.
Created attachment 172224 [details] [review] datetime: Handle timezone offset in usecs Updated version.
Btw, already got a patch for -base.
Comment on attachment 172223 [details] [review] datetime: Use seconds as double > datetime: Use seconds as double Should we also add g_return_if_fail() or g_warn_if_fail() for the tzoffset arguments here? (might have missed them though)
Comment on attachment 172224 [details] [review] datetime: Handle timezone offset in usecs > datetime: Handle timezone offset in usecs Bah :)
Comment on attachment 172223 [details] [review] datetime: Use seconds as double Updated with the check and commited.
Comment on attachment 172224 [details] [review] datetime: Handle timezone offset in usecs Ok, so not pushing this. It looks ugly.
commit e9312870e5881295b52853825e7dab95a9c34b6e commit 0d3c623b4b7c1e5ee44e5ad67c59a724b0d8efc6 commit c7e5bc1e5d7ff76aec986aa36eed860d4df484f2 commit 6d883ed95c19ca26eb7df479a6c53ac51c9e6c12 pushed to core.
Wouldn't gstreamer 1.0 be a good time to replace GstDateTime with GDateTime?
> Wouldn't gstreamer 1.0 be a good time to replace GstDateTime with GDateTime? We need additional features that GDateTime does not support, namely partial dates (only year, only year/month, etc.), and had little hope that we'd have been able to push those into GLib for the GLib release version we would have needed them for.