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

images: Add AutoOrientation flag #8751

Closed

Conversation

Roganik
Copy link

@Roganik Roganik commented Jul 13, 2021

When AutoOrientation flag set to true, image will be transformed
after decoding according to the EXIF orientation tag (if present).
This will help with cases when EXIF orientation tag is unexpectidly
overriden to default value.

Fixes #5181

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2021

CLA assistant check
All committers have signed the CLA.

@Roganik
Copy link
Author

Roganik commented Jul 13, 2021

Hi,

I've faced a problem that one of the images loses EXIF orientation when resized by Hugo. After digging I found the #5181 issue

I've tested my changes locally, and when I set the AutoOrientation to true my image keeps the original EXIF orientation as I expect.

I don't know if it's a breaking change or not, so I've set the default AutoOrientation to false to keep the default Hugo behavior unchanged. But honestly, I'd expect that the default value for AutoOrientation will be true, so other people won't need to figure out why their images have wrong orientation.

I'd love to hear a maintainer opinion on the default value AutoOrientation. And please let me know if I missed something.

Thanks

@bep
Copy link
Member

bep commented Jul 13, 2021

I will happy to merge this if you can do it without introducing a new big image library.

@bep
Copy link
Member

bep commented Jul 15, 2021

We should probably look a this in relation to #8586

@Roganik Roganik force-pushed the 5181-support-AutoOrientation branch from 328070c to 738c0c7 Compare July 17, 2021 09:49
@Roganik Roganik changed the title images: Add AutoOrientation flag [wip] images: Add AutoOrientation flag Jul 17, 2021
@Roganik
Copy link
Author

Roganik commented Jul 17, 2021

I considered two approaches:
1) - always write the exif data that's lost on image modification (image.go -> doWithImageConfig)
but I didn't found good examples on writing exif data so I went with another approach.

2) - read orientation tag and rotate image per tag value in hugo image processing pipeline
I've implemented this approach.
The caveat is that right now i don't support all orientation tag values. I support only one's that rotate image (3 = Rotate 180 ,6 = Rotate 90 CW ,8 = Rotate 270 CW ), and don't support the ones that mirror image (2 = Mirror horizontal, 4 = Mirror vertical, 5 = Mirror horizontal and rotate 270 CW, 7 = Mirror horizontal and rotate 90 CW).

It'll fix my use case: image shot on a phone and has Orientation = 6 = Rotate 90 CW.
But i'm not sure if it's an acceptable solution.

Bep, I'd love to know your thoughts before i continue working on a code.
Is second approach an acceptable or the first one is more desired?

Thanks

Note: i'm working on the top of Bep changes from master...bep:feat/replace-exif-lib so review looks messy right now. (it mixes both Bep and my changes)

@Roganik
Copy link
Author

Roganik commented Jul 18, 2021

Oh, i found another lib imageorient. It's a small lib that only handles image orientation and nothing more.

Bep, will you merge changes if I use the small library imageorient instead of using big library imaging and instead of writing custom rotation implementation?

The changes will be similar to 2d13df8

@Roganik Roganik force-pushed the 5181-support-AutoOrientation branch from 738c0c7 to 2c6f1f3 Compare July 25, 2021 07:27
@Roganik
Copy link
Author

Roganik commented Jul 25, 2021

I've used imageorient lib that only handles image orientation and nothing more.

Tested locally - it works.

@Roganik Roganik changed the title [wip] images: Add AutoOrientation flag images: Add AutoOrientation flag Jul 25, 2021
When AutoOrientation flag set to true, image will be transformed
after decoding according to the EXIF orientation tag (if present).
This will help with cases when EXIF orientation tag is unexpectidly
overriden to default value.

Fixes gohugoio#5181
@Roganik Roganik force-pushed the 5181-support-AutoOrientation branch from 2c6f1f3 to 79373e2 Compare July 27, 2021 02:22
@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
Please check https://github.com/gohugoio/hugo/blob/master/CONTRIBUTING.md#code-contribution and verify that this code contribution fits with the description. If yes, tell is in a comment.
This PR will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@github-actions github-actions bot added the Stale label Feb 13, 2022
@Roganik
Copy link
Author

Roganik commented Feb 13, 2022

Bep, you've asked to not use big library, so I've added small one: imageorient that only handles image orientation and nothing more. It's the lib of the same author that wrote gift image processing library that's already used in Hugo.

@bep Can you please advice if this changes are good to be merged?

If they do, I'll rebase the changes and re-check code.
If my code needs some polishing - I'm open to feedback.
If you don't wish to merge such changes please let me know so I can close this PR.

Thanks

@github-actions github-actions bot removed the Stale label Feb 14, 2022
@Roganik
Copy link
Author

Roganik commented Feb 16, 2022

@jmooring, is there a chance you can advice on the fate of this PR?
Just rebase it and re-check that changes works. Improve something. Or close it off without merging?

@bep
Copy link
Member

bep commented Feb 16, 2022

I'm not adding more dependencies to get this done (we do not want to read the Exif info twice, for once).

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for image AutoOrientation
3 participants