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

Extensibility: mid-level API and harmonizing encoder/decoder structs. #250

Open
feefladder opened this issue Oct 3, 2024 · 2 comments
Open

Comments

@feefladder
Copy link

feefladder commented Oct 3, 2024

This is mainly a conversation starter. I myself was interested in having a Cloud-Optimized-Geotiff reader in Bevy, which led to #245. Now, @spoutn1k is working on harmonizing the API together with EXIF support over at #242. Both PRs, along with other issues:@fintelia said #232 (comment) multiple places that the encoding API needs an overhaul. Also in #222, someone agreed with me that private fields in the Image make inspection difficult. The main point is that I think that overhaul would mean using a more config/property type of design, rather than a generics design #205 (comment) #205.

How I would like to be able to build on top of image/tiff for GeoTiff (and hopefully also OME for others #228)

Basically in #246:

let reader = RangedStreamer::new(length, 30*1024, range_get);
    
    // this decoder will read all necessary tags
    let decoder =  Decoder::new_overview_async(reader, 0).await.expect("oh noes!");
    println!("initialized decoder");
    let cloneable_decoder = tiff::decoder::ChunkDecoder::from_decoder(decoder);

and over at geotiff:

struct GeoImage {
  image: tiff::Image, // Image is currently private-ish
  geo_tags: MyStruct
  // etc
}

What API/internal structure would help here

The What I think this library needs is:

  • Decoder/Encoder harmonization: The API is different, the structs are different, everything is different.
  • Extensibility: This library is made to build things on top of, and sort of provides API at different levels, but imho not as clearly as inspired by "libtiff: provides interfaces to image data at several layers of abstraction (and cost). At the highest level image data can be read [...] without regard for the underlying data organization, colorspace, or compression scheme. Below this high-level interface the library provides scanline-, strip-, and tile-oriented interfaces that return data decompressed but otherwise untransformed. [...] At the lowest level the library provides access to the raw uncompressed strips or tiles, returning the data exactly as it appears in the file."
  • ASync-supporting (reader agnostic) design: Base the decoding on images on memreaders/u8 buffers, <- where the mid-level api resides. For high-level API (current) There is the wrapping Decoder full of convenience functions for easy reading and inspection.

Now, I think it would be a good idea to have three levels of wrapping structs that hold data:

/// Hold all Images, possibly decoded. Fields are public so its structure is part of the api. (reduces maintenance burden, increases risk of breaking changes)
/// This is the "wrapper" type of struct that other implementers would re-implement. Where we provide a basic API over the mid-level API,
/// Importantly, it holds all structural elemtents of a TIFF that don't belong in an encoder/decoder and allows for incrementally reading tiffs. Basically, Encoder/Decoder both write from/to this struct while encoding/decoding an image.
/// Just use u64, since we're not targetting super memory-constrained things
struct TIFF {
   // ifd_offsets: Option<Vec<u64>> // was seen_ifds in Decoder, but I would like a scan_ifds() function
   images: Vec<enum {Image(Image), Offset(u64)}> // indices should match, or enum 
   bigtiff: bool,
   byte_order: ByteOrder,
}

/// 
/// IFD that contains an image as opposed to a sub-ifd (which also may contain an image, but we don't necessarily check there
struct Image<'a> {
  // useful fast-access tags
  // _not_ necessarily bigtiff or endianness
  sub_ifds: Vec<IFD>
}

/// IFD that doesn't necessarily hold all tags, or even image data
struct<'a> IFD {
  sub_ifds: Vec<IFD>, // vec ?is? a pointer, so ?no? infinity otherwise Vec<Box/Arc<IFD>>
  inner: BTreeMap<Tag, Entry>
}

