-
Notifications
You must be signed in to change notification settings - Fork 668
Expose IPTC metadata for PNG, JPG and Tiff #2611
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
base: main
Are you sure you want to change the base?
Conversation
This change adds an `iptc_metadata` method (similar to `xmp_metadata` and `exif_metadata`) to the `ImageDecoder` and implements it for PNG and JPG.
This comment was marked as outdated.
This comment was marked as outdated.
/// This method converts a `Value` to a vector of bytes. A `Value` in Tiff can have different | ||
/// types, e.g. a byte, a short or a float. This method intents to convert all these types to | ||
/// a vector of bytes (e.g. a u32 can be represented as a [u8; 4]). However, since this is only | ||
/// intended to parse values stored in XMP, IPTC and EXIF metadata sections, we ignore / return | ||
/// an error on a few Values that don't make sense in this context (e.g. Value::Ifd). | ||
fn value_to_bytes(value: Value, bytes: &mut Vec<u8>) -> ImageResult<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one thing I'd worry about here is architectural difference. Since this is doing little-endian conversion it makes me suspicious it may only work on little endian targets? Then again, to quote Linus Torvalds, "Oh Christ. Is somebody seriously working on BE support in 2025?" so I think this may be fine if we first check target_endianess
and always error for non-little targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I came up with this piece of code during the CI run which I think was running this on BE? I might misunderstood sth there :) Alternatively we could also just handle endianess correctly? I am also fine with erroring out, though!
The CI failure seems to be due to a spurious network failure in a setup step. Is there a way to retrigger the run? |
Friendly ping on this PR, would anyone have a moment to take a look? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder, got deep into other aspects. Except for the tiff aspect this LGTM, certainly that we do expose this in the same manner as other kinds of metadata.
/// This method converts a `Value` to a vector of bytes. A `Value` in Tiff can have different | ||
/// types, e.g. a byte, a short or a float. This method intents to convert all these types to | ||
/// a vector of bytes (e.g. a u32 can be represented as a [u8; 4]). However, since this is only | ||
/// intended to parse values stored in XMP, IPTC and EXIF metadata sections, we ignore / return | ||
/// an error on a few Values that don't make sense in this context (e.g. Value::Ifd). | ||
fn value_to_bytes(value: Value, bytes: &mut Vec<u8>) -> ImageResult<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one thing I'd worry about here is architectural difference. Since this is doing little-endian conversion it makes me suspicious it may only work on little endian targets? Then again, to quote Linus Torvalds, "Oh Christ. Is somebody seriously working on BE support in 2025?" so I think this may be fine if we first check target_endianess
and always error for non-little targets.
This change adds an
iptc_metadata
method (similar toxmp_metadata
andexif_metadata
) to theImageDecoder
and implements it for PNG and JPG.This is related to #2610. If my research was correct and IPTC metadata only exists in PNG and JPG, then this should fix the issue.