EXIF tag handing

[spoutn1k]

Hey,

I am coming back to talk about EXIF tag handling following what I think is a good enough prototype in #242.

The PR is in draft and will stay that way as I am not fully happy with the implementation and agree with @fintelia's comments. However I am now able to read/write tags using the changes and this is a milestone I would like the library to have. This is especially important as the tools available to do so is essentially just the excellent exiftool, but calling external perl scripts is just not the best in every situation.

Reading tags is as easy as iterating over them. This is done by calling get_exif_data on the decoder:

let mut exif: Directory<ProcessedEntry> = decoder
    .get_exif_data()
    .expect("Unable to read Exif data")
    .into_iter()
    .collect();
print!("{exif}");

Writing tags is done by creating a Directory<ProcessedEntry> and passing it to the encoder:

/// Create a tag directory
impl From<ExposureData> for Directory<ProcessedEntry> {
    fn from(e: ExposureData) -> Self {
        let mut out = Self::new();

        if let Some(value) = e.author {
            out.insert(Tag::Artist, ProcessedEntry::new(Value::Ascii(value)));
        }

        if let Some(make) = e.make {
            out.insert(Tag::Make, ProcessedEntry::new(Value::Ascii(make)));
        }

        if let Some(model) = e.model {
            out.insert(Tag::Model, ProcessedEntry::new(Value::Ascii(model)));
        }

        out
    }
}

fn main() -> Result {
    // [...]
    let mut encoder = TiffEncoder::new(buffer)?
        .with_compression(Compression::Lzw)
        .with_predictor(Predictor::Horizontal)
        .with_exif(data.into());

    encoder.write_image::<colortype::RGB8>(
        photo.width(),
        photo.height(),
        &photo,
    )?;
    // [...]
}

This is compatible with both regular tags, GPS tags, and Interop Tags.

To do so, I promote the IFD module to have a singular approach for both encoder and decoder. Tags are separated in types for each of the three "classes" above. The definition of the Directory object used above is essentially a map:

/// Type representing an Image File Directory
#[derive(Debug, Clone)]
pub struct ImageFileDirectory<T: Ord + Into<u16>, E>(BTreeMap<T, E>);
pub type Directory<E> = ImageFileDirectory<Tag, E>;
//                                         ^^^ General purpose tags, no GPS or interop, those are other types

The entries are the values used. There are multiple definitions for convenience:

• DecodedEntry is closest to what exists in the main branch today with the raw information from the buffer (type + count + offset to data), has no meaning without a exif file;
• BufferedEntry is a type that contains a buffer with all the relevant information for the tag (type + count + data);
• ProcessedEntry is an array of ifd::Value with all the above data chunked up in the logical representation of the information.

Ie a list of 5 shorts would be read from the file as a decoded entry with type short, count u32 (or u64 for bigtiff) with value 5 and offset a u32 or u64 with the offset to the data in the buffer. We build a BufferedEntry with the same type, count as a u64 for convenience, and all the 5*(sizeof short) bytes in the buffer. The user can then convert this buffer into a Vecifd::Value::Short of 5 elements. The inverse transformation is operated when arbitrary tags are given by the user to the encoder.

Are you interested by such a feature and this kind of approach. Any feedback would be greatly appreciated.

[fintelia]

I think something in the style of #221 would be a better fit for integrating with the main image crate which now has a ImageDecoder::exif_metadata to return the EXIF data as a blob of bytes. Especially with there being third party crates like kamadak-exif and gufo-exif that can interpret the EXIF info itself. That would hopefully significantly cut down on the amount of code changes/additions needed in this crate.

See also image-rs/image#2494 tracking EXIF handling across the various image formats.

[spoutn1k]

kamadak-exif is a pure parsing library, so its scope is limited.

I am glad to see GNOME is implementing its own EXIF writer, but this stems from the limitation of this crate and IIRC was pretty lasckluster when I started working on this.

EXIF being encoded as TIFF, this crate should be able (and is, I am just adding a QOL layer) to handle it. Maybe not the validation of which tag has which value, but the encoding/decoding.
I don't think the solution to decode exif is to implement a blob extractor in the image-rs TIFF decoder to pass to a third-party TIFF decoder.

The changes in #221 are present in this PR, I just added the convenience structs I highlight above. The rebasing process was lengthy and I squashed the changes.

[197g]

I tend to agree. The internal structure of EXIF is practically given by the existing IFD handling code. The smallest steps beyond that would be the interpretation of the recorded information, that is the Exif IFD pointer tag (00EE) and maybe chaining the related EXIF dictionaries through some interface on the IFD data model. But yes, we need not interpret any of the contents within the chunks. (except: There are value offsets being encoded that are relative to the tiff file; as far as I understand, some (vendors) do that in the data portion so these can not reliably adjusted or separated from the underlying file completely. We could make an effort though).