Then, in the mid-level API, (Image level) has static functions, and is a thin, ergonomic wrapper that can be re-implemented if other tag-reading logic is wanted. (I'd almost say, add an EasyDecoder struct that needs less initialization.

impl Decoder {
  /// static method for reading in an IFD, are these on Image? If they are, I think they should be exposed there
  pub fn read_ifd<R: Read + Seek>(reader: R, offset: u64) -> IFD {todo!;}
  pub fn read_ifd_async<R: AsyncRead + AsyncSeek + Unpin> (reader: R, offset: u64) -> IFD {todo!().await}

  
  /// scans over the linked list of ifds without reading them
  pub fn scan_images(&mut self) {
    // same as next_ifd, but doesn't read and goes on completely
    self.ifd_offsets = todo!();
  }
  /// read in all ifds of all images
  pub fn read_image_ifds<R: Read + Seek>(&self) {
    if self.ifd_offsets.is_none() {self.scan_ifds();} //should be unnecessary
    let Some(offsets) = self.ifd_offsets else { unreachable!(); }
    let self.images = offsets.map(|offset| self.read_image_ifd(offset)).collect();
  }
}
  • Document high-level vs mid/low-level api, which - I think - would
  • Publish Image for debugging(Private fields and interfaces make file inspection difficult #222)/wrapping purposes(Expose Image and friends to allow for Multithreading implementations #246)
    • For this, it should be clear what Image is. Now, there are multiple use-cases for image:
      • (partially) decoding a tiff <- the Directory<Tag, Entry> holds offsets and/or Values.
      • encoding a tiff or further processing <- the Directory<Tag, DecodedEntry> holds only Values
        • How to handle sub-IFDs in this case? Should we have something like:
          pub struct DecodedIfd<'a> {
            ifd: BTreeMap<Tag, DecodedEntry>;
            sub_ifds: vec<&'a DecodedIfd>
          }
          /// An Image that has retrieved all IFD info when decoding, or for encoding 
          pub struct DecodedImage<'a> {
            sub_ifds: Vec<&'a DecodedIfd> // or a more "basic" sub-ifd type
          }
          
          Then DecodedEntry holds a Value::IFD(sub_ifds.find(the_ifd)) or Value::IFD8(vec_index). Allows for recursion? Or Box something?
          I would say: "the [Image] struct holds all necessary metadata for decoding an image from an (async) decoder" <- tying Image and Decoder closer together, basically I'm trying to avoid a big maintenance
  • More control over the decoding process: Allow reading an image without directly reading all tags (Cloud compatibility #85) or read them all (Provide a way to retrieve all tags defined within a file #243 Exif support attempt number 2 #242 yay tiff magic).
    • This would also allow reading just an IFD, without needing to read in all things

Differences between Encoder/Decoder

Main difference is that the Encoder uses generics for SampleType (RGB8/RFloat64-type images)/TiffKind "bigtiffnes", whereas decoder uses properties/fields. As suggested elsewhere, fields is the preferred way forward, but this begs several questions.

  • How badly should we break the API?
    • new_image::() is very ergonomic, much better than manually setting fields. However, it leads to big manual lists of each and every type with its sampledepth etc due to combinatorial explosion.
    • I would say, keep the API, and create an Image/TIFF from it that holds the information as properties.

e.g.

impl ImageEncoder { // no generics
  fn new_image<T: SampleType>() {
     let image = Image::new()
     /// set the Image's values based on T, 
     image.set_tag(Tag::SamplesPerPixel, T::SAMPLES_PER_PIXEL)
  }
}

It's a bit rough atm, but here's the idea!

@fintelia
Copy link
Contributor

fintelia commented Oct 6, 2024

I unfortunately don't think there's maintainer bandwidth to support a complete overhaul of the decoding API. Doubly so if it involves a substantial expansion of the crate's API surface

@feefladder
Copy link
Author

As in there's none now, or there probably never will be? At its core, this issue was an attempt to harmonize @spoutn1k's efforts at harmonizing data structures between Encoder/Decoder. Then I thought that harmonization was a separate issue from EXIF, so opened this issue, because EXIF is also more build-something-on-top-of-tiff

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

No branches or pull requests

2 participants