Date handling when simplifying date strings without a timezone seems incorrect

bwindels / exif-parser

A javascript library to extract EXIF metadata from JPEG images, in node and in the browser.

MIT License

219 stars 53 forks source link

Date handling when simplifying date strings without a timezone seems incorrect #14

Open dremonkey opened 8 years ago

dremonkey commented 8 years ago

First off... thanks for the library. Of the ones that I have seen it is by far the most complete.

However, it seems like your logic when simplifying a date string may be wrong. As far as I can tell you are treating a date string without the an explicit timezone as UTC time when according to ISO standards it should be treated as local time 1.

If you agree that this is a problem I can open a pull request with a fix...

dremonkey commented 8 years ago

Actually after giving this a bit more thought... my suggestion would be to remove the simplify option completely and focus only on parsing. Let a more complete library like moment.js handle an transformations on the data. Thoughts?

rprieto commented 7 years ago

Hi, I second that: thanks for this awesome EXIF parser! I think I have similar issues with DateTimeOriginal and timezones. For a concrete example, I have a photo called 2017-01-04 11.29.12.jpg (Dropbox naming convention, I did take it at 11:29am). If I call exiftool directly, it confirms:

Date/Time Original: 2017:01:04 11:29:12   # matches the file name

When I use exif-parser, I get:

exifparser.create(fileContents).parse().tags.DateTimeOriginal)
// returns 1483529352

new Date(1483529352 * 1000)
// Wed Jan 04 2017 22:29:12 GMT+1100 (AEDT)

So it looks like it assumed GMT and added 11 hours for AEDT which is my local timezone. I started noticing because I just added the "create albums by date" functionality to thumbsup (which uses exif-parser as the core parser), and noticed photos from New Years Eve appearing in the "January" album 😄).

bwindels commented 6 years ago

Wow, this is an old one. Sorry I missed this for so long. Great catch about the default time zone. Also the parsing for explicit time zones is quite incomplete. Briefly looking at the spec; optional colons between hours and minutes of the offset are not handled and the Z postfix for UTC is not handled at all.

I would not remove simplifying EXIF values completely, as a) you can disable it b) numeric EXIF values are expressed in unique ways (fractional, degrees) that make little sense to consume as is. c) I don't want to require a third-party library like moment.js just to parse a date

Splitting up in two libraries would be too much overhead as the release process is already somewhat cumbersome.

I would be very happy with a PR for fixing the default time zone to local time, or a setting to just disable simplifying dates :)

bwindels commented 6 years ago

Could use this for the parsing: https://www.npmjs.com/package/iso8601-convert

edemaine commented 6 years ago

Just coming to this package for the first time, and ran into this issue. I assume it remains unresolved?

A few notes from the EXIF 2.2 spec:

DateTimes have the format YYYY:MM:DD HH:MM:SS of exactly 19 characters (plus null terminator). The colons are not optional, and timezones cannot be specified.
Each Y/M/D/H/M/S character can instead be a space. That would wreck havoc with the split(' ') currently in the code. (I've never seen this in practice, though, and the regex in another package https://www.npmjs.com/package/exif-date is similarly broken.)

I'm not sure it would be great to return the local time, when this library can be used server side and the client's notion of timezone may differ. I can see a couple of options:

Leave the date as an EXIF string. Not ideal, but then the user can do what they want.
Change the date into an ISO8601 string, so it's easy to parse via e.g. moment (by default with local timezone, or you could easy add a timezone offset to the end)

Personally I think Option 2 is a bit better. Thoughts?

bwindels commented 6 years ago

Yeah, parsing dates with a timezone needs to be improved. I'd rather do it with a very light dependency as mentioned before, or none at all. moment.js is 16kb minified, too big to justify for this problem IMO.

Apart from that, one thing mentioned in earlier comments is that some tools assume that a date without a timezone is assumed to be in the local timezone. An option to set the default timezone could make sense. I'll do it when I get to it, unless anyone beats to me making a PR :)

lqzhgood commented 4 years ago

The time zone is wrong

bwindels commented 4 years ago

The time zone is wrong

Care to elaborate? Can you share the date string in the file you see the issue with?

lqzhgood commented 4 years ago

The time zone is wrong

Care to elaborate? Can you share the date string in the file you see the issue with?

like https://github.com/bwindels/exif-parser/issues/14#issuecomment-275368437

I test .Jpg Exif is local time zone not 0 time zone(GMT). --> exif.DateTimeOriginal

Relevant links found https://exiftool.org/forum/index.php?topic=1886.0 https://community.adobe.com/t5/lightroom-classic/how-to-change-the-timezone-in-photo-metadata/td-p/6429833?page=1

by the way

I test .mov is 0 time zone(GMT) not local time zone~ -->exif.createDate Exif specification It's too messy .....