Skip to content

Conversation

@maxrjones
Copy link
Member

@maxrjones maxrjones commented Nov 6, 2025

This PR fixes the failure related to virtual-zarr/virtual-tiff#52.

I added instructions for downloading the file rather than subsetting and committing the file because gdal drops the problematic tag.

This looks to be a tag that was proposed as an addition to the GeoTIFF standard but never actually added to the spec - https://github.com/OSGeo/gdal/blob/657c5e4a4fb5b1389fd800ea9cc964435ad89a18/frmts/gtiff/libgeotiff/geokeys.inc#L34.

xref https://trac.osgeo.org/geotiff/wiki/TOWGS84GeoKey

@maxrjones maxrjones changed the title Add failing test for unknown tag Add support for GeogTOWGS84GeoKey Nov 7, 2025
@maxrjones maxrjones requested a review from kylebarron November 7, 2025 00:29
src/cog.rs Outdated
#[tokio::test]
async fn tmp_towgs84() {
let folder = "/Users/kyle/github/developmentseed/async-tiff";
let path = object_store::path::Path::parse("USGS_13_s14w171.tif").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to create like a one-pixel version of this file to check into the tests?

Maybe try

gdal_translate input.tif output_1px.tif -srcwin 0 0 1 1

to copy just the upper-left-most pixel of the file.

You'd want to manually check that the desired header still exists.

Copy link
Member Author

@maxrjones maxrjones Nov 7, 2025

Choose a reason for hiding this comment

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

It does not, I'd need to specifically compile GDAL to read and write that part of the header. The wheels on PyPI will drop the TAG during GDAL translate.

I can do that if you think it's necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test file in e4cecf5 (#131)

@maxrjones maxrjones self-assigned this Nov 10, 2025
GeogInvFlattening = 2059,
GeogAzimuthUnits = 2060,
GeogPrimeMeridianLong = 2061,
GeogToWGS84 = 2062,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GeogToWGS84 = 2062,
GeogTOWGS84GeoKey = 2062,

/// defined by its longitude relative to the international reference meridian (for the earth
/// this is Greenwich).
pub geog_prime_meridian_long: Option<f64>,
pub geog_to_wgs84: Option<Vec<f64>>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub geog_to_wgs84: Option<Vec<f64>>,
pub geog_to_wgs84_geokey: Option<Vec<f64>>,

let mut geog_inv_flattening = None;
let mut geog_azimuth_units = None;
let mut geog_prime_meridian_long = None;
let mut geog_to_wgs84 = None;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut geog_to_wgs84 = None;
let mut geog_to_wgs84_geokey = None;

GeoKeyTag::GeogPrimeMeridianLong => {
geog_prime_meridian_long = Some(value.into_f64()?)
}
GeoKeyTag::GeogToWGS84 => geog_to_wgs84 = Some(value.into_f64_vec()?),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GeoKeyTag::GeogToWGS84 => geog_to_wgs84 = Some(value.into_f64_vec()?),
GeoKeyTag::GeogTOWGS84GeoKey => geog_to_wgs84_geokey = Some(value.into_f64_vec()?),

geog_inv_flattening,
geog_azimuth_units,
geog_prime_meridian_long,
geog_to_wgs84,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
geog_to_wgs84,
geog_to_wgs84_geokey,

Copy link
Member

Choose a reason for hiding this comment

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

Rename to tests/geogtowgsgeokey_tiff.rs

@weiji14
Copy link
Member

weiji14 commented Nov 12, 2025

To be clear, I'm not sure if we should add support for this GeogTOWGS84GeoKey. From reading opengeospatial/geotiff#6, it sounds like 2062 will be reserved, but it doesn't look like it will become part of the GeoTIFF 1.2 spec (whenever that happens). We should however, at least handle the error gracefully instead of panicking.

@maxrjones
Copy link
Member Author

To be clear, I'm not sure if we should add support for this GeogTOWGS84GeoKey. From reading opengeospatial/geotiff#6, it sounds like 2062 will be reserved, but it doesn't look like it will become part of the GeoTIFF 1.2 spec (whenever that happens). We should however, at least handle the error gracefully instead of panicking.

Does this mean that async_tiff would always error on files containing this key, even if the errors are more informative?

@weiji14
Copy link
Member

weiji14 commented Nov 12, 2025

Does this mean that async_tiff would always error on files containing this key, even if the errors are more informative?

Yes, there will be an error. But no, it won't always be an error if we get #100 done, because that means users can pass in a custom tag registry (that could include this GeogTOWGS84GeoKey).

@weiji14
Copy link
Member

weiji14 commented Nov 22, 2025

FYI, discussed with Kyle about this offline at FOSS4G. Decided that unknown GeoTags (such as this 2062: GeogTOWGS84GeoKey) can just be skipped, and if someone wants to parse it properly, they can use a custom extension tag registry once that is done for #100.

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

Successfully merging this pull request may close these issues.

3 participants