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

Add support for coordinates in Adobe format in XMP tags #653

Merged
merged 12 commits into from
Sep 6, 2023

Conversation

malconsei
Copy link
Contributor

  • exif parser: Add support for Adobe coordinates
  • exiftool parser: Try to get coordinates in the XMP-exif namespace, too

Copy link
Member

@ptpt ptpt left a comment

Choose a reason for hiding this comment

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

These two cases are doesn't share much in common. It feels easier to understand if you separate them. How about this:

def _extract_lon_lat_numeric():
    # the numeric case
    pass

def _extract_lon_lat_adobe():
    # the adobe case
    pass

def extract_lon_lat():
    lon_lat = self._extract_lon_lat_numeric()
    if lon_lat is not None:
        return lon_lat

    lon_lat = self. _extract_lon_lat_adobe()
    if lon_lat is not None:
        return lon_lat

    return None

mapillary_tools/exif_read.py Outdated Show resolved Hide resolved
Copy link
Member

@ptpt ptpt left a comment

Choose a reason for hiding this comment

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

Accept to unblock.

if lon is None:
return None

ref = self._extract_alternative_fields(["exif:GPSLongitudeRef"], str)
if ref and ref.upper() == "W":
if ref and ref.upper() == "W" and lon > 0:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to apply the reference for the adobe value again? I thought adobe has NSEW parsed from its own format so it doesn't have to respect this one here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, the Adobe coords already have the right sign, that's why I added the check on lon/lat > 0 - to avoid multiplying them by -1 again. But yeah, it's hacky, I'll make it more explicit :)

Copy link
Member

Choose a reason for hiding this comment

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

Why not separating them? So adoble values don't need to respect the ref tags. See my proposal above.

@malconsei malconsei merged commit 8ce64fb into main Sep 6, 2023
16 checks passed
@malconsei malconsei deleted the fix-adobe-coords branch September 6, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants