802: handle EXIF orientations 3, 6, and 8 when cropping photos #816
Conversation
|
I’m mixed on writing tests for this. On one hand, it would ensure this works, does not break other things, and does not regress; I agree it’s a best practice. On the other hand, I’m not that comfortable with Python and I’m unsure how to write the tests for this case. Help?! This page has some useful test images having EXIF orientation values: https://magnushoff.com/articles/jpeg-orientation/ These files have rotations 3, 6, and 8, which this PR covers: |
| exif = dict(image._getexif().items()) | ||
|
|
||
| if exif[orientation] == 3: | ||
| current_app.logger.info('image had EXIF orientation 3; rotating 180 degrees') |
There was a problem hiding this comment.
These loggers might be useless. They don’t really provide any actionable information and they expose a behavior the user would otherwise assume “just works.”
There was a problem hiding this comment.
I don't see the detriment to keeping them. It would be interesting for admins to see how often we run across these EXIF rotation issues.
| @@ -473,6 +474,32 @@ def create_description(self, form): | |||
| date_updated=datetime.datetime.now()) | |||
|
|
|||
|
|
|||
| def conditionally_rotate_image(image): | |||
| try: | |||
| # NOTE: Pillow >= 6.0 provides `ImageOps.exif_transpose` which acts like the below code | |||
There was a problem hiding this comment.
Any reason not to bump the Pillow dependency version instead?
There was a problem hiding this comment.
I’ll try that, too. I was a bit hesitant to mess with dependencies.
I would suggest adding those sample 'F' images to |
|
@dismantl thanks for the feedback! I was also thinking the hashes of the image bytes would be useful for testing. I’ll poke around. Additionally — is there an automated test runner for this code? I finally decided to speed up my test runs by specifying individual files in the |
|
I’m going to rework this on Pillow >= 6.0 |
@parhamr Yes, |
Status
Ready for review
Description of Changes
Fixes #802.
Changes proposed in this pull request:
Screenshots
The first two photos for Haywood BUTZ are crops of an image having EXIF orientation values; this proves it works because the F is upright. The last photo (cartoon) has no EXIF orientation and it appears upright, as expected.
Here’s what happens with the same image files against the
developbranch: (theFis erroneously rotated 90º counter-clockwise)Tests and linting
I have rebased my changes on current
developpytests pass in the development environment on my local machine
flake8checks pass