[spoutn1k]

@197g Your message is not clear, with my points or @fintelia's ?

All I am looking for is to expose a coherent interface to retrieve the chunks and be able to write them to the tiff file being encoded in a easy-to-use interface.

The data should returned as-is, with no interpretation apart from its logical representation. When given a directory with data, the data will be serialized and written in valid TIFF, no matter if it represents valid EXIF.

I want to add that passing a tiff blob is not feasible to write EXIF in a TIFF file. On master, the encoder is not made accessible, and the EXIF tags are hardcoded:

image-tiff/src/encoder/mod.rs

Lines 420 to 438 in 7648317

	encoder.write_tag(Tag::ImageWidth, width)?;
	encoder.write_tag(Tag::ImageLength, height)?;
	encoder.write_tag(Tag::Compression, compression.tag().to_u16())?;
	encoder.write_tag(Tag::Predictor, predictor.to_u16())?;
	encoder.write_tag(Tag::BitsPerSample, <T>::BITS_PER_SAMPLE)?;
	let sample_format: Vec<_> = <T>::SAMPLE_FORMAT.iter().map(|s| s.to_u16()).collect();
	encoder.write_tag(Tag::SampleFormat, &sample_format[..])?;
	encoder.write_tag(Tag::PhotometricInterpretation, <T>::TIFF_VALUE.to_u16())?;
	encoder.write_tag(Tag::RowsPerStrip, u32::try_from(rows_per_strip)?)?;
	encoder.write_tag(
	Tag::SamplesPerPixel,
	u16::try_from(<T>::BITS_PER_SAMPLE.len())?,
	)?;
	encoder.write_tag(Tag::XResolution, Rational { n: 1, d: 1 })?;
	encoder.write_tag(Tag::YResolution, Rational { n: 1, d: 1 })?;
	encoder.write_tag(Tag::ResolutionUnit, ResolutionUnit::None.to_u16())?;

IIRC my research back then led me to believe that any further tag for the image being encoded would need an offset from that writer, which is not accessible.

[197g]

I was suggesting that we should be able to have basic exif support in much fewer than +-2.4k loc with TIFFs close ties to exif. Maybe we can have an 80% solution with much fewer implementation overhead by only poviding an interface into the existing implementation, re-using large chunks under the hood. You don't need a lot of the typing overheads in #242 for the basic ability of writing data into the EXIF IFDs.

It's much more crucial to me to identify the requirements, i.e. the inaccessible in-image offset we both noted, than a type-level meta programming implementation of the well-formedness requirements from the EXIF standard that can also be manually adhered to or added later.

Tl;dr: feedback is that any approach making extensive use of generics is probably overengineered.

[spoutn1k]

Fair points. The one thing I feel is necessary first is the merging of the two implementations of exif directories, as passing one struct to the other is a lot less clean now than if there was one consistent definition. I can omit the generics at the expense of code succintness. Can this be its own PR ?

[spoutn1k]

I just saw there was some work on directories/exif merged while I was out. Anything I can help with ?

[197g]

From the above draft, the method for copying all Exif information into a separate byte buffer is still missing. I've been focused on the underlying infrastructure code for accessing all kinds of directories and left the semantics of specific kinds of directories for later.

Also, I think there may be value in having a separate TagExif enumeration which represents only known ExifTags so they can be filtered with the macro-generated new. The Tags are separately defined from the Tiff 6.0 standard tags and are in the undefined range (>0x8000), hence it'd be odd to interpret them in all directionaries the same way. And then there are GPS tags and Interability Tag which, to my understanding, should only be present in their respective IFD because they use the range 0x00–0x1f.

[spoutn1k]

Its worth noting that some valuable information is present outside of those well-defined ranges, as the below image shows:

Which new macro are you talking about ? I do not personally think filtering is worth it for this library, the parsing libraries are able to figure it out. You are correct about GPS and Interop tags.

The potential improvement I am seeing is now to simplify access to the encoder from those third-party libraries. I am passing all undefined tags as Tag::Unknown(0xval) which is not idiomatic.

[197g]

Sorry, I was referring to from_u16, not new. With regards to filtering though it is valuable if we want to offer functionality to extract the tiff directories into a separate independent file (i.e. that is the data format image expects it, a separate Vec<u8>). We can't rule out unknown tags being or containing encoded offset values that would only be valid for the original file and will be at best confusing in a separate pure EXIF file. In general, if we copy values from a dictionary to another stream we should at least know their semantics.

Of course, exposing a method that operates at byte-level is fine, if the caller knows more about the semantics. But if such a directory copy is built-in specifically for the purpose of extraction then I would rather not copy data with uncertain effects on the output file.