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

Support to set the new orientation as EXIF metadata #189

Closed
h2non opened this issue Apr 6, 2015 · 14 comments
Closed

Support to set the new orientation as EXIF metadata #189

h2non opened this issue Apr 6, 2015 · 14 comments
Milestone

Comments

@h2non
Copy link

h2non commented Apr 6, 2015

In case of image rotation / flip, it could be useful to store the new orientation as image metadata header before saving?

I guess it could be done from image headers functions:
http://www.vips.ecs.soton.ac.uk/supported/7.40/doc/html/libvips/libvips-header.html#vips-image-set

Thanks

@h2non h2non changed the title Support to set the image orientation metadata on save Support to set the new orientation as image metadata Apr 6, 2015
@lovell
Copy link
Owner

lovell commented Apr 6, 2015

Is this due to a side effect of using the existing auto-orient feature of rotate() and withMetadata() together, where the output image might then have the incorrect metadata value? If so, a quick fix might be to clear this header if a rotation/flip was required to achieve auto-orientation.

If more than this, then It's possible to set the orientation value; the complexity is in what value it should be set to.

What would you expect to happen if there is already an existing orientation value combined with an explicit rotation or flip? A few worked examples or test cases would help the implementor of such a feature.

Simplifying things slightly, would exposing the ability to manually set arbitrary EXIF headers provide enough API for you to plug in your own logic?

@h2non
Copy link
Author

h2non commented Apr 6, 2015

Thank you Lovell for the quick response.

The idea is, essentially, in the case that the user want to flip an image, and then reprocess it, I think there's no way to auto detect the rotation degrees of the processed image. Regarding to the original value, the idea could be simply override the previous rotation value, if it exists o not.
In any case, I think the user should explicitly define a option to enable the override/update the image headers over the different transformation processes.

Regarding to the EXIF headers, I think it could be a good solution, therefore supporting native bindings from the public API could be a reasonable approach to provide the freedom to manipulate the EXIF metadata with more versatility.

@h2non h2non changed the title Support to set the new orientation as image metadata Support to set the new orientation as EXIF metadata Apr 6, 2015
@lovell
Copy link
Owner

lovell commented Jun 29, 2015

My current thinking around the behaviour and API to support this is to do both of the following:

  • Remove the orientation metadata, if present, when using any of rotate, flip or flop.
  • Extend the existing withMetadata method to accept optional key/values to override. Initially this would only support the orientation attribute, e.g. withMetadata({orientation: 0}).

@lovell lovell added this to the v0.11.0 milestone Jul 11, 2015
@lovell
Copy link
Owner

lovell commented Jul 13, 2015

The current libvips logic seems to be only able to set EXIF tags in JPEG output where those tags already existed in the input (and have not been explicitly removed).

This means the use of withMetadata({orientation: 3}) will only update an existing EXIF Orientation tag and will not add it.

@h2non
Copy link
Author

h2non commented Jul 13, 2015

What about vips_image_set()?

@lovell
Copy link
Owner

lovell commented Jul 13, 2015

@h2non Whilst it's possible to set arbitrary image headers during the pipeline, libvips' JPEG writer seems to loop over the input's EXIF headers to set the output headers and therefore is ignoring any inserted EXIF headers.

@jcupitt Would it make sense for libvips' logic to be modified to instead loop over all image headers with an exif- prefix at write-time?

@jcupitt
Copy link
Contributor

jcupitt commented Jul 13, 2015

Oh yes, you're right, you can't set new EXIF tags, I hadn't thought of that. You're right, it should probably look for any metadata starting with "exif-". I'll have a look.

On autorotate, I think my 2p would be to have a special autorotate() method which looked at the orientation tag, applied it, then removed it. I'd think it would be unexpected to auto remove it anywhere else.

lovell added a commit that referenced this issue Jul 13, 2015
Clear Orientation when rotate/flip/flop are used
@lovell
Copy link
Owner

lovell commented Jul 13, 2015

Thanks for confirming John. sharp's parameter-less version of rotate() provides the "autorotate" feature.

With commit d303703 I've added the ability to override the Orientation tag using withMetadata.

Any change to libvips to allow the insertion of new EXIF tags should get automagically picked up too.

The default behaviour of sharp, which I believe meets most web use cases, is to strip EXIF metadata anyway. Should someone rotate or mirror an image then any existing Orientation value is going to be wrong. At least they can now manually re-insert the wrong value if so desired :)

@lovell
Copy link
Owner

lovell commented Jul 13, 2015

This is now in master awaiting release.

@lovell
Copy link
Owner

lovell commented Jul 15, 2015

v0.11.0 will be released today with this improvement, thanks for the original suggestion.

@lovell lovell closed this as completed Jul 15, 2015
@h2non
Copy link
Author

h2non commented Jul 15, 2015

@lovell Thank you!

jcupitt added a commit to libvips/libvips that referenced this issue Jul 15, 2015
change exif names again: we were storing under @title, but that's both
subject to i18n, and unlookupable in libexif

we now use @name, which is not subject to i18n and can be searched for
... this will break most code which expects certain exif tag names

also, when we update exif, allow any tag, not just updates to existing
tags, see:

lovell/sharp#189
@jcupitt
Copy link
Contributor

jcupitt commented Jul 15, 2015

I've made a vips branch with this change.

Unfortunately I had to change the way exif tags are encoded as vips metadata fields :( before it was

"exif-ifd%d-%s", ifd, title

I'm not sure why. But the title field of an exif entry is subject to i18n and also can't be looked up by libexif, for obvious reasons, so there was no way to create a new exif tag on output.

I've changed it to be

"exif-ifd%d-%s", ifd, name

where name is the canonical tag name. These can now be looked up, so it's possible to create new tags by setting vips metadata.

Of course this means most vips exif metadata has been renamed, very unfortunate. The only good thing seems to be that the "Orientation" tag has the same title and name, so that at least hasn't changed.

All code which looks at exif tags needs checking and updating.

@lovell
Copy link
Owner

lovell commented Jul 15, 2015

Thanks John, your libvips' change feels like the right future-proof approach (even though it breaks the API).

Currently the only EXIF tag sharp cares about is Orientation so should work before and after.

@jcupitt
Copy link
Contributor

jcupitt commented Jul 17, 2015

I've fixed up some more stuff, added a test for exif modification, and merged to master.

jcupitt/libvips@2a6dd4e...677a7db

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants