From e8ceff8ec43a04e3c2da81a882f6d07ab1121010 Mon Sep 17 00:00:00 2001 From: Felix Crux Date: Sun, 22 Jan 2023 22:09:05 -0500 Subject: [PATCH] Make altitude component of GpsInfo optional This matches the real-world behaviour of apps that only set lat/long when drawing points on a map. What made this tricky is that up until gexiv2 0.12.2, this was not necessary. If altitude (or indeed latitude or longitude) were missing, the function would still return successfully overall as long as at least one piece of data could be found. However in 0.12.2 this changes so that *all three* components need to be present for `gexiv2_metadat_get_gps_info` to return `TRUE`. I am not sure if this change was intentional, so I have filed a bug upstream with gexiv2, but in the meantime we still need a workaround. Bug report: https://gitlab.gnome.org/GNOME/gexiv2/-/issues/72 Debian Stable as of today ships with 0.12.1, so I couldn't reproduce or test this locally, but the CircleCI Mac OSX executor is installing a recent version of the library via homebrew, so if the tests pass there it should be good. This patch works around the problem by making the altitude (and only altitude) component of `GpsInfo` `Optional`. This is slightly annoyingly asymmetrical, but makes sense in reality as there's no reason to have, say, a latitude and an altitude but not a longitude. The workaround relies on the implementation detail that gexiv2 upstream happens to check and set latitude and longitude first, and altitude last. That means that even if we get a `FALSE` return, the latitude and longitude pointers may nonetheless have been populated, if the problem was simply that altitude was unset. For compatibility with various versions of gexiv2, we handle both cases: the newer behaviour that returns `FALSE`, and also the older behaviour that returns `TRUE` but has altitude set to `0.0`. Addresses bug report #42 - https://github.com/felixc/rexiv2/issues/42 References: - 0.12.1 version of the code: https://gitlab.gnome.org/GNOME/gexiv2/blob/gexiv2-0.12.1/gexiv2/gexiv2-metadata-gps.cpp#L186 - 0.12.2 version: https://gitlab.gnome.org/GNOME/gexiv2/blob/gexiv2-0.12.2/gexiv2/gexiv2-metadata-gps.cpp#L246 - Commit that made the change: https://gitlab.gnome.org/GNOME/gexiv2/-/commit/f2d96b4df221d922ededfbff9236636273a56029 --- CHANGELOG.md | 6 ++++++ src/lib.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d533a9..1825cde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ SPDX-License-Identifier: CC0-1.0 ## [NEXT] - Unreleased * Bugfix: Stop reducing/simplifying rational values like apertures. + * Breaking API change/bugfix: The altitude part of GpsInfo is now Optional. + This works around an upstream change that caused `get_gps_info` to return + `None` if altitude was unset, even if lat/long were present. Now lat/long + will be returned, but altitude will be `None`. Altitudes that may have been + previously shown as `0.0` will now be `None`. Calls to `set_gps_info` will + also need updating. Thank you Jonas Hagen for the report and investigation. ## [v0.10.0] - 2023-01-21 * New API: `new_from_app1_segment` allows reading metadata from a buffer. diff --git a/src/lib.rs b/src/lib.rs index e330bac..365cb2b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -121,7 +121,7 @@ pub struct PreviewImage<'a> { pub struct GpsInfo { pub longitude: f64, pub latitude: f64, - pub altitude: f64, + pub altitude: Option, } /// The possible data types that a tag can have. @@ -1129,24 +1129,68 @@ impl Metadata { // GPS-related methods. /// Retrieve the stored GPS information from the loaded file. + /// + /// # Examples + /// ``` + /// # let minipng = [137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82, 0, 0, 0, + /// # 1, 0, 0, 0, 1, 8, 0, 0, 0, 0, 58, 126, 155, 85, 0, 0, 0, 10, 73, 68, 65, + /// # 84, 8, 215, 99, 248, 15, 0, 1, 1, 1, 0, 27, 182, 238, 86, 0, 0, 0, 0, 73, + /// # 69, 78, 68, 174, 66, 96, 130]; + /// # let meta = rexiv2::Metadata::new_from_buffer(&minipng).unwrap(); + /// meta.set_gps_info(&rexiv2::GpsInfo { longitude: 0.2, latitude: 0.3, altitude: Some(0.4) }); + /// assert_eq!( + /// meta.get_gps_info(), + /// Some(rexiv2::GpsInfo { longitude: 0.2, latitude: 0.3, altitude: Some(0.4) }), + /// ); + /// # Ok::<(), Box>(()) + /// ``` pub fn get_gps_info(&self) -> Option { let lon = &mut 0.0; let lat = &mut 0.0; let alt = &mut 0.0; match unsafe { gexiv2::gexiv2_metadata_get_gps_info(self.raw, lon, lat, alt) } { - 0 => None, - _ => Some(GpsInfo { longitude: *lon, latitude: *lat, altitude: *alt }), + 0 => { + // If we get a "false" result, it might just be because the altitude is missing. + // This is the behaviour since gexiv2 0.12.2. In 0.12.1 and earlier, we'd still + // get a "true" result but altitude would be set to 0.0, which we handle below. + if *lon != 0.0 && *lat != 0.0 && *alt == 0.0 { + Some(GpsInfo { longitude: *lon, latitude: *lat, altitude: None }) + } else { + None + } + } + _ => Some(GpsInfo { + longitude: *lon, + latitude: *lat, + altitude: if *alt != 0.0 { Some(*alt) } else { None }, + }), } } /// Save the specified GPS values to the metadata. + /// + /// # Examples + /// ``` + /// # let minipng = [137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82, 0, 0, 0, + /// # 1, 0, 0, 0, 1, 8, 0, 0, 0, 0, 58, 126, 155, 85, 0, 0, 0, 10, 73, 68, 65, + /// # 84, 8, 215, 99, 248, 15, 0, 1, 1, 1, 0, 27, 182, 238, 86, 0, 0, 0, 0, 73, + /// # 69, 78, 68, 174, 66, 96, 130]; + /// # let meta = rexiv2::Metadata::new_from_buffer(&minipng).unwrap(); + /// assert_eq!(meta.get_gps_info(), None); + /// meta.set_gps_info(&rexiv2::GpsInfo { longitude: 0.8, latitude: 0.9, altitude: None }); + /// assert_eq!( + /// meta.get_gps_info(), + /// Some(rexiv2::GpsInfo { longitude: 0.8, latitude: 0.9, altitude: None }), + /// ); + /// # Ok::<(), Box>(()) + /// ``` pub fn set_gps_info(&self, gps: &GpsInfo) -> Result<()> { unsafe { int_bool_to_result(gexiv2::gexiv2_metadata_set_gps_info( self.raw, gps.longitude, gps.latitude, - gps.altitude, + gps.altitude.unwrap_or(0.0), )) } }