Exif support attempt number 2

[spoutn1k]

Rebase @seaeagle1's PR and try to pull together multiple definitions of IFDs and entries. A lot has changed from the use of common structures.

[feefladder]

Hey, I saw your comment on #243 and had a look around in your code, where I saw you added generics to Image. In my pr #245, I also had a deep look at the Image struct and how it works with the decoder. Also, @fintelia said in 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 e.g. #205. I don't know what that would mean for this PR, and it should belong in its own PR, let's say we have these "problems":

• 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."

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

/// Hold all Images, possibly decoded. Also provides convenience functions
/// 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>
}

Additionally, I think the Decoder could use static methods for reading IFDs, like so:

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(#222)/wrapping purposes(#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 (#85) or read them all (#243 #242 yay tiff magic).
  • This would also allow reading just an IFD, without needing to read in all things

It's getting all a bit messy now and I can't save drafts. Also should this be its separate issue/discussion? (big refactor + API overhaul)

[spoutn1k]

Decoder/Encoder harmonization: The API is different, the structs are different, everything is different.

That's essentially what is being attempted here. The Directory type is now at the root and used in both encoder and decoder. However it expects to be tagged with the tiff format and that is wy generics are being passed down. I would sleep well knowing them gone, but this is a draft PR to see where this can be taken.

I appreciate the amount of research that shows through your comment, but I am swamped at the moment and cannot reciprocate. Please do create a discussion and we can discuss the implementation there.

[feefladder]

Hey, thanks for your reply, I'll see if I can make a discussion tomorrow. I think harmonization is a bit different than exif and this PR addresses both and my PR also overhauls internal structures. So I made this comment to separate that, after gathering what is needed for exif support. As I understand, it's also not entirely clear, but at least the following:

• smooth suppport for sub-ifds. (As I could see in your PR now, it writes to the same offset it came from?)
  • reading/writing of said sub-ifds
  • Currently, you use the decoder to read in the EXIF sub-ifd. In my async implementation, asyncness function signatures bubble up all the way, so the general idea is to have generic "decoder::read_ifd()" functions that work on buffers (or memreaders for ergonomics) also on the decoding side. Then if that is useful for encoding exif, that's great. <- the synergy 🌈

The Directory type is now at the root and used in both encoder and decoder.

Yes, and I'm suggesting 2 "types", images and "raw" ifds. Ill see if I can polish my post more better tomorrow.

However it expects to be tagged with the tiff format and that is wy generics are being passed down.

In the decoder, this information is a boolean flag, rather than a generic. I think essentially bigtiffness should only have to be used when reading/writing tags or headers, after which this information is redundant (if we store everything in u64 in our structures). Also ""TileByteLength"" tag in bigtiff doesn't have to be u64 per se (I think it doesn't make sense, since you'd have a very weird tiled tiff if it has tiles so big they don't fit into u32, you've done something wrong with the tiling then), which the boolean-flag-design allows for more easily.

[fintelia]

This PR is far too large, with for too much all rolled up into it to meaningfully review. The proper way to add a big feature would be starting with an issue to discuss the proposed design, then if there is favorable reception for maintainers, making a series of moderately sized PRs to implement it. Though that also assumes sufficient maintainer bandwidth, which at the moment is stretched very thin.

[spoutn1k]

Why is #221 not closed then ?

[fintelia]

I don't fully recall the state of that other PR, but it is more than 5x smaller than this one.

TBH I haven't figured out how to handle multi-thousand line PRs. Seems to be lose-lose regardless. Essentially every time I've tried to review such a large PR, it stalls out after several rounds of feedback, either because the author gets tired of making more and more changes (understandable!) or due to design disagreements that cannot be reconciled. Ends up being a waste of everyone's time and very demoralizing.

On the other hand, other options aren't much better. Closing such a huge PR comes across as harsh given the work that went into it, but not closing is often mistaken as an encouragement to do more work. Requesting a PR to be broken into several pieces comes across as agreeing to all the feature changes. Leaving some PR feedback at the same time as one of these other options leaves very mixed messages.

[spoutn1k]

I get your point. It did seem unfair and preferrential, especially as this one is a draft for exposure on an experiment and not submitted for review. I want to add that the numbers are inflated by moving code around, but that does not change the ultimate point you rightfully make. @fintelia one of the first things I did in this PR is centralize code implemented in both the encoder and decoder. Would that be a worthwhile PR to look at ?

[fintelia]

Do you mean the "Created global ifd definition" change? Haven't looked in detail, but from skimming:

• Making Decoder object to be generic over normal vs bigtiff: I don't think we should do this. It makes the API more inconvenient for callers, who now have to know in advance whether they've got a normal or bigtiff file
• Switches Encoder to working on typed IFD entries rather than bytes: Not sure I understand the motivation here. Is this to make it easier to encode nested IFDs later?

(Also reopening the PR, so we can continue in the same PR thread if we figure out how to pivot this to a sufficiently self contained change)

[197g]

It's way too many conceptual changes at once for what it's trying to add (also causing that a need to rebase again and again). We can take individual PRs for each of

• adding new recognize tags
• designing DecodedEntry (Entry + Value), ensuring it provides value beyond just those two types in combination; also consider that #249 is overlapping
• convincing us that the generic on Image, Directory, etc are ach a change to the positive. Code size matters a bit, more types are a burden on consuming them
• the Unknown planar configuration breaking change that left some unreachable!() in match statements. This needs careful review not to actually panic, which is not happening in a 2.4k PR
• the other spurious added details to Tag within the derive macro.
• the DirectoryEncoder which now has an existing implementation on main.

Please, factor your changes into individual units that can be invidiually looked at. I don't think the critique of that PR being too big was overstated or bloated by moving code around. It's just as bloated in concepts. For reference, take a look at the evolution of #229 as it incorporated feedback of simplification; and the even further reduction I managed to make before merging it; in terms of added concepts as well as dependencies.

And it wouldn't hurt to update which part of EXIF handling this is addressing now that we have more general IFD-pointer handling. Seems like EXIF is now almost just a specialization of given infrastructure, so why did it take more lines?

[spoutn1k]

Thanks for the housekeeping, this PR was haphazard in the addition of someone else's code in an unfamiliar codebase, and I really appreciate the core concepts being merged in main. I plan on maybe adding an example of how I successfully, yet clunkily, manage exif tags with the new changes and we can discuss the above changes are the most suited to simplify/streamline the user experience.

[197g]

Yes that would be great :tada: We have an Entry/Directory structure that is supposed to be shared between different styles of directories and thus unrelated ID lists so it would be great to find out more specifically where there is the most friction for trying to use non-tiff::tag::Tag keys etc.