Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Take daylight savings into account for Nikon's WorldTime TimeZone tag #215

Closed
C-Otto opened this issue Oct 4, 2024 · 3 comments
Closed

Comments

@C-Otto
Copy link

C-Otto commented Oct 4, 2024

Describe the bug
Nikon uses a proprietary TimeZone tag in the WorldZone Subdirectory. This timezone is taken literally. The information in TimeZone needs to be adjusted by one hour if DaylightSavings is also set in WorldTime.

To Reproduce / Expected behavior
Example images in immich-app/immich#13141:

immich-app/immich#13141 (comment) should have offset +13:00 (with daylight savings), but +12:00 is returned

  | | | 2)  WorldTime (SubDirectory) -->
  | | | + [BinaryData directory, 4 bytes]
  | | | | TimeZone = 720
  | | | | DaylightSavings = 1

immich-app/immich#13141 (comment) should have offset +08:00 (without daylight savings) (which is working as intended)

  | | | 2)  WorldTime (SubDirectory) -->
  | | | + [BinaryData directory, 4 bytes]
  | | | | TimeZone = 480
  | | | | DaylightSavings = 0

Environment (please complete the following information):

  • Version of this library: v28.3.1
  • OS and version: Linux, Debian Stable
  • Node.js version: v20.17.0
@mceachen
Copy link
Member

mceachen commented Oct 4, 2024

Thanks for taking the time to dig into this, @C-Otto !

This is a mess.

I verified with several Canon, Fuji, and Olympus cameras, and it appears that everyone apart from Nikon follows these semantics:

  • TimeZone is the offset to UTC. Period. No other heuristics need to be in play.

  • The DaylightSavings tag is purely additional information: that the offset may be including an additional hour offset--not that the application needs to sometimes add an hour or remove an hour from the given TimeZone.

At least for a couple Nikon camera models, it seems that I'm supposed to add an offset to TimeZone, but only if exiftool returns Yes for DaylightSavings (which I should probably convert to a boolean?)

That said, every camera manufacturer runs roughshod over the metadata specs, and Nikon may be playing shenanigans here with only a few of their models.

The timezone heuristics in this library are already terribly convoluted, but I've justified those (mostly) as they are addressing fairly common metadata situations.

I will add an option that is a predicate to determine if we need to respect the DaylightSavings tag or not: something like

  /**
   * The TimeZone tag normally represents the offset from UTC.
   *
   * Unfortunately, at least for some Nikon cameras, the TimeZone tag **and the
   * DaylightSavings tag** must be taken into account to find the UTC offset.
   *
   * By default, this is a predicate that returns `true` if the `Make` tag is
   * `Nikon`. If you find other makes and models that need this treatment,
   * please open a ticket on GitHub with example images or videos and we can
   * update the default predicate.
   * 
   * The return value is the number of minutes to adjust the timezone by.
   *
   * @see https://github.com/photostructure/exiftool-vendored.js/issues/215
   */
  adjustTimeZoneIfDaylightSavings: (tags: Tags, tz: string) => Maybe<number>

Here is the default implementation:

  adjustTimeZoneIfDaylightSavings: (t: Tags) => {
    // `DaylightSavings` may be "Yes" or `true`:
    return true === toBoolean(t.DaylightSavings) &&
      // Daggum Nikon likes "FS-Nikon", "Nikon", "NIKON", and "NIKON CORPORATION"
      /\bnikon\b/i.test(String(t.Make))
      ? 60
      : undefined
  },

Do you think this will suffice? (If we always add 60 minutes to the offset if DaylightSavings this could just return a boolean, I guess?)

@mceachen
Copy link
Member

mceachen commented Oct 4, 2024

Here's the implementation:

export function incrementZone(
  z: string | Zone | number,
  minutes: number
): Maybe<Zone> {
  const norm = normalizeZone(z) as any
  if (true !== norm?.isUniversal) return

  // hack to reach into the FixedOffsetZone, as the fixed value isn't
  // (currently) exported in the Typescript typings:
  // See https://github.com/moment/luxon/issues/1661
  const fixed = norm?.["fixed"]
  return isNumber(fixed) ? FixedOffsetZone.instance(fixed + minutes) : undefined
}

export function extractTzOffsetFromTags(
  t: Tags,
  opts?: Pick<ExifToolOptions, "adjustTimeZoneIfDaylightSavings">
): Maybe<TzSrc> {
  // We have to iterate twice: if it's from a timezone offset tag, we can
  // trust it, even if it's UTC.
  for (const tagName of TimezoneOffsetTagnames) {
    if (t[tagName] != null) {
      const offset = extractZone(t[tagName])
      if (offset != null) {
        const minutes = opts?.adjustTimeZoneIfDaylightSavings?.(t, offset.tz)
        if (minutes != null) {
          const adjustedZone = incrementZone(offset.tz, minutes)
          if (adjustedZone != null)
            return { tz: adjustedZone.name, src: tagName }
        }
        return { tz: offset.tz, src: tagName }
      }
    }
  }
  return
}

@C-Otto
Copy link
Author

C-Otto commented Oct 5, 2024

It's always 60 minutes, as far as I'm aware, yes. Ultimately it's your library and your decision, but I'd appreciate this hack so that Nikon users just get something that works out of the box. I don't own a Nikon camera nor know a lot about this, so please be aware that this fix might need further adjustments in the future. I'm also fine with telling users that any wrong offset information is Nikon's fault, though (side node: iPhones sometimes seem to provide -07:59 as an offset, and I don't think this should be fixed in code).

PS: Maybe it suffices to change the order of properties that are used to determine the offset (i.e. pushing "TimeZone" down in priority)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants