Reading GPS info fails if GPSAltitude is not set

[jonas-hagen]

The function gexiv2_metadata_get_gps_info returns false if not all three coordinates (lat, lon, altitude) are set. This is a minor problem for gexiv2 because the gexiv2_metadata_get_gps_longitude/latitude/altitude functions exist separately. Of course, the tags can be accessed by name directly, but there are some important transformations done (see this comment in gexiv2). So it is currently not possible to get reliable GPS information from rexiv2 directly if the altitude tag is not set.

It is quite common that the altitude is unset, the examples/example.jpg does not have this tag for example (so the example code fails).

Possible solutions:

1. Fix the example and show how to get latitude and longitude and do the necessary transformations (no API change).
2. Expose the gexiv2_metadata_get_gps_longitude/latitude/altitude functions (no breaking change).
3. Change the GpsInfo struct (most idiomatic).
   Suggestion:

pub struct GpsInfo {
    pub longitude: f64,
    pub latitude: f64,
    pub altitude: Option<f64>,
}

[felixc]

Hi, and thanks for the report!

I'm not sure I'm understanding the issue correctly, though. It looks to me like gexiv2_metadata_get_gps_info in the code you linked to returns FALSE if all three of (lat, lon, alt) are unset... but it should return TRUE if any of them are set.

If any are unset, those specific fields are set to the value 0.0, but the other fields should be populated and the overall result should be TRUE, no?

It looks like this changed some time ago, according to https://gitlab.gnome.org/GNOME/gexiv2/-/commit/058abb2a530ebc465ea994f144f0cf682c2ffc51 the old behaviour was as you described (any unset value causes the whole thing to return FALSE) — are you perhaps seeing this behaviour in an older version of libgexiv2?

I definitely agree with your point about Optional values being more idiomatic, though, and we could make that change. Do you think only altitude needs that (definitely the most common case) or should we do it for all three fields since all of them can be optional?

Looks like we'd have to check for equality with 0.0 to "detect" a None value... I guess that's extremely unlikely to be real valid value, so... a good enough sentinel value?

[jonas-hagen]

Ah, good catch! I looked at two different versions of the code and didn't even realize they are different. Apparently this behavior changed (again) from 0.12.1 to 0.12.2 (which now ships with Arch).

So, the current version of gexiv2 is 0.12.2 and gexiv2_metadata_get_gps_info returns false if any component is unset.

Do you think only altitude needs that (definitely the most common case) or should we do it for all three fields since all of them can be optional?

My opinion: The common use case for this data is to draw it on a map or resolve a location name. Both can be done with latitude and longitude, without altitude. Thus, I would argue that latitude and longitude together make a valid location info. If one of them is not present, the location is not known and thus None. Altitude is optional. An API user would then implement something like: If GPSInfo is Some, draw a point onto the map. Anything more sophisticated would require the three separate functions.

But really, anything goes as long as images do not start to land on Null Island.

[jonas-hagen]

I prepared a PR to make altitude optional in GpsInfo. The following problem occurs:

set_gps_info relies on GpsInfo having all the fields set and calls gexiv2_metadata_set_gps_info, which expects latitude, longitude and altitude. If altitude is optional, what should be passed here in case altitude is None? Sadly, gexiv2 seems not to expose any methods to set latitude and longitude separately. Not even functions to set a tag with tree rationals as value, if I understand correctly?

I see the following possibilities:

1. Let GpsInfo be as is. It will then serve primarily as the type-of-the-argument to call set_gps_info. For reading GPS information, the separate functions get_gps_{latitude,longitude,altitude} shall be used. As in Expose functions to get GPS latitude, longitude and altitude separately #43.
2. Change GpsInfo as proposed here. It will then serve as a convenient way to read GPS information in any case. For setting GPS information, the three coordinates have to be known and provided explicitly as f64s to set_gps_info. As in Make altitude optional in GPS info #44.

[Dalan94]

Can you merge one of the pull request ?
I add GPS information on my photo manually using darktable. And it doesn’t set the altitude (and I don’t care about it).
So I cannot get the GPS information using rexvi2 whereas I want them to get the location on a map.

[felixc]

I'm looking at how to work around this, but in the meantime I've also filed a bug upstream since I think the change in 0.12.2 may possibly have been unintentional: https://gitlab.gnome.org/GNOME/gexiv2/-/issues/72

Thank you for the detective work to narrow it down to the change between 0.12.1 and 0.12.2. I'm on Debian Stable which has 0.12.1, so I couldn't reproduce any variant of this, but now I understand the problem.

[felixc]

I've got a third possible patch for this over at #69. It follows the same basic idea as your change in #44, making altitude Optional inside GpsInfo. However it has a slightly different implementation that I think is more robust to different versions of the library, doesn't require separate lat/long/alt function calls, and keeps the interface more similar for the set_gps_info function. The downside of it is that it does kind of rely on an opaque implementation detail of the order in which upstream gexiv2 happens to perform the lookups...

What do you think, is that too risky?

Does the patch work for your files that were exhibiting this problem?

[Dalan94]

Your patch work well with my files 😀
I think your method is OK because the gexiv2 implementation shouldn't change (and the change in 0.12.2 is probably a bug).

[felixc]

Thanks for testing! I've merged #69, but will leave the bug open for now until the fix is out in a published version. If anyone feels we shouldn't use the approach from that patch, now would be the best time to mention that, before it's in a release :)