After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 594504 - Need a GType of "Date AND Time AND Timezone"
Need a GType of "Date AND Time AND Timezone"
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 606405 634999 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-09-08 14:32 UTC by Edward Hervey
Modified: 2010-11-16 17:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstvalue: Add datetime gvalue (9.79 KB, patch)
2010-04-29 21:21 UTC, Thiago Sousa Santos
rejected Details | Review
taglist: Add datetime get functions (4.92 KB, patch)
2010-06-29 17:49 UTC, Thiago Sousa Santos
none Details | Review
tag: Adds GST_TAG_DATE_TIME tag (2.04 KB, patch)
2010-06-29 17:50 UTC, Thiago Sousa Santos
none Details | Review
gstvalue: Adds datetime functions (5.00 KB, patch)
2010-06-29 17:51 UTC, Thiago Sousa Santos
none Details | Review
gststructure: Adds datetime getter function (4.00 KB, patch)
2010-06-29 17:51 UTC, Thiago Sousa Santos
none Details | Review
gstvalue: Adds tests for datetime (5.21 KB, patch)
2010-06-29 17:52 UTC, Thiago Sousa Santos
none Details | Review
tag: Adds GST_TAG_DATE_TIME tag (2.04 KB, patch)
2010-06-29 18:07 UTC, Thiago Sousa Santos
none Details | Review
gstvalue: Adds datetime functions (6.24 KB, patch)
2010-07-01 13:14 UTC, Thiago Sousa Santos
none Details | Review
gstvalue: Adds tests for datetime (14.24 KB, patch)
2010-07-01 13:15 UTC, Thiago Sousa Santos
none Details | Review
tag: xmp: Maps GST_TAG_DATE_TIME (8.07 KB, patch)
2010-07-01 13:22 UTC, Thiago Sousa Santos
none Details | Review
tag: exif: Map GST_TAG_DATE_TIME (7.30 KB, patch)
2010-07-01 13:22 UTC, Thiago Sousa Santos
none Details | Review
jifmux: Adds tests for GST_TAG_DATE_TIME (3.93 KB, patch)
2010-07-01 13:23 UTC, Thiago Sousa Santos
none Details | Review
gstdatetime: Adds GstDateTime (29.63 KB, patch)
2010-07-17 18:38 UTC, Thiago Sousa Santos
none Details | Review
taglist: Add datetime get functions (5.12 KB, patch)
2010-07-17 18:40 UTC, Thiago Sousa Santos
none Details | Review
gstvalue: Adds datetime functions (7.90 KB, patch)
2010-07-17 18:41 UTC, Thiago Sousa Santos
none Details | Review
tag: Adds GST_TAG_DATE_TIME tag (2.04 KB, patch)
2010-07-17 18:43 UTC, Thiago Sousa Santos
none Details | Review
gststructure: Adds datetime getter function (4.15 KB, patch)
2010-07-17 18:44 UTC, Thiago Sousa Santos
none Details | Review
gstvalue: Adds tests for datetime (8.11 KB, patch)
2010-07-17 18:45 UTC, Thiago Sousa Santos
none Details | Review
gstdatetime: Adds GstDateTime (28.75 KB, patch)
2010-07-21 21:49 UTC, Thiago Sousa Santos
committed Details | Review
taglist: Add datetime get functions (13.40 KB, patch)
2010-07-21 21:50 UTC, Thiago Sousa Santos
needs-work Details | Review
tag: Adds GST_TAG_DATE_TIME tag (2.04 KB, patch)
2010-07-21 21:50 UTC, Thiago Sousa Santos
committed Details | Review
gststructure: Adds datetime getter function (4.15 KB, patch)
2010-07-21 21:51 UTC, Thiago Sousa Santos
committed Details | Review
gstvalue: Adds tests for datetime (8.36 KB, patch)
2010-07-21 21:51 UTC, Thiago Sousa Santos
committed Details | Review
gstvalue: Adds datetime functions (8.48 KB, patch)
2010-07-22 01:35 UTC, Thiago Sousa Santos
committed Details | Review
taglist: Add datetime get functions (5.12 KB, patch)
2010-07-22 01:35 UTC, Thiago Sousa Santos
committed Details | Review

Description Edward Hervey 2009-09-08 14:32:42 UTC
We don't have a unified type for Date/Time/Timezone information.

Several elements can parse that information from various streams (demuxers, exif tag parsers, etc...) , but since we don't have such a type, we can't use that information in a uniform way.

What we should have is a new GType which stores all the following in a GValue:
* Date (YYYY/MM/DD)
* Time (HH:MM:SS.XXX) (It would be nice to have millisecond precision at least)
* Timezone (+/- HH:MM)
* Dailyght Saving Time (applied or not)

SMPTE specifications S12M and S309M have explanations on how to pack this information using BCD coding.

This should come with methods or macros to easily read/write all the various information from the new type.
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-08 14:39:10 UTC
Looks good. Exif wants 
DateTime as string, "YYYY:MM:DD HH:MM:SS"
GPSTimeStamp as 3 rational, time in UTC (Coordinated Universal Time).
GPSDateStamp as string, "YYYY:MM:DD"

