Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(server): prefer explicit timezone over GPS based guess #12707

Closed
wants to merge 1 commit into from

Conversation

C-Otto
Copy link
Contributor

@C-Otto C-Otto commented Sep 15, 2024

exiftool-vendored sadly treats the offset +00:00 (used in several countries) as unknown and may return a timezone which is based on GPS coordinates. Because of this, when one manually fixes the timezone offset to +00:00, this fix is overwritten. With this fix, we explicitly prefer +00:00 (if set).

To test: Have an asset with GPS coordinates that exiftool-vendored maps to a timezone (you can check the exif database table, column timeZone). For this asset, manually change the date to use an offset of +00:00 (e.g. Atlantic/Reykjavik). The XMP files correctly includes +00:00. Without this fix, the database still contains the old (wrong?) timezone. With this fix, the timezone is UTC+0.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides the linting issue ;)

@C-Otto
Copy link
Contributor Author

C-Otto commented Sep 15, 2024

I added // eslint-disable-next-line unicorn/prefer-ternary

@danieldietzler
Copy link
Member

I added // eslint-disable-next-line unicorn/prefer-ternary

Why not use a ternary?

exiftool-vendored sadly treats the offset +00:00 (used in several countries) as unknown
and may return a timezone which is based on GPS coordinates. Because of this, when
one manually fixes the timezone offset to +00:00, this fix is overwritten. With this fix,
we explicitly prefer +00:00 (if set).
@C-Otto
Copy link
Contributor Author

C-Otto commented Sep 15, 2024

Why not use a ternary?

Good question. I'm not used to it, but it doesn't look too bad.

@jrasm91
Copy link
Contributor

jrasm91 commented Sep 18, 2024

Why would the gps timezone ever be wrong?

@C-Otto
Copy link
Contributor Author

C-Otto commented Sep 19, 2024

The GPS location (long/lat) is used to derive the timezone. Based on my experience, this can be wrong in at least two scenarios.

  • the phone didn't update the GPS location, yet (after travelling to a different timezone), so that a previously determined GPS location is used
  • there are two (or more) timezones close to the GPS location, and exiftool-vendored guesses the wrong one

In both cases there's no way to override/correct this guess, which is rather annoying. One could tweak the location, but that's rather unintuitive.

@jrasm91
Copy link
Contributor

jrasm91 commented Sep 19, 2024

This PR doesn't address those situations at all. The change is a hard coded work around for the +00:00 offset itself. Would it not make more sense to address the actual issue directly instead?

We seem to have two options:

  • "if xmp has DateTimeOriginal with timezone offset, ignore gps inferred timezone" - specifically set by immich in xmp
  • "always use the timezone associated with dateTime and use exif.tz as a fallback - handle this case in original metadata for a bigger variety of tags, but keep the fallback since it is a good supplement with no other information is available

@C-Otto
Copy link
Contributor Author

C-Otto commented Sep 19, 2024

The behavior you want is already what happens for offsets other than +00:00.

@jrasm91
Copy link
Contributor

jrasm91 commented Sep 19, 2024

The behavior you want is already what happens for offsets other than +00:00.

So then this is more of an exiftool-vendored bug than anything. Let's just wait for it to be fixed upstream in that case.

@C-Otto
Copy link
Contributor Author

C-Otto commented Sep 20, 2024

Yeah, we could do that. However, I haven't received any response in the past three weeks.

@C-Otto C-Otto closed this Oct 7, 2024
@C-Otto C-Otto deleted the timezone-fix branch October 7, 2024 18:11
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants