Extension tag registry

[kylebarron]

Just one quick note: I wonder if we could have a registry of extension tags. I'd love to remove geo specific tags from this repo, or at least remove them from their special status. Perhaps we could let users register custom tag codes plus how to parse them?

Originally posted by @kylebarron in #97

[weiji14]

I'd love to remove geo specific tags from this repo, or at least remove them from their special status.

I guess this means removing the geo_key_directory, model_pixel_scale and model_tiepoint fields and putting it in other_tags here?:

async-tiff/src/ifd.rs

Lines 133 to 141 in 6b7df90

	// Geospatial tags
	pub(crate) geo_key_directory: Option<GeoKeyDirectory>,
	pub(crate) model_pixel_scale: Option<Vec<f64>>,
	pub(crate) model_tiepoint: Option<Vec<f64>>,
	// GDAL tags
	// no_data
	// gdal_metadata
	pub(crate) other_tags: HashMap<Tag, Value>,

Perhaps we could let users register custom tag codes plus how to parse them?

Do you have a reference on how to make such a registry? I'm not sure how to extend the enum Tag here

async-tiff/src/tiff/tags.rs

Line 77 in 6b7df90

pub enum Tag(u16) unknown("A private or extension tag") {

and also how to extend the match case here to handle extension tags

async-tiff/src/ifd.rs

Lines 249 to 255 in 6b7df90

	// Geospatial tags
	// http://geotiff.maptools.org/spec/geotiff2.4.html
	Tag::GeoKeyDirectoryTag => geo_key_directory_data = Some(value.into_u16_vec()?),
	Tag::ModelPixelScaleTag => model_pixel_scale = Some(value.into_f64_vec()?),
	Tag::ModelTiepointTag => model_tiepoint = Some(value.into_f64_vec()?),
	Tag::GeoAsciiParamsTag => geo_ascii_params = Some(value.into_string()?),
	Tag::GeoDoubleParamsTag => geo_double_params = Some(value.into_f64_vec()?),

Do we... make a TiffTag trait with a method that can convert a Tag number to a Value?

[kylebarron]

I guess this means removing the geo_key_directory, model_pixel_scale and model_tiepoint fields and putting it in other_tags here?:

No definitely not. other_tags is a basket of tags that we don't know how to parse, but having them be so opaque makes them especially hard to read.

I'd like to find an API that doesn't make it harder to read geospatial tags but doesn't hard code support for them.

Do you have a reference on how to make such a registry?

There are a couple of registry-like things in this repo even! Take a look at

async-tiff/src/decoder.rs

Lines 14 to 50 in 6b7df90

	/// A registry of decoders.
	///
	/// This allows end users to register their own decoders, for custom compression methods, or
	/// override the default decoder implementations.
	#[derive(Debug)]
	pub struct DecoderRegistry(HashMap<CompressionMethod, Box<dyn Decoder>>);
	impl DecoderRegistry {
	/// Create a new decoder registry with no decoders registered
	pub fn new() -> Self {
	Self(HashMap::new())
	}
	}
	impl AsRef<HashMap<CompressionMethod, Box<dyn Decoder>>> for DecoderRegistry {
	fn as_ref(&self) -> &HashMap<CompressionMethod, Box<dyn Decoder>> {
	&self.0
	}
	}
	impl AsMut<HashMap<CompressionMethod, Box<dyn Decoder>>> for DecoderRegistry {
	fn as_mut(&mut self) -> &mut HashMap<CompressionMethod, Box<dyn Decoder>> {
	&mut self.0
	}
	}
	impl Default for DecoderRegistry {
	fn default() -> Self {
	let mut registry = HashMap::with_capacity(5);
	registry.insert(CompressionMethod::None, Box::new(UncompressedDecoder) as _);
	registry.insert(CompressionMethod::Deflate, Box::new(DeflateDecoder) as _);
	registry.insert(CompressionMethod::OldDeflate, Box::new(DeflateDecoder) as _);
	registry.insert(CompressionMethod::LZW, Box::new(LZWDecoder) as _);
	registry.insert(CompressionMethod::ModernJPEG, Box::new(JPEGDecoder) as _);
	Self(registry)
	}
	}

and also how to extend the match case here to handle extension tags

I think we'd have something like a Tag::CustomRegistry or something like that. We do still want to put tag data in other_tags when the tag keys don't match known values in the tag registry that the user has defined.

But I probably need to spend a little more time thinking about it than I've done so far.

[maxrjones]

Could the registries be iterable? E.g., I'd like to be able to do attrs = {k: v for k, v in ifd.geo_key_directory.items() if v is not None}.

[kylebarron]

I don't think it's worth having a way to inject custom registries from Python. I think this would be a Rust-only thing. I'm not sure how the Python API would change.

[kylebarron]

Could the registries be iterable? E.g., I'd like to be able to do attrs = {k: v for k, v in ifd.geo_key_directory.items() if v is not None}.

I think this is an unrelated question. The GeoKeyDirectory is a "tag of tags". It's unique in that the single TIFF tag contains a whole bunch of information. Most custom tags will only have a single piece of information

[weiji14]

Throwing in this PR - apache/arrow-rs-object-store#375 as another potential reference on 'how to make a registry'.

[feefladder]

The GeoKeyDirectory is a "tag of tags". It's unique in that the single TIFF tag contains a whole bunch of information. Most custom tags will only have a single piece of information

and in also interacts with two other tags, so three tags: GeoKeyDirectory u16 for the tags, GeoDoubleParamsTag f64 for the numerical values and GeoAsciiParamsTag String? for text stuff. e.g. this informative and quite understandable part of the spec appendix <- that does complicate stuff a bit more, since its not a function on a single tag that we could have in a registry, but rather feed multiple tags to a single registry entry

it all non-parseable/unknown tags that we read???.

I think if we'd want that parsing during metadata reading, the interaction of GeoKeyDirectory/GeoAsciiParamsTag/GeoDoubleParamsTag first idea would need something like (pseudocode):

pub struct ExtensionTagRegistry(BTreeMap<Tag, Box<dyn ExtensionTag>); // BTreeMap for congruence with tiff structure,  aesthetics and ultimate happiness
/// trait for custom tag handlers to implement
pub trait ExtensionTag {
  fn process_my_tag(value: Value)
  /// this will get all tags unknown to async-tiff when reading tiff metadata
  /// should return `true` if it stores the data
  fn process_other_tag(tag: Tag, value: Value) -> bool {
    false
  }
  // then here we'd want a "collection of functions that will cater to the special needs of all tags"
}

ImageFileDirectory::from_tags(tag_data: HashMap<Tag, Value>, &registry: ExtensionTagRegistry) {
  // ...
  match Tag {
    _ => if registry.iter().map(/*feed it the tag*/).all_is_false() {
      // I forgot about read_my_tag, will be slightly more complex: if we have a matching tag, do we still feed it to others? what about users having conflicting implementations?
      unknwown_tags.insert(tag, value)
    }
  }
}

I think the computational overhead of n*m function calls can be small because they most often don't do anything, but I do worry a bit about implementation ergonomics if users have to implement a match even if they only have one tag...¹

For the "collection of functions that will cater to the special needs of all tags", I think we can supertrait like ExtensionTag: std::any::Any and a user can ifd.extension_registry[GeoKeyDirectory.as_u16()].downcast_ref<UserGeoType>().my_method()? but that's a long line -could it be better?

In tiff2 I made an ifd kind-of-exactly a BTreeMap<Tag, Value>², but also recursive for sub-ifds to match the file structure as closely as possible. There I thought people would make a wrapper struct that parses and takes out their special tags. I think this abstraction could be better, since it doesn't require composition for e.g. reading both exif and geo tags (or other combinations).

Idea: use HashMap<Tag, Arc<dyn ExtensionTag>>

So multiple tags aka BTree/HashMap keys map to the same underlying registry entry. Would look like:

// tags being `u16` is kind of raw, they have names, but I think we can't use that user-defined stuff here 
pub trait ExtensionTag: std::any::Any {
  fn tags() -> &[u16];
  fn process_tag(&mut self, tag: u16, value: Value) -> AsyncTiffResult<()>;
}

pub struct ExtensionTagRegistry(BTreeMap<Tag, Arc<dyn ExtensionTag>>);

impl ExtensionTagRegistry {
  fn register(&mut self, tags: Arc<dyn ExtensionTag>) -> AsyncTiffResult<()> {
    // check for overlap
    for tag in tags.tags() {
      if self.0.contains(tag) {
        panic!()
      }
    }
    // add to self
    for tag in tags.tags() {
      self.0[tag] = tags.clone();
    }
  }
}

// geotiff crate
// actually these are already defined in src/tiff/tags
// const GEO_KEY_DIR: u16 = 34735;
// const GEO_DOUBLE_PARAMS: u16 = 34736;
// const GEO_ASCII_PARAMS: u16 = 34737;


pub enum GeoTags { // not sure if this enumness is nice, This crate favors option-filled structs I think
  Partial {
    key_dir: Option<Vec<u16>>,
    double_params: Option<Vec<f64>>,
    ascii_params: Option<String>,
  },
  Parsed(GeoKeyDirectory)
}

impl ExtensionTag for GeoTags {
  fn tags() -> &[u16] {
    &[Tag::GeoKeyDirectoryTag.as_u16(),Tag::GeoDoubleParamsTag.as_u16(),GeoAsciiParamsTag.as_u16()]
  }
  fn process_tag(&mut self, tag: u16, value: Value) {
    match tag {
      GEO_KEY_DIR => {
        // pseudocode
        let GeoTags::Partial{key_dir, double_params, ascii_params} = self else {panic!();};
        *key_dir = value.into_u16_vec();
      }
      GEO_ASCII_PARAMS => {
        ...
        // if ordered input, can parse self here
      }
      _ => panic!();
    }
    // otherwise need a check and parse here
  }
}

I just don't know if we can guarantee there is no overlap in tags that are needed for the parsing for different extensions/implementations, but I think that's a safe requirement. Using std::any::Any supertrait, we can retrieve the ExtensionTag from any of the tags it uses. e.g.:

// In the end we want to pass this registry to IFDReader.read_next_ifd(fetch)
// so that's TiffMetadataReader.read_all_ifds()
// I think it makes sense to add a registry to the metadata reader
// once that is there, e.g. in geotiff
fn main() {
  // get an ifd
  let ifd = magic!()
                                // literally any of the three
  let transform = ifd.extra_tags[Tag::GeoKeyDirectoryTag].downcast_ref::<GeoMetaData>().epsg_code()
}

I think "ExtensionTagRegistry" is quite a long word (7 syllables). I also don't know if all additional/extra tags people could want a custom parser for are necessarily the strict "tag as extension of tiff spec" that "Extension" suggests. So I would opt for ExtraTag(Registry) or CustomTag(Registry) (6 syllables), which is also more conceptually forgiving (than having Extra and Unknown tags).

Footnotes

1. I think those would be slightly improved if tag_data is a BTreeMap, since those guarantee ordering, so the geo tag machinery only has to check which tag is currently being read in to know if others also have been read (or is that again a leaky abstraction messing up intentionally separate layers) ↩

2. BTreeMaps keep the order of tags, which the tiff spec folks seem to like. ↩