all can be derived from it.
Comment 2 Edward Hervey 2009-09-08 14:45:58 UTC
I'm guessing unused fields can just be set to reasonable defaults:
* time fields set to 0
* date/m/y set to 1
* timezone set to UTC
* DST ... no idea (off ?)
Comment 3 Sebastian Dröge (slomo) 2009-09-08 14:47:26 UTC
For defining how we store this in a GValue please keep in mind, that we have 2*64 bits that can be filled. But we should nonetheless use BCD because it wastes space for nothing (AFAIK it's only useful if you want to directly show it on those 7 element LED displays).

I guess with the 2*64 bits we can even get nanoseconds resolution.


Now the question is how we represent all those parts.

Date: Some kind of MJD or YYYY-MM-DD format or +-days since some date? Or multiple possible formats like in S12M, which are selected by a single bit?
Time: Nano/Micro/Milliseconds from start of day? Or all parts separate, i.e. HH:MM:SS.X?
Timezone: Probably something like the Timezone representation in S309M, i.e. an enumeration for the 48 possible timezones.
Comment 4 Wim Taymans 2009-09-08 14:52:41 UTC
Correction: there are 2 64 bit values in a GValue available for encoding this.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2009-09-08 14:53:45 UTC
I would pick something that is easy to serialize. Please not multiple formats
in there, lets rather have datetime helpers to convert, if we find e.g. SMPTE
timestamps in multiple plugins.

Date: days since an offset (I would even be fine with days since 1. Jan. 0000).
Time: nanoseconds since the star of the day?
tz: looks good
Comment 6 Sebastian Dröge (slomo) 2009-09-08 15:04:17 UTC
For the time in nanoseconds since start of day we would need 47 bits. Sounds good IMHO.

For the timezone we would need 6 bits, DST 1 bit. This makes 54 bits, leaving 74 bits for the date itself.

So offset in days from whatever date you want should be possible, so let's take MJD. Note however that it's not really trivial to convert this into YYYY-MM-DD notation because of leap years and all that. But it's not too complex, even GLib does it for GDate ;)


Another thing that we should think about is the serialization that we want.
Comment 7 Sebastian Dröge (slomo) 2009-09-08 15:07:48 UTC
For the timezone, there actually is a non-30 minute timezone: Chatham Island have UTC+12:45. So maybe we should encode the timezone in minutes, then we're on the save side ;) That would be 11 bit then.
Comment 8 Tim-Philipp Müller 2009-09-08 15:14:15 UTC
Why not just a G_TYPE_STRING and a suitable well-defined string format that contains all the information needed and is easy to parse?
Comment 9 Arnout Vandecappelle 2009-09-08 15:26:13 UTC
Why not use a GstClockTimeDiff against the Epoch and a timezone in some representation (string or enum)?  That allows you to convert to anything, even other calendar systems (= generalized timezone).

I think it's important to store the UTC timestamp directly, because whatever unified processing you do on it, it will probably be done on the UTC timestamp...
Comment 10 David Schleef 2009-09-08 20:33:24 UTC
ISO 8601 seems appropriate for this.
Comment 11 Sebastian Dröge (slomo) 2009-09-09 04:33:12 UTC
(In reply to comment #10)
> ISO 8601 seems appropriate for this.

That doesn't handle DST separate from the timezone and Edward wanted that (why?)
Also see my the last comment a few lines below, ISO 8601 would require to use a string.

(In reply to comment #9)
> Why not use a GstClockTimeDiff against the Epoch and a timezone in some
> representation (string or enum)?  That allows you to convert to anything, even
> other calendar systems (= generalized timezone).
> 
> I think it's important to store the UTC timestamp directly, because whatever
> unified processing you do on it, it will probably be done on the UTC
> timestamp...

This will only allow about +-300 years from the epoch.

(In reply to comment #8)
> Why not just a G_TYPE_STRING and a suitable well-defined string format that
> contains all the information needed and is easy to parse?

Edward wanted to not require an additional allocation and store it directly inside the GValue. Also, even if we define a string format, it still is uglier to parse than some binary format.
Comment 12 Arnout Vandecappelle 2009-09-09 09:06:49 UTC
(In reply to comment #6)
> For the time in nanoseconds since start of day we would need 47 bits. Sounds
> good IMHO.
> 
> For the timezone we would need 6 bits, DST 1 bit. This makes 54 bits, leaving
> 74 bits for the date itself.
> 
> So offset in days from whatever date you want should be possible, so let's take
> MJD. Note however that it's not really trivial to convert this into YYYY-MM-DD
> notation because of leap years and all that. But it's not too complex, even
> GLib does it for GDate ;)

 However, if it's not in nanoseconds, every operation on it is really non-trivial.

 By the way, what is the point of splitting the representation into time and date?  Makes it difficult to deal with leap seconds and DST shifts...

(In reply to comment #11)
> (In reply to comment #9)
> > Why not use a GstClockTimeDiff against the Epoch and a timezone in some
> > representation (string or enum)?  That allows you to convert to anything, even
> > other calendar systems (= generalized timezone).
> > 
> > I think it's important to store the UTC timestamp directly, because whatever
> > unified processing you do on it, it will probably be done on the UTC
> > timestamp...
> 
> This will only allow about +-300 years from the epoch.

 OK, take additional bits from the second part of the GValue.  You can make a shortcut check against 0 to ascertain that the normal GstClockTimeDiff operations are valid.

 However, I'm not even sure if dates beyond 300 years from the epoch really need to be supported...

 So, my proposal is:
data[0].v_int64 as a GstClockTimeDiff against the Epoch
data[1].v_int as a GstTimeZone enum

 Regards,
 Arnout
Comment 13 Edward Hervey 2009-09-09 09:29:46 UTC
> That doesn't handle DST separate from the timezone and Edward wanted that
> (why?)

  Because 12:00 at GMT (!DST) does not represent the same moment in time as 6:00 GMT-6 (DST). Different countries changed DST at different times. And some countries don't at all.

  Real case example :
   Plenty of people from around the world come to a certain conference. Some will set their clock properly, some others not. People take plenty of photos/videos, and at the end of the conference you grab them all because you want to make a nice edit.
   Now there's one scene which many people took (like Linus shaving bdale's beard) from many different angles. You want to be able to figure out really quickly which photos/videos happen at the same time. If you have the *exact* time for everything... you can do it easily... else you'll have to find it yourself.


> Strings

  NO NO NO NO NO and NO ! Strings are for human beings and are a serious PITA to parse ! The whole point of this type is to have a numerical type to store all this info which have a clearly defined meaning.
  One use case I have in mind for this type are original recording date/time for videos/photos, which are repeated often. String parsing is expensive and more prone to errors than numbers.

  That being said... we can very easily have converters to/from some string representations (like ISO 8601).
Comment 14 David Schleef 2009-09-09 09:41:31 UTC
Timezone, including DST, is just a human readable string that people are more likely to remember and be comfortable with than the timezone offset.  The only way to unambiguously represent a time is with UTC, or localtime +/- UTC offset.

So the remaining question is: do you want to store 3-4 letter timezone ID (which btw are not unique, nor unambiguous) and a bit for "DST"?
Comment 15 Sebastian Dröge (slomo) 2009-09-09 09:48:06 UTC
(In reply to comment #14)
> Timezone, including DST, is just a human readable string that people are more
> likely to remember and be comfortable with than the timezone offset.  The only
> way to unambiguously represent a time is with UTC, or localtime +/- UTC offset.
> 
> So the remaining question is: do you want to store 3-4 letter timezone ID
> (which btw are not unique, nor unambiguous) and a bit for "DST"?

The timezone would be stored as offset to UTC (i.e. +0800 etc). Question is, do we need to store DST? IMHO DST or not just changes the timezone by one hour
Comment 16 Arnout Vandecappelle 2009-09-09 10:16:26 UTC
(In reply to comment #13)
> > That doesn't handle DST separate from the timezone and Edward wanted that
> > (why?)
> 
>   Because 12:00 at GMT (!DST) does not represent the same moment in time as
> 6:00 GMT-6 (DST). Different countries changed DST at different times. And some
> countries don't at all.
> 
>   Real case example :
>    Plenty of people from around the world come to a certain conference. Some
> will set their clock properly, some others not. People take plenty of
> photos/videos, and at the end of the conference you grab them all because you
> want to make a nice edit.
>    Now there's one scene which many people took (like Linus shaving bdale's
> beard) from many different angles. You want to be able to figure out really
> quickly which photos/videos happen at the same time. If you have the *exact*
> time for everything... you can do it easily... else you'll have to find it
> yourself.

 So why do you need timezone info and not just UTC timestamp?  You need to be able to parse different string formats into consistent timestamps so you can compare them.  Timezone info is not relevant at that point.  You could even use a GTimeVal (except that that doesn't go beyond 2038 and doesn't have nanosec precision).

 For me, the use case was e.g. picking images from a video stream, storing them as JPEG files, and making sure the EXIF info has the correct creation date.  For that, you need to add the frame offset to the video creation date, and regenerate the date in the original local time.
Comment 17 Arnout Vandecappelle 2009-09-09 10:19:48 UTC
(In reply to comment #14)
> Timezone, including DST, is just a human readable string that people are more
> likely to remember and be comfortable with than the timezone offset.  The only
> way to unambiguously represent a time is with UTC, or localtime +/- UTC offset.

 That unambiguously represents a time, but doesn't allow you to reproduce localtime after doing an operation on it.  DST shifts are different for different timezones, and are even different in different years.

 An enumeration of the known timezones allows you to deal with all that.  Unfortunately, AFAIK, there is no standardized list of unique timezones.
Comment 18 Tim-Philipp Müller 2010-01-08 13:32:18 UTC
*** Bug 606405 has been marked as a duplicate of this bug. ***
Comment 19 Thiago Sousa Santos 2010-01-08 14:36:34 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=50076

Just adding here the glib bug for it.
Comment 20 Thiago Sousa Santos 2010-01-08 19:17:34 UTC
how about proposing the struct below to glib. 

struct _GDateTime
{
  GDate date;        /* date (DMY) */
  guint32 msecs;     /* milisecs since the start of the day */

  guint16 tz_offset; /* timezone offset in minutes */
  gboolean dst;      /* if it is on daylight saving time */
};

This way we get the g_date_* functions for free.
The other option is already store in the struct the same way we'd like to store in GValues.

What do you think?
Comment 21 Tim-Philipp Müller 2010-04-28 19:20:08 UTC
One more aspect: other than the lack of time information, a problem we currently have with GDate is also that it's not really possible to leave the day field or the day/month fields unspecified. So 1/1/1980 can't be distinguished from 'Year 1980'.
Comment 22 Thiago Sousa Santos 2010-04-29 14:32:14 UTC
FYI, just found this:

http://audidude.com/?p=342

chegert seems to have started implementing GDateTime for glib.
Comment 23 Thiago Sousa Santos 2010-04-29 21:21:06 UTC
Created attachment 159947 [details] [review]
gstvalue: Add datetime gvalue

Simple patch that implements the datetime fields all separate into
the GValue.

From the code:
/*
 * Brief explanation of datetime representation in a GValue:
 *
 * We store each field separately, they are: year, month, day of month,
 * hour, minute, second, nanosecs, timezone (in minutes), dst (bit)
 *
 * Those are stored as:
 * at the first uint64:
 *   unused:24, year:16, month:4, day:5, hour:5, minutes:4, secs:6
 * at the second uint64:
 *   unused:22, nanosecs:30, timezone:11, dst:1
 *
 * TODO:
 * - how to do validation?
 */

How does it sound?

(Yes, tests are needed, this is just to give an idea)
Comment 24 Sebastian Dröge (slomo) 2010-04-30 04:24:16 UTC
Looks good in general but how to you represent 60 minutes in 4 bits? :) And the getters should take a const GValue * as parameter.
Comment 25 Benjamin Otte (Company) 2010-04-30 08:47:57 UTC
Uh, first of all I'd like it much more if this was a boxed type, so you could carry that type around and don't have to use a GValue anywhere. Second, I don't like this being a fundamental type. Fundamental types are supposed to be fundamental, and this clearly is not. Third, why don't you use a struct tm or at least a time_t internally? That way you'd get access to easy formatting functions (aka strftime).

And fourth, I'd love if we had the GDateTime stuff finished in glib first so we'd avoid lots of NIH, but I guess that would take too long to result in anything useful.
Comment 26 Christian Hergert 2010-05-04 05:38:51 UTC
I'd love to push forward on the GDateTime stuff if I can get some constructive criticism.  I know that code and design review takes time, but I would appreciate it.

http://github.com/chergert/gdatetime
Comment 27 Thiago Sousa Santos 2010-06-29 17:45:18 UTC
Comment on attachment 159947 [details] [review]
gstvalue: Add datetime gvalue

We have ongoing work on gdatetime for glib.
Comment 28 Thiago Sousa Santos 2010-06-29 17:49:44 UTC
Created attachment 164909 [details] [review]
taglist: Add datetime get functions

Adds _date_time_get and _date_time_get_index functions to
taglist.

API: gst_tag_list_get_date_time
API: gst_tag_list_get_date_time_index
Comment 29 Thiago Sousa Santos 2010-06-29 17:50:28 UTC
Created attachment 164910 [details] [review]
tag: Adds GST_TAG_DATE_TIME tag

Adds a new tag that represents a date and time a media was
created
Comment 30 Thiago Sousa Santos 2010-06-29 17:51:20 UTC
Created attachment 164911 [details] [review]
gstvalue: Adds datetime functions

Adds a datetime functions to gstvalue
Comment 31 Thiago Sousa Santos 2010-06-29 17:51:53 UTC
Created attachment 164912 [details] [review]
gststructure: Adds datetime getter function

Adds gst_structure_get_date_time function

API: gst_structure_get_date_time
Comment 32 Thiago Sousa Santos 2010-06-29 17:52:19 UTC
Created attachment 164914 [details] [review]
gstvalue: Adds tests for datetime

Adds tests for datetime fields in gstvalue tests
Comment 33 Tim-Philipp Müller 2010-06-29 18:03:19 UTC
Comment on attachment 164910 [details] [review]
tag: Adds GST_TAG_DATE_TIME tag

>+  gst_tag_register (GST_TAG_DATE_TIME, GST_TAG_FLAG_META, G_TYPE_DATE_TIME,
>+      _("datetime"),
>+      _("date and time the data was created (as a GDate structure)"), NULL);

Small typo there: GDate -> GDateTime.
Comment 34 Thiago Sousa Santos 2010-06-29 18:07:22 UTC
Created attachment 164915 [details] [review]
tag: Adds GST_TAG_DATE_TIME tag

Adds a new tag that represents a date and time a media was
created
Comment 35 Thiago Sousa Santos 2010-07-01 13:14:24 UTC
Created attachment 165026 [details] [review]
gstvalue: Adds datetime functions

Updated with a better serialization format
Comment 36 Thiago Sousa Santos 2010-07-01 13:15:18 UTC
Created attachment 165027 [details] [review]
gstvalue: Adds tests for datetime

Updated the tests.
Comment 37 Thiago Sousa Santos 2010-07-01 13:22:05 UTC
Created attachment 165028 [details] [review]
tag: xmp: Maps GST_TAG_DATE_TIME

Adds mapping for GST_TAG_DATE_TIME.
Tests included.
Comment 38 Thiago Sousa Santos 2010-07-01 13:22:28 UTC
Created attachment 165029 [details] [review]
tag: exif: Map GST_TAG_DATE_TIME

Adds mapping to the exif helper library for GST_TAG_DATE_TIME.
Tests included.
Comment 39 Thiago Sousa Santos 2010-07-01 13:23:11 UTC
Created attachment 165030 [details] [review]
jifmux: Adds tests for GST_TAG_DATE_TIME

Adds tests for GST_TAG_DATE_TIME serialization into exif.
Comment 40 Thiago Sousa Santos 2010-07-17 18:38:21 UTC
Created attachment 166085 [details] [review]
gstdatetime: Adds GstDateTime

Adds a GstDateTime type to be used in gstreamer.
Comment 41 Thiago Sousa Santos 2010-07-17 18:40:17 UTC
Created attachment 166086 [details] [review]
taglist: Add datetime get functions

Adds _date_time_get and _date_time_get_index functions to
taglist.

API: gst_tag_list_get_date_time
API: gst_tag_list_get_date_time_index
Comment 42 Thiago Sousa Santos 2010-07-17 18:41:19 UTC
Created attachment 166087 [details] [review]
gstvalue: Adds datetime functions

Adds a datetime functions to gstvalue

API: gst_date_time_compare
Comment 43 Thiago Sousa Santos 2010-07-17 18:43:10 UTC
Created attachment 166088 [details] [review]
tag: Adds GST_TAG_DATE_TIME tag

Adds a new tag that represents a date and time a media was
created

API: GST_TAG_DATE_TIME

Fixes #594504
Comment 44 Thiago Sousa Santos 2010-07-17 18:44:33 UTC
Created attachment 166089 [details] [review]
gststructure: Adds datetime getter function

Adds gst_structure_get_date_time function

API: gst_structure_get_date_time

Fixes #594504
Comment 45 Thiago Sousa Santos 2010-07-17 18:45:19 UTC
Created attachment 166090 [details] [review]
gstvalue: Adds tests for datetime

Adds tests for datetime fields in gstvalue tests

Fixes #594504
Comment 46 Tim-Philipp Müller 2010-07-18 11:29:46 UTC
First the nitpicking:

> taglist: Add datetime get functions

 - is missing Since: markers in gtk-doc chunks


> Adds a datetime functions to gstvalue

 - -> gstvalue: add datetime functions

 - has Since: markers with glib version (2.26)

 - GST_TYPE_DATE_TIME has no Since markers

 - do we need to expose the _compare() function?
   I think we should keep it private/internal for now.

 - I assume when GstDateTime is serialised, we now
  get "(GstDateTime)2010...." right? I wonder if we should
  add a mnemonic without prefix ("datetime"?) so we
  don't have problems with "GDateTime" vs. "GstDateTime"
  later when switching GstDateTime to GDateTime (if it
  ever happens). Of course we can just leave it at "(GstDateTime)"
  and add some special casing later.


> gststructure: Adds datetime getter function

Why do we need a datetime getter for GstStructure?


> gstvalue: Adds tests for datetime

Could you fix up the two chunks at the bottom that gst-indent thinks it needs to re-indent? Just split them into to GValue foo = { ... }; lines.


> gstdatetime: Adds GstDateTime

 - why do we have gst_date_time_get_dmy(), but
   for the time fields it's _get_hour(), get_minute()
   etc.? Instead of one function?
Comment 47 Thiago Sousa Santos 2010-07-18 23:51:39 UTC
(In reply to comment #46)
> First the nitpicking:
> 
> > taglist: Add datetime get functions
> 
>  - is missing Since: markers in gtk-doc chunks

Fixed.

> 
> 
> > Adds a datetime functions to gstvalue
> 
>  - -> gstvalue: add datetime functions
> 
>  - has Since: markers with glib version (2.26)

Fixed.

> 
>  - GST_TYPE_DATE_TIME has no Since markers

Fixed.

> 
>  - do we need to expose the _compare() function?
>    I think we should keep it private/internal for now.

Added its prototype to gst_private.h and it is now private.

> 
>  - I assume when GstDateTime is serialised, we now
>   get "(GstDateTime)2010...." right? I wonder if we should
>   add a mnemonic without prefix ("datetime"?) so we
>   don't have problems with "GDateTime" vs. "GstDateTime"
>   later when switching GstDateTime to GDateTime (if it
>   ever happens). Of course we can just leave it at "(GstDateTime)"
>   and add some special casing later.
> 
> 
> > gststructure: Adds datetime getter function
> 
> Why do we need a datetime getter for GstStructure?

Added those to be used on datetime serialization/deserialization tests (in gstvalue tests).

All other tests uses the GstStructure functions, including GstDate tests.

But I guess I can update the tests to use the GstTagList functions to get them. Is this preferable?

> 
> 
> > gstvalue: Adds tests for datetime
> 
> Could you fix up the two chunks at the bottom that gst-indent thinks it needs
> to re-indent? Just split them into to GValue foo = { ... }; lines.

Fixed.

> 
> 
> > gstdatetime: Adds GstDateTime
> 
>  - why do we have gst_date_time_get_dmy(), but
>    for the time fields it's _get_hour(), get_minute()
>    etc.? Instead of one function?

Because for days you need to get the month first, and for the month, you'd need the year. Getting all 3 at once is faster than getting one by one. For the time functions this doesn't happen.


Have the patches updated locally, will put them here once the still open discussions above are finished.
Comment 48 Tim-Philipp Müller 2010-07-21 11:55:48 UTC
> Added its prototype to gst_private.h and it is now private.

Make sure to prefix it with priv_ or remove the _*gst prefix, otherwise it'll still get exported (see: grep GST_LIB_LDFLAGS Makefile).


> > Why do we need a datetime getter for GstStructure?
> 
> Added those to be used on datetime serialization/deserialization tests (in
> gstvalue tests).
> 
> All other tests uses the GstStructure functions, including GstDate tests.
> 
> But I guess I can update the tests to use the GstTagList functions to get them.
> Is this preferable?

Ah, I didn't realize we had a gst_structure_get_date(), even though it was me who added it :-). I'm not sure what that's good for, but I guess we may just as well add a _get_date_time() for consistency and ease of use in unit tests then.


> >  - why do we have gst_date_time_get_dmy(), but
> >    for the time fields it's _get_hour(), get_minute()
> >    etc.? Instead of one function?
> 
> Because for days you need to get the month first, and for the month, you'd need
> the year. Getting all 3 at once is faster than getting one by one. For the time
> functions this doesn't happen.

Ah, I see. That makes sense, but sounds very implementation-driven. The question is what is the better/nicer/more minimal API IMHO.

We could just have a _get_dmy_and_time() for example, and people can then just pass NULL for stuff they're not interested in. Since they need to do a _get_foo() for the date anyway, that doesn't seem particularly inconvenient (if it was all _get_day() etc. then one could argue that it would be convenient in print functions etc. if you can just use the getters there directly, but that's not an option anyway now).

I can live with _get_{hour,minute,second,etc.}() though (in case we do go with this, I wonder if it should be plural, ie. get_hours(), get_minutes(), get_seconds()?).

---

Next: how about:

  gst_date_time_new_now() -> gst_date_time_new_now_local_time()

and

  gst_date_time_new_utc_now() -> gst_date_time_new_now_utc()

As for the order, I'm not sure which one sounds nicer, best to ask a native speaker probably. I think it'd be good to make it explicit in _new_now() that it is local time, so people using this API get what they want.

---

How about:

    gst_date_time_get_utc_offset() -> gst_date_time_get_time_zone_offset() ?

Too long?

----

gst_date_time_new() + _getters: why do we use gint here when the values are alwyas unsigned? To allow passing -1 in future in case we change our minds and want to allow some fields to remain unset (e.g. for less accuracy?)

And I still think that it would be more natural to specify the tz_offset to gst_date_time_new() in hours as a gdouble rather than in minutes. 1/2 and 1/4 can be perfectly expressed in floats, so I don't think there'll be any rounding issues.

---

Another thing: what about accuracy? Do we want that/need that? (I somewhat do)

---

The gstdatetime.h file should have a gtk-doc blurb for GstDateTime telling people that it's opaque etc., plus since marker.
Comment 49 Thiago Sousa Santos 2010-07-21 21:49:56 UTC
Created attachment 166334 [details] [review]
gstdatetime: Adds GstDateTime

Updated.
Comment 50 Thiago Sousa Santos 2010-07-21 21:50:19 UTC
Created attachment 166335 [details] [review]
taglist: Add datetime get functions

Updated.
Comment 51 Thiago Sousa Santos 2010-07-21 21:50:44 UTC
Created attachment 166336 [details] [review]
tag: Adds GST_TAG_DATE_TIME tag

Updated.
Comment 52 Thiago Sousa Santos 2010-07-21 21:51:00 UTC
Created attachment 166337 [details] [review]
gststructure: Adds datetime getter function

Updated.
Comment 53 Thiago Sousa Santos 2010-07-21 21:51:18 UTC
Created attachment 166338 [details] [review]
gstvalue: Adds tests for datetime

Updated.
Comment 54 Thiago Sousa Santos 2010-07-21 21:55:12 UTC
(In reply to comment #48)
> > Added its prototype to gst_private.h and it is now private.
> 
> Make sure to prefix it with priv_ or remove the _*gst prefix, otherwise it'll
> still get exported (see: grep GST_LIB_LDFLAGS Makefile).

Fixed.

> 
> 
> > > Why do we need a datetime getter for GstStructure?
> > 
> > Added those to be used on datetime serialization/deserialization tests (in
> > gstvalue tests).
> > 
> > All other tests uses the GstStructure functions, including GstDate tests.
> > 
> > But I guess I can update the tests to use the GstTagList functions to get them.
> > Is this preferable?
> 
> Ah, I didn't realize we had a gst_structure_get_date(), even though it was me
> who added it :-). I'm not sure what that's good for, but I guess we may just as
> well add a _get_date_time() for consistency and ease of use in unit tests then.

Ok.

> 
> 
> > >  - why do we have gst_date_time_get_dmy(), but
> > >    for the time fields it's _get_hour(), get_minute()
> > >    etc.? Instead of one function?
> > 
> > Because for days you need to get the month first, and for the month, you'd need
> > the year. Getting all 3 at once is faster than getting one by one. For the time
> > functions this doesn't happen.
> 
> Ah, I see. That makes sense, but sounds very implementation-driven. The
> question is what is the better/nicer/more minimal API IMHO.
> 
> We could just have a _get_dmy_and_time() for example, and people can then just
> pass NULL for stuff they're not interested in. Since they need to do a
> _get_foo() for the date anyway, that doesn't seem particularly inconvenient (if
> it was all _get_day() etc. then one could argue that it would be convenient in
> print functions etc. if you can just use the getters there directly, but that's
> not an option anyway now).
> 
> I can live with _get_{hour,minute,second,etc.}() though (in case we do go with
> this, I wonder if it should be plural, ie. get_hours(), get_minutes(),
> get_seconds()?).

Changed internal representation to separate fields and added a get_ for each of them fields, removing the get_dmy. 

Kept the time functions at singular, a think both make sense and have no strong opinion.

> 
> ---
> 
> Next: how about:
> 
>   gst_date_time_new_now() -> gst_date_time_new_now_local_time()
> 
> and
> 
>   gst_date_time_new_utc_now() -> gst_date_time_new_now_utc()
> 
> As for the order, I'm not sure which one sounds nicer, best to ask a native
> speaker probably. I think it'd be good to make it explicit in _new_now() that
> it is local time, so people using this API get what they want.

Changed to your suggestions.

> 
> ---
> 
> How about:
> 
>     gst_date_time_get_utc_offset() -> gst_date_time_get_time_zone_offset() ?
> 
> Too long?

Done.

> 
> ----
> 
> gst_date_time_new() + _getters: why do we use gint here when the values are
> alwyas unsigned? To allow passing -1 in future in case we change our minds and
> want to allow some fields to remain unset (e.g. for less accuracy?)

Exactly.

> 
> And I still think that it would be more natural to specify the tz_offset to
> gst_date_time_new() in hours as a gdouble rather than in minutes. 1/2 and 1/4
> can be perfectly expressed in floats, so I don't think there'll be any rounding
> issues.
> 

Ok. Changed to float.

> ---
> 
> Another thing: what about accuracy? Do we want that/need that? (I somewhat do)

XMP uses it too, so I'll need it at some point.

> 
> ---
> 
> The gstdatetime.h file should have a gtk-doc blurb for GstDateTime telling
> people that it's opaque etc., plus since marker.

Done.
Comment 55 Tim-Philipp Müller 2010-07-21 22:25:28 UTC
Comment on attachment 166335 [details] [review]
taglist: Add datetime get functions


>+/* This is only meant for internal uses */
>+gint _gst_date_time_compare (gconstpointer dt1, gconstpointer dt2);
>+
...
>+/**
>+ * gst_date_time_compare:
....
>+ */
>+gint
>+_gst_date_time_compare (gconstpointer dt1, gconstpointer dt2)
>+{
...
>--- a/win32/common/libgstreamer.def
>+++ b/win32/common/libgstreamer.def
>@@ -35,6 +35,7 @@ EXPORTS
> 	__gst_debug_min DATA
> 	_gst_alloc_trace_register
> 	_gst_buffer_list_initialize
>+	_gst_date_time_compare
> 	_gst_debug_bin_to_dot_file
> 	_gst_debug_bin_to_dot_file_with_ts
> 	_gst_debug_category_new

This function is still exported as a symbol. See my comments above, you need to prefix it with something like 'priv' or 'non' or so. Also, there shuoldn't be a gtk-doc chunk for it.
Comment 56 Tim-Philipp Müller 2010-07-21 22:30:55 UTC
Comment on attachment 166336 [details] [review]
tag: Adds GST_TAG_DATE_TIME tag

This looks good to me.
Comment 57 Tim-Philipp Müller 2010-07-21 22:32:27 UTC
Comment on attachment 166337 [details] [review]
gststructure: Adds datetime getter function

Looks ok to me.
Comment 58 Tim-Philipp Müller 2010-07-21 22:48:27 UTC
Comment on attachment 166334 [details] [review]
gstdatetime: Adds GstDateTime

This works for me. Might want to wait for a bit longer to see if there are more comments though.
Comment 59 Tim-Philipp Müller 2010-07-21 22:54:30 UTC
Comment on attachment 166335 [details] [review]
taglist: Add datetime get functions

>Subject: [PATCH] taglist: Add datetime get functions
>
>Adds _date_time_get and _date_time_get_index functions to
>taglist.
>
>API: gst_tag_list_get_date_time
>API: gst_tag_list_get_date_time_index
>
>Fixes #594504
>---
> docs/gst/gstreamer-sections.txt |    7 +++
> gst/gst_private.h               |    3 +
> gst/gstdatetime.c               |   42 ++++++++++++++++
> gst/gststructure.c              |    2 +
> gst/gsttaglist.c                |   62 +++++++++++++++++++++++
> gst/gsttaglist.h                |    8 +++
> gst/gstvalue.c                  |  103 +++++++++++++++++++++++++++++++++++++++
> gst/gstvalue.h                  |   22 ++++++++
> win32/common/libgstreamer.def   |    4 ++
> 9 files changed, 253 insertions(+), 0 deletions(-)

Further to the above, the commit message doesn't seem entirely accurate, it does a bit more than just add taglist getters, namely set up all the gstvalue stuff / serialisation / deserialisation / get_type func etc. (maybe this can or should be split?)
Comment 60 Thiago Sousa Santos 2010-07-22 01:35:27 UTC
Created attachment 166347 [details] [review]
gstvalue: Adds datetime functions

Somehow managed to merge these patches together, splitting them up.
Comment 61 Thiago Sousa Santos 2010-07-22 01:35:48 UTC
Created attachment 166348 [details] [review]
taglist: Add datetime get functions

And the other one.
Comment 62 Tim-Philipp Müller 2010-07-22 10:35:32 UTC
Comment on attachment 166347 [details] [review]
gstvalue: Adds datetime functions

Looks good to me
Comment 63 Sebastian Dröge (slomo) 2010-07-25 14:53:51 UTC
Some minor comments:
- add configure checks for TM_GMTOFF, GMTIME_R
- maybe add setter functions? or say it's immutable in the docs
- use check macros instead of GLib check macros in tests (not g_assert_cmpint)
- patches 2 and 3 uses GST_TYPE_DATE_TIME from patch 5, reorder them
- maybe a gst_structure_set_date_time() with dmy, etc parameters?
- #undef GST_DATE_TIME_COMPARE_VALUE after usage
- gst_tag_list_get_date_time() doesn't check if its a GstDateTime
Comment 64 Thiago Sousa Santos 2010-07-26 15:25:05 UTC
commit d40776411041efd474e0c67a7959a4cfc6d5f156
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Wed Jun 23 11:31:33 2010 -0300

    gstvalue: Adds tests for datetime
    
    Adds tests for datetime fields in gstvalue tests
    
    Fixes #594504

commit f2c18c6c98b8e22ca7de43f76fea37f847e4b0e6
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Wed Jun 23 11:30:02 2010 -0300

    gststructure: Adds datetime getter function
    
    Adds gst_structure_get_date_time function
    
    API: gst_structure_get_date_time
    
    Fixes #594504

commit 137d19d621a4b5ada4bfc2c5a31701a3cded1c14
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Mon Jun 21 23:42:44 2010 -0300

    tag: Adds GST_TAG_DATE_TIME tag
    
    Adds a new tag that represents a date and time a media was
    created
    
    API: GST_TAG_DATE_TIME
    
    Fixes #594504

commit 3449bfc30ebf78ac0e2a4a10455ccd2d9d9a5067
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Wed Jul 21 22:08:21 2010 -0300

    taglist: Add datetime get functions
    
    Adds _date_time_get and _date_time_get_index functions to
    taglist.
    
    API: gst_tag_list_get_date_time
    API: gst_tag_list_get_date_time_index
    
    Fixes #594504

commit b4870282cb93f0fdf8e3fc22526e8819faa33c0c
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Wed Jul 21 22:04:23 2010 -0300

    gstvalue: Adds datetime functions
    
    Adds a datetime functions to gstvalue
    
    Fixes #594504

commit 6425bde6ece6e56af7dff6932fcb4159931bc1f5
Author: Thiago Santos <thiago.sousa.santos@collabora.co.uk>
Date:   Fri Jul 16 14:09:12 2010 -0300

    gstdatetime: Adds GstDateTime
    
    Adds GstDateTime to represent dates + time + timezone
    information.
    
    Tests included.
    
    API: GstDateTime
    API: gst_date_time_get_day
    API: gst_date_time_get_month
    API: gst_date_time_get_year
    API: gst_date_time_get_hour
    API: gst_date_time_get_microsecond
    API: gst_date_time_get_minute
    API: gst_date_time_get_second
    API: gst_date_time_get_time_zone_offset
    API: gst_date_time_new
    API: gst_date_time_new_local_time
    API: gst_date_time_new_from_unix_epoch
    API: gst_date_time_new_now_local_time
    API: gst_date_time_new_now_utc
    API: gst_date_time_ref
    API: gst_date_time_unref
    
    Fixes #594504
Comment 65 Thiago Sousa Santos 2010-11-16 17:27:33 UTC
*** Bug 634999 has been marked as a duplicate of this bug. ***