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

Image metadata handling #2222

Open
fintelia opened this issue Apr 30, 2024 · 3 comments
Open

Image metadata handling #2222

fintelia opened this issue Apr 30, 2024 · 3 comments
Labels
kind: API missing or awkward public APIs kind: enhancement

Comments

@fintelia
Copy link
Contributor

fintelia commented Apr 30, 2024

This crate currently has limited support for handling image metadata. Taking a bit of time to jot down some thoughts on how we might be able to add support for basic image metadata. This is very much still a draft

Structure of metadata

Basing on what what metadata can be stored in PNG images (though other formats are similar) we might want to represent metadata something like:

/// Orientations from EXIF
pub enum Orientation {
    Normal = 1,
    FlipH = 2,
    ...
}

#[non_exhuastive]
pub enum ColorPrimaries {
    Unknown,
    bt709,
    rec2020,
    ...
}

#[non_exhuastive]
pub enum TransferFunction {
    Unknown,
    Srgb,
    Gamma(f32),
    Linear,
    ...
}
pub enum ColorSpace{
    /// Based on cICP PNG chunk
    Cicp {
        primaries: ColorPrimaries,
        transfer_function: TransferFunction,
        full_range: bool,
    },
    // Would be nice to parse the ICC profile further if we had a library that could do so.
    Icc(Vec<u8>),
}
pub const SRGB_COLOR_SPACE= ColorSpace::Cicp {
    primaries: ColorPrimaries::Srgb,
    transfer_function: TransferFunction::Srgb,
    full_range: true,
}

#[non_exhuastive]
pub struct Metadata {
    // Most (but not all) formats store orientation in the EXIF data. However, often this is the 
    // only metadata people care about from the EXIF chunk. Should be possible to extract this
    // field without implementing full EXIF parsing support.
    pub orientation: Orientation,

    pub color_space: ColorMetadata,

    // Could consider parsing this ourselves, but might be better to just tell users to use the 
    // `kamadak-exif` crate if they care about the contents. 
    pub exif: Option<Vec<u8>>,

    // XML encoded. Not super eager to ship a full XML parser to decode it...
    pub xmp: Option<Vec<u8>>,
}

impl Reader {
    fn decode_with_metadata(self) -> (DynamicImage, Metadata) { ... }
}

Decoding API

trait ImageDecoder {
    fn icc_profile(&mut self) -> ImageResult<Option<Vec<u8>>>; // already exists
    fn color_metadata(&mut self) -> ImageResult<ColorMetadata>;
    fn orientation(&mut self) -> ImageResult<Orientation>;
    fn exif_metadata(&mut self) -> ImageResult<Option<Vec<u8>>>;
    fn xmp_metadata(&mut self) -> ImageResult<Option<Vec<u8>>>;
}

impl Reader {
    fn decode_with_metadata(self) -> ImageResult<(DynamicImage, Metadata)> { ... }
}

Conversions

impl DynamicImage {
    fn apply_orientation(&mut self, orientation: Orientation) { ... }

    // Hard!
    fn convert_to_srgb(&mut self, current_color_space: &ColorMetadata) { ... }

    // Really hard!
    fn convert_colorspace(&mut self, current_color_space: &ColorMetadata, new_color_space: &ColorMetadata) { ... }
}

Encoding

TODO

@Shnatsel
Copy link
Contributor

This seems like a pretty large undertaking, and would be difficult to design in one go and without prototyping first. But long-running branches suck because of editing conflicts.

How about we get started on this in-tree, but gate the whole thing behind a non-default feature such as metadata-I-know-this-is-unstable ? This will let us prototype in-tree and get initial feedback from the interested users, without mega-branch that will burn out the authors with conflicts.

@Shnatsel
Copy link
Contributor

roxmltree could be a good candidate for XML parsing. It was written for resvg because other XML parsers were too complex and bloated. It has zero dependencies, 100% safe code, and has been extensively tested as part of resvg.

That doesn't solve writing XMP because "ro" stands for "read-only". But I guess we can feature-gate the functions relying on the parsed XML (if any) and let the users bring their own XML parser?

@fintelia
Copy link
Contributor Author

fintelia commented May 16, 2024

Yeah, that could be helpful if we ever need to parse XMP metadata. Though I think we could actually implement a bunch of the basic functionality without ever having to do that. Just having a way for the user to get the blob of XML bytes and telling them to figure out how to parse it if they care what's inside would probably be enough to start with.

I like the idea of experimenting behind a feature flag. For the ImageDecoder::{exif_metadata, xmp_metadata} (and maybe orientation) methods, I think the API is clear enough that we could skip that step, but for the bigger pieces it would be a nice assurance that we aren't going to back ourselves into a corner right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: API missing or awkward public APIs kind: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants