Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 4 years ago

#37140 closed defect (bug) (fixed)

Rotating images doesn't adjust orientation for imagick

Reported by: triplejumper12's profile triplejumper12 Owned by: kirasong's profile 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)

37140.patch (656 bytes) - added by triplejumper12 8 years ago.
Patch to remove the orientation of the image after rotation
37140-1.patch (4.2 KB) - added by sanchothefat 8 years ago.
Same as original but with unit test
orientation-1.jpg (136.2 KB) - added by sanchothefat 8 years ago.
orientation data value 1
orientation-2.jpg (134.1 KB) - added by sanchothefat 8 years ago.
orientation data value 2
orientation-3.jpg (137.7 KB) - added by sanchothefat 8 years ago.
orientation-4.jpg (137.3 KB) - added by sanchothefat 8 years ago.
orientation-5.jpg (134.4 KB) - added by sanchothefat 8 years ago.
orientation-6.jpg (134.4 KB) - added by sanchothefat 8 years ago.
orientation-7.jpg (137.3 KB) - added by sanchothefat 8 years ago.
orientation-8.jpg (138.0 KB) - added by sanchothefat 8 years ago.
37140-3.patch (12.8 KB) - added by sanchothefat 8 years ago.
Rewritten to only remove exif orientation data on user edit
37140.4.patch (923 bytes) - added by azaozz 8 years ago.
37140-1.2.diff (2.4 KB) - added by kirasong 8 years ago.
Merged with existing Imagick tests, shifted to 90 degree rotation in test, moved to using one test image, comments.
37140-1.3.diff (860 bytes) - added by kirasong 8 years ago.
feature check before orientation set

Download all attachments as: .zip

Change History (61)

@triplejumper12
8 years ago

Patch to remove the orientation of the image after rotation

#1 @joemcgill
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.

#2 @joemcgill
8 years ago

  • Component changed from General to Media

#3 @joemcgill
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 @ocean90
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 @sanchothefat
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: @lukecavanagh
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 @sanchothefat
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.

@sanchothefat
8 years ago

Same as original but with unit test

@sanchothefat
8 years ago

orientation data value 1

@sanchothefat
8 years ago

orientation data value 2

#10 @sanchothefat
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: @joemcgill
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: @azaozz
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.

Last edited 8 years ago by azaozz (previous) (diff)

#15 in reply to: ↑ 13 @azaozz
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 @sanchothefat
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

@sanchothefat
8 years ago

Rewritten to only remove exif orientation data on user edit

#18 @sanchothefat
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 @azaozz
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 @joemcgill
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

@azaozz
8 years ago

#24 @azaozz
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).

Last edited 8 years ago by azaozz (previous) (diff)

#25 @kirasong
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

#27 @joemcgill
8 years ago

  • Owner changed from joemcgill to mikeschroder

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: @kirasong
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: @sanchothefat
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 @kirasong
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

@kirasong
8 years ago

Merged with existing Imagick tests, shifted to 90 degree rotation in test, moved to using one test image, comments.

#38 @kirasong
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40123:

Media: Reset Exif orientation after rotate in WP_Image_Editor_Imagick.

Due to inconsistencies in the way browsers handle Exif orientation data,
if a user manually rotates an image within WordPress, set the Exif orientation to
the default (1) so that the image displays with the same rotation/flip in every browser.

Props sanchothefat, triplejumper12, joemcgill, azaozz, markoheijnen, mikeschroder.
See #14459.
Fixes #37140.

#39 @kirasong
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 @kirasong
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.

@kirasong
8 years ago

feature check before orientation set

#41 @kirasong
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?

#42 @kirasong
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40129:

Media: After [40123], Feature check setImageOrientation.

In [40123], WP_Image_Editor_Imagick started using
Imagick::setImageOrientation and Imagick::ORIENTATION_TOPLEFT,
but had no equivalent feature check.

While they were introduced more than 9 years ago, it's important
to double-check everything is available before using with Imagick.

Fixes #37140.

#43 @kirasong
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.

#44 @joemcgill
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40135:

Media: Reset Exif orientation after rotate in WP_Image_Editor_Imagick.

Due to inconsistencies in the way browsers handle Exif orientation data,
if a user manually rotates an image within WordPress, set the Exif orientation to
the default (1) so that the image displays with the same rotation/flip in every browser.

Props sanchothefat, triplejumper12, joemcgill, azaozz, markoheijnen, mikeschroder.
Merges [40123] and [40129] to the 4.7 branch.
Fixes #37140. See #14459.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


5 years ago

#46 @donnaWPadmin
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!!! :-)

This ticket was mentioned in Slack in #core-media by mike. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.