#37140 closed defect (bug) (fixed)
Rotating images doesn't adjust orientation for imagick
Reported by: | triplejumper12 | Owned by: | kirasong |
---|---|---|---|
Milestone: | 4.7.3 | Priority: | normal |
Severity: | normal | Version: | 3.5 |
Component: | Media | Keywords: | needs-patch fixed-major |
Focuses: | Cc: |
Description
If you rotate an image that contains EXIF information, the EXIF orientation isn't adjusted to reflect that the image had been rotated.
For instance: If I upload an image that has an orientation of 8, and rotate it so it is actually being displayed as a portrait instead of landscape, the orientation of the new rotated image is still 8.
The orientation should be updated or removed since the image itself is rotated and doesn't need the orientation to display properly.
Attachments (14)
Change History (61)
#1
@
8 years ago
- Keywords needs-patch added
Thanks for the report @triplejumper12 and welcome to Trac!
While this isn't an exact duplicate of #14459, it's very similar. Both issues could be fixed by improving our handling of orientation data during upload/editing.
#3
@
8 years ago
- Keywords has-patch needs-testing needs-unit-tests added; needs-patch removed
Looks like I added that needs-patch keyword a bit too quickly :)
#4
@
8 years ago
- Milestone changed from Awaiting Review to Future Release
- Version changed from trunk to 3.5
This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-images by markoheijnen. View the logs.
8 years ago
#7
@
8 years ago
I tested with simply setting the orientation exif data to undefined in the Imagick WP_Image_Editor rotation method - my thinking being if a user has started rotating the image there's a reason, in my user's case the orientation data not being used. If they see it rotated the way they want without any orientation data then that orientation will be consistent across platforms & devices.
Worked well with an image that appeared upside down in the WP admin on upload but presented the right way up when viewing the image on it's own in a new tab.
After rotating and removing orientation data it was then consistent between the WP admin, the rendered post page, image in it's own tab (Chrome, Safari & FF on mac) and on my iPhone too.
I then found this ticket and the patch is exactly same code :) Works well.
#8
follow-up:
↓ 9
@
8 years ago
@sanchothefat
Great news.
Hopefully this can be fixed in core. Since it seems like it has been a minor issue for a while.
#9
in reply to:
↑ 8
@
8 years ago
Replying to lukecavanagh:
@sanchothefat
Great news.
Hopefully this can be fixed in core. Since it seems like it has been a minor issue for a while.
Awesome. while it's not fixed on upload immediately I think it at least makes sense to update the orientation data when someone intentionally rotates the image. With some further testing setting the value to Imagick::ORIENTATION_TOPLEFT
eg. 1 works fine and is probably preferable.
Will have a go at adding some tests.
#10
@
8 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
@lukecavanagh sorry for the spam - added a patch with a unit test and sample images with orientation exif data to test against.
This only addresses the issue when actively rotating an image via the admin as per the scope of this ticket but it's a good place to start regarding correcting the problem on upload and for adding additional tests. I'll look into suitable solutions for correcting images on upload next.
This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-images by mike. View the logs.
8 years ago
#13
follow-up:
↓ 15
@
8 years ago
I don't think it will be a problem, but we should make sure that we test the orientation of all images included in a srcset
attribute here so we don't end up with different users seeing different orientations based on their viewport width or pixel density.
#14
follow-up:
↓ 16
@
8 years ago
- Keywords needs-patch added; has-patch removed
The orientation should be updated or removed since the image itself is rotated and doesn't need the orientation to display properly.
Not only after a rotation. When an image is edited by the user, we should check and remove/reset the orientation in all cases, no matter what the editing action was.
After editing the image is how the user intends it to be, including the orientation. This will also match the behavior in GD where EXIF is removed.
#15
in reply to:
↑ 13
@
8 years ago
Replying to joemcgill:
Don't think this will be a problem too. When adding srcset we check for edited images and output only the sub-sizes that match the file name. Editing an image will always reset the orientation (see previous comment) and all sub-sizes will "inherit" that.
#16
in reply to:
↑ 14
@
8 years ago
Replying to azaozz:
Not only after a rotation. When an image is edited by the user, we should check and remove/reset the orientation in all cases, no matter what the editing action was.
Thanks for the feedback! I'll update the patch and tests asap.
This ticket was mentioned in Slack in #core-media by rob. View the logs.
8 years ago
#18
@
8 years ago
- Keywords has-patch added; needs-patch removed
@azaozz @joemcgill I attached a rewrite (albeit with the wrong number suffix). It adds a helper function wp_get_image_edit_hash()
which replaces the preg_match()
calls. I haven't written a test for that yet but I've moved the exif orientation removal to the _save()
method of the image editor class and checked it's a user edit before doing so.
Also added tests to check orientation data is removed on various edit methods and that the data is preserved when no edit hash is found.
#19
@
8 years ago
- Milestone changed from Future Release to 4.7.2
37140-3.patch looks good here. @joemcgill anything still stopping this? It will be needed when editing older images even if #14459 gets in.
Not sure if Imagick::ORIENTATION_UNDEFINED
(37140.patch) is better than Imagick::ORIENTATION_TOPLEFT
. Seems logical to set it to the proper value.
#20
@
8 years ago
- Owner set to joemcgill
- Status changed from new to assigned
@azaozz I don't think there is anything else blocking. I'll own giving this another look over before committing. Thanks @sanchothefat.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#24
@
8 years ago
Been having "second thoughts" about this. It seems best to make the behavior exactly the same as when GD is used: reset the exif orientation on all resized (or otherwise processed) images.
Then at least the behavior will be consistent (GD, ImageMagick), and the resized or edited images will not be affected by any browser or device inconsistencies/differences.
See 37140.4.patch (untested).
#25
@
8 years ago
As we talked about in last week's media chat, I think/agree that we should go ahead and fix the case described in this ticket (reset when rotating images) for 4.7.x, and continue considering something wider reaching in #14459.
For this ticket, I like the approach in 37140-1.patch the best so far.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by mike. View the logs.
8 years ago
#30
follow-up:
↓ 32
@
8 years ago
I identified some .tmp
files being left over while reviewing, and tracked it down.
Have a question -- @sanchothefat, do you think it's necessary to test all 8 rotations you've uploaded, or to help keep tests lean, do you think it's sufficient to pick one rotation, and use that?
I'm tending towards the latter, but wanted to check with you before moving forward in that direction.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#32
in reply to:
↑ 30
;
follow-up:
↓ 33
@
8 years ago
Replying to mikeschroder:
Have a question -- @sanchothefat, do you think it's necessary to test all 8 rotations you've uploaded, or to help keep tests lean, do you think it's sufficient to pick one rotation, and use that?
Hi Mike, yeah I think that's fine, I confirmed they all work so probably just we can just use the upside down one as that's the most common problem.
#33
in reply to:
↑ 32
@
8 years ago
- Keywords needs-testing removed
Replying to sanchothefat:
Hi Mike, yeah I think that's fine, I confirmed they all work so probably just we can just use the upside down one as that's the most common problem.
Thanks! Will take care of it.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jnylen. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by mike. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by mike. View the logs.
8 years ago
@
8 years ago
Merged with existing Imagick tests, shifted to 90 degree rotation in test, moved to using one test image, comments.
#39
@
8 years ago
- Keywords fixed-major added; has-unit-tests has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for 4.7.3.
#40
@
8 years ago
- Keywords needs-patch added; fixed-major removed
Annnd, I've realized I missed something (that took a while).
We need to double-check for existence of both the method and constant.
#41
@
8 years ago
- Keywords has-patch added; needs-patch removed
37140-1.3.diff feature-checks to make sure both the constant and method are available.
It looks like they've been around for at least 9 years, so this should succeed most of the time, but it's better to be safe.
Since we're on edge cases, is there any reason we should make sure this only happens with JPGs, or may as well handle it for other image types as well?
#43
@
8 years ago
- Keywords needs-patch fixed-major added; has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening again for 4.7.3.
This ticket was mentioned in Slack in #core-media by mike. View the logs.
5 years ago
#46
@
5 years ago
I can see that a lot of work went in to fixing this issue, over quite a long period of time, with MANY developers assisting. I just want to thank everyone who contributed to this fix. I've been waiting a very long time for this.
Thank you again, all of you!!! :-)
Patch to remove the orientation of the image after rotation