Make WordPress Core

Opened 14 years ago

Closed 5 years ago

Last modified 3 years ago

#14459 closed enhancement (fixed)

Rotate Full Size Images on Upload

Reported by: mrroundhill's profile mrroundhill Owned by: kirasong's profile kirasong
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.0
Component: Media Keywords: has-patch needs-testing needs-unit-tests has-dev-note
Focuses: Cc:

Description

It may be worth a revisit to #7042. Some mobile devices that use WordPress for Android are not capturing images in the correct orientation, instead they are writing the EXIF orientation to the image instead (which is a standard method these days). In wp-android and other external clients that offer full size image upload, these images will not be rotated correctly upon upload.

Since most mobile users are on the go with no access to the wp-admin area to rotate the images themselves, it would work best if the image was rotated for them automatically.

Hopefully there's a solution that wouldn't strip the EXIF data, some way to copy the EXIF data before rotating, then save it back again?

Attachments (10)

14459.patch (1.3 KB) - added by msaggiorato 9 years ago.
Fix with GD Library
14459.diff (2.4 KB) - added by wpdavis 9 years ago.
14459.2.diff (2.1 KB) - added by markoheijnen 8 years ago.
14459.imagemagick.patch (1.3 KB) - added by dhuyvetter 8 years ago.
Imagemagick version of 14459.patch
14459.3.diff (10.7 KB) - added by pbiron 5 years ago.
CarolinaRaptorCenter_2005_1032.exiftool.txt (5.0 KB) - added by pbiron 5 years ago.
EXIF and IPTC metadata from CarolinaRaptorCenter_2005_0132.jpg. This was generated with exiftool and can be used to test that these metadata are preserved by my patch.
CarolinaRaptorCenter_2005_0132.jpg (93.9 KB) - added by pbiron 5 years ago.
Sample image with EXIF orientation tag value that requires rotation on upload. In addition to EXIF, it also contains IPTC and ICC Profile metadata, to test that part of my patch.
embedded-thumbnails-windows-explorer.png (40.8 KB) - added by pbiron 5 years ago.
example of Windows Exploring using the unrotated embedded thumbnail for an image that has be rotated on upload
14459.4.diff (8.5 KB) - added by azaozz 5 years ago.
14459.5.diff (9.3 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (112)

#1 @Otto42
14 years ago

+1, the EXIF Orientation tag is not respected by the image handler. This happens to iPhone images that are uploaded through the media uploader.

Documentation on the tag and the values therein can be found here:
http://sylvana.net/jpegcrop/exif_orientation.html

#2 @nacin
14 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#3 @mrroundhill
14 years ago

  • Keywords mobile added

#4 @azaozz
14 years ago

There was an older ticket about this (can't find it at the moment) that had a patch too. Think we didn't do it at the end as many users upload images directly from their cameras and rotating a 10-12 megapixel image was causing PHP to run out of memory.

#5 @Otto42
14 years ago

Related: #11536

Suggestion: Since there is compatibility issues with image rotation, maybe it's worth taking a look at the various functionality we need for image handling and write a wrapper class or two that works based on what support is offered on the host, as well as handling cases where available memory is low and such. Give the most functionality possible given the available functions.

#6 @westi
13 years ago

  • Keywords westi-like 3.4-early added

We should revisit this because it is becoming more and more common for people to want to upload and post quickly without having to manually fix the rotation of images.

#7 @Otto42
13 years ago

Rotating the image is easy enough, but I can't find any way to do this in PHP and keep the EXIF data in the resulting file. So the original unmodified file would have to be saved as well.

Unless I'm missing something.

#8 @dd32
13 years ago

Unless I'm missing something.

Unfortunately, IIRC, GD doesnt support EXIF preservation at all. That's one of the advantages of ImageMagik. Unless of course there's a PHP library to add exif back into the images afterwards.

#9 follow-up: @Otto42
13 years ago

Side note: The new image_resize checkbox will cause the code to strip the EXIF data as well, before it gets read in as the attachment metadata.

#10 in reply to: ↑ 9 @azaozz
13 years ago

Replying to Otto42:

The resizing is actually done with js when supported (most cases) which preserves exif data. Have a look at uploader->features in Firebug's dom tab. Wondering if they will add image rotation there too :)

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

#11 @Otto42
13 years ago

True, but if the resize isn't done on the client side (not supported case), then it's done in wp_handle_upload, and exif will be lost there.

#12 follow-up: @azaozz
13 years ago

Currently it's supported for all runtimes that we use (HTML5, Flash, Silverlight) except in Safari and Opera when using HTML5 http://www.plupload.com/. Alternatively we can disable resizing in php completely and allow it only when supported on the client side.

#13 in reply to: ↑ 12 @westi
13 years ago

Replying to azaozz:

Currently it's supported for all runtimes that we use (HTML5, Flash, Silverlight) except in Safari and Opera when using HTML5 http://www.plupload.com/. Alternatively we can disable resizing in php completely and allow it only when supported on the client side.

This doesn't really help when images are uploaded not using the uploader - e.g. XML-RPC, or a post-by-email solution - I don't think we should expect all clients to do the rotation for us.

Having it work only a %age of the time is confusing.

#14 @dd32
12 years ago

  • Milestone changed from Future Release to 3.5

Closed #21006 as a duplicate, pulling this into the 3.5 timeline for consideration as that's where the other ticket was.

#15 @merty
12 years ago

  • Cc merty92@… added

#16 follow-up: @azaozz
12 years ago

The best way to handle this in PHP is with jpegtran and exiftool. Both are command line tools available for all platforms, the EXIF is fully preserved (EXIF orientation is reset with exiftool) and the quality of the JPEG is preserved 100%. However using them would also require PHP's exec() to be enabled.

Another option is to rotate only the resized images leaving the original as-is (flikr does this). Perhaps this could be done only for larger JPEG images like photos, larger than the max size.

Yet another option would be to rotate the images on display. In most current browsers this is possible now but will be hard to implement in core.

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

#17 @azaozz
12 years ago

  • Keywords mobile 3.4-early removed
  • Owner set to azaozz
  • Status changed from new to reviewing

#18 @nacin
12 years ago

  • Milestone changed from 3.5 to Future Release

This requires jpegtran, exiftool, etc. Now that we have WP_Image_Editor, we can do this in a future release.

#19 @daniloercoli
12 years ago

  • Cc ercoli@… added
  • Keywords mobile added

#21 follow-up: @webmystery
12 years ago

It would be nice to fully recognize exif rotation. When a user uploads an exif rotated image the correct image sizes are not created and all of my automatic placement of the featured image /gallery images are displayed at full size, etc... Took me a bit of head scratching to figure out why one image in a particular post was displaying full-size, especially as I was not the one who uploaded it and the user who reported the problem neglected to tell me that hey had to rotate the image after the upload. Regenerating the image sizes didn't work either. I had to re-save the image in photoshop and re-upload it to fix the problem. This would have been impossible for the less geeky to figure out.

#22 in reply to: ↑ 21 @SergeyBiryukov
12 years ago

Replying to webmystery:

the correct image sizes are not created and all of my automatic placement of the featured image /gallery images are displayed at full size, etc...

This sounds like a result of #22985 or #19889.

#23 in reply to: ↑ 16 @henry.wright
11 years ago

I'm seeing this problem more and more - I think perhaps due to my site visitors moving away from their desktops and towards mobile access.

Replying to azaozz:

using them [jpegtran and exiftool] would also require PHP's exec() to be enabled.

Is PHP's exec() usually enabled by default?

Another option is to rotate only the resized images leaving the original as-is

Preserve Exif info on the original but obliterate it on resized images? Sounds the most logical approach

Yet another option would be to rotate the images on display.

Wouldn't this impact performance?

This ticket was mentioned in Slack in #core by simonwheatley. View the logs.


10 years ago

#25 @helen
10 years ago

#28676 was marked as a duplicate.

@msaggiorato
9 years ago

Fix with GD Library

#26 @msaggiorato
9 years ago

I just posted a patch for this, that fixed it in my case.

I implemented it in production for my site using a separate class that extends the load method of WP_Image_Editor_GD. (To avoid touching the WP Core files)

With this fix, original images are kept untouched, and thumbnails are generated correctly. I think the EXIF Orientation head will be lost in thumbnails, but i don't think that's a problem, since they will be in the correct size and orientation now.

PS: I tried several locations in the exif_read_data array, because i've found online that the Orientation header can be present in several places.

This is the first time I post a patch, so if I did something wrong, please let me know.

Version 0, edited 9 years ago by msaggiorato (next)

This ticket was mentioned in Slack in #core by msaggiorato. View the logs.


9 years ago

#28 @oliver033
9 years ago

I would ask that this be looked at. I don't mind that the rotation may be off when I upload but I am frustrated that I when I rotate the image myself in the media gallery after uploading that it does not always respect this rotation in the mobile format of my site.

#29 @heatherleecosta
9 years ago

I would also love a fix for this. Currently my site shows the photos fine on mobile but on desktop, they are sideways. I have multiple users uploading photos daily and most are not computer-savvy. Walking them through the process of editing their photos before upload has been very time consuming and is usually very frustrating for them.

#30 @SergeyBiryukov
9 years ago

#33051 was marked as a duplicate.

@wpdavis
9 years ago

#31 @wpdavis
9 years ago

Just added a patch option that catches this on upload.

However, something weird is going on. It seems the issue is when an image gets rotated but the orientation field in the EXIF data doesn't get changed. I'm not actually sure how to identify this, but if somebody has an idea I'm all ears.

Here are examples of pretty much the same file. test1, if uploaded with the patch applied, will appear correctly. test2, if uploaded with the patch applied, will actually be over-rotated.

http://trunk.wpdavis.com/test1.jpg => Exif: http://trunk.wpdavis.com/test1.php
http://trunk.wpdavis.com/test2.jpg => Exif: http://trunk.wpdavis.com/test2.php

If we figure out a way to identify more conclusively which images need to be rotated, we could always implement it with ImageMagick only to preserve IPTC data.

We've noticed that respect for the orientation field seems to vary by browser, so I think this would be nice to handle in core to make sure all images are properly rotated, if we can grok it.

Last edited 9 years ago by wpdavis (previous) (diff)

#32 @msaggiorato
9 years ago

Back when I posted my patch (which half solved the issue), I kept working on this issue. And came to realize that the GD library is not the best way to solve this issue as there are much more things in place than just the EXIF rotation change.

After a bunch of research about the binary format of JPEG files, i came across this library https://github.com/lsolesen/pel.

With the help of this library, i was able to extract most of the EXIF data from one image, and put it into the corrected image generated by the GD library (that has no EXIF data, or color profile, or any other information).

I also went a bit further and even fixed the thumbnail of the PNG (as it may be required, and it also should be consistent with the full image).

In the end, I managed to make this work at a decent level. But didn't post back the solution here because it involved too much server work and image data binary manipulation, that shouldn't happen if browsers did their job nicely.

I also thought that a solution like the one I implemented wouldn't be totally approved by the community.

I'll try to put together a GitHub gist with my solution as it is now if someone's interested.

#33 @jcmboyer
9 years ago

This problem just started 4 days ago for me. I have photos from my Android and they are turned in my dropbox. When I upload, they are the correct orientation in the post draft. When I preview the post, the image is upside down on my browser (using Chrome). When the post appears on mobile, it's correct. When the post is posted on social media, it's upside down.

I tried adding the image rotation plug in but it doesn't work for the newest version of WP. I am very frustrated as it worked and now it doesn't. I blog daily. I have read this entire thread but am wondering why I have to change the settings on the photos when it worked just fine 4 days ago.

#34 follow-up: @n7studios
9 years ago

Patch:
https://gist.github.com/n7studios/6a764d46bc1d515ba406

I encountered this problem today, and built a small WordPress Plugin to resolve it which:

  1. Checks for the EXIF orientation flag, to see if the image needs rotating,
  2. Rotates the image, if required,
  3. Writes the EXIF and IPTC data from the original image to the modified, rotated version
  4. Sets the EXIF orientation flag to 1, to prevent applications which read the flag from rotating the image (which would result in over rotation)

I appreciate there's still a lot of work to do, namely:

  1. Submitting this as an actual patch for WordPress core,
  2. Using the WP Filesystem for ovewriting the image file vs. fopen/fwrite wrappers
  3. I'm a bit dubious about line 140, as I think it'd potentially replace any 3, 6 or 8 value with 1 (potentially breaking other EXIF metadata - although in my tests, this didn't happen) - I struggled with hexadecimals:
    $exif_data = str_replace( chr( dechex( $original_orientation ) ) , chr( 0x1 ), $exif_data );
    

This doesn't require any particular PHP extensions / libraries, and most of the code is inspired by this ticket or submissions on php.net, which I've tried to credit where applicable. The main thing is that EXIF / IPTC data is retained - this is typically lost when using GD to edit an image.

Would be great to get some thoughts / feedback and see how this can be improved before submitting as a patch.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#36 @joemcgill
8 years ago

#36446 was marked as a duplicate.

#37 @divorcetheworld
8 years ago

I suggest taking a look at this plugin, which seems to work for me, even with non-iOs images.

https://wordpress.org/plugins/ios-images-fixer/

Photos taken with some older cameras display fine in Windows Photo viewer, but they appear sideways in the Media Library after upload. With this plugin installed, they appear properly oriented in the Media Library. Unfortunately, the "Crunching" time after upload was fairly long.

#38 @lukecavanagh
8 years ago

Do not know about this issue until today.

This ticket was mentioned in Slack in #core-images by lukecavanagh. View the logs.


8 years ago

#40 @lukecavanagh
8 years ago

Seems like a fix that should have made it into core a while back. So it seems from ticket 21006 that the issue is not on WordPress.com since that was fixed. But the issue is just on self-hosted WordPress.

@markoheijnen
8 years ago

This ticket was mentioned in Slack in #core-images by markoheijnen. View the logs.


8 years ago

#42 @lukecavanagh
8 years ago

@markoheijnen

The patch applies cleanly.

#43 @msaggiorato
8 years ago

Doesn't rotating the image like the last patch removes the EXIF information of the images? (at least when using GD)

#44 follow-ups: @lukecavanagh
8 years ago

@msaggiorato

You should be able to keep the EXIF information and use

http://php.net/manual/en/imagick.getimageorientation.php

#45 in reply to: ↑ 44 @msaggiorato
8 years ago

@lukecavanagh

I meant the rotate function from the GD library (which as far as i know is the most commonly available everywhere)

When you rotate the exif information of the original file was removed (at least until a year ago, not sure if there's been updates to GD ever since).

This has been discussed in this very same ticket (or related ones) at some point.

One more thing, we're ignoring the rest of the rotation positions (horizontally flipped versions of each one of the other)

This article here should mention all the possible Orientations.

http://www.impulseadventure.com/photo/exif-orientation.html

I hope this helps.

#46 @lukecavanagh
8 years ago

@msaggiorato

Thanks, I read that article last week as well.

#47 in reply to: ↑ 44 @triplejumper12
8 years ago

Replying to lukecavanagh:

@msaggiorato

You should be able to keep the EXIF information and use

http://php.net/manual/en/imagick.getimageorientation.php

Using Imagick's getImageOrientation is probably preferable to using exif_read_data, because exif_read_data can throw an error for some jpegs depending on how they have been modified in the past. getImageOrientation seemed to be more reliable and does the logic of finding the orientation from the exif data itself.

However, getImageOrientation will return the same orientation after an image has been rotated, so you would still have to remove the orientation or set it to 1 after you rotate it. Unfortunately, Imagick doesn't adjust the orientation when you rotate it like GD does.

Thought that might be helpful since I ran into this while trying to fix orientations on upload. I don't have any examples of the images this failed on anymore, but just something you might want to consider.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#49 @joemcgill
8 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.7

#50 @johnbillion
8 years ago

  • Keywords westi-like mobile removed

Related: #37140

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#52 @kirasong
8 years ago

On the above, unless we do something like @n7studios is suggesting, if we apply this for GD, it would indeed remove EXIF and IPTC data. I like this route, and it's worth exploring, but it's probably a wider scope than this ticket (Related: #11877).

I would rather not destroy this data by default for GD, since it's important to have in full size images for things including License/Copyright and color profiles.

Questions:

  • How much does this increase image resize CPU time/Memory usage?
  • What unexpected behavior might occur for clients that do support the rotation EXIF flag?
  • Should we fix this for Imagick first, if it looks most feasible?

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 by jeffpaul. View the logs.


8 years ago

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


8 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


8 years ago

#57 @joemcgill
8 years ago

  • Component changed from Upload to Media
  • Milestone changed from 4.7 to Future Release
  • Owner changed from azaozz to mikeschroder

I'm moving this to the Media component, so we can keep better visibility on this ticket and moving it out of the milestone for now. @mikeschroder I've reassigned this to you since you're the latest currently working on moving this forward.

#58 @SergeyBiryukov
8 years ago

#39534 was marked as a duplicate.

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 joemcgill. 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 joemcgill. View the logs.


8 years ago

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


8 years ago

#64 @kirasong
8 years ago

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.

#65 @joemcgill
8 years ago

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.

@dhuyvetter
8 years ago

Imagemagick version of 14459.patch

#66 @SergeyBiryukov
6 years ago

#44898 was marked as a duplicate.

#67 @joemcgill
5 years ago

#43808 was marked as a duplicate.

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


5 years ago

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


5 years ago

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


5 years ago

@pbiron
5 years ago

#71 in reply to: ↑ 34 @pbiron
5 years ago

  • Keywords needs-testing needs-unit-tests added

Replying to n7studios:

Patch:
https://gist.github.com/n7studios/6a764d46bc1d515ba406

  1. I'm a bit dubious about line 140, as I think it'd potentially replace any 3, 6 or 8 value with 1 (potentially breaking other EXIF metadata - although in my tests, this didn't happen) - I struggled with hexadecimals:
    $exif_data = str_replace( chr( dechex( $original_orientation ) ) , chr( 0x1 ), $exif_data );
    

14459.3.diff incorporates much of the plugin in the gist by @n7studios into core. First, props to him for providing a great foundation on which to base this patch!

Here's a high-level overview of how the patch achieves what it does:

  1. hooks into wp_handle_upload and rotates uplaoded JPEG images (with the image editor classes) if necessary based on the EXIF orientation
    1. the function hooked is in /wp-includes/media.php, the hook is added in /wp-includes/default-filters.php
    2. adding the hook there (as opposed to /wp-admin/includes/admin-filters.php where I had originally tried) allows it to work in the block editor, as well as the Media Library and classic editor
    3. see TODO below for one quirk about support in the block editor
  1. adds methods to WP_Image_Editor_GD that:
    1. when a JPEG image is saved by the editor, any JPEG application segments in the originaly uploaded image are copied to the saved image (this is necessary because GD strips all application segments from JPEGs)
      1. note that WP_Image_Editor_Imagick handles this automatically, because Imagick preserves those segments
      2. currently, only EXIF (APP1), ICC Profile (APP2) and IPTC (APP13) segments are copied, but I think the code is general enough that it can easily be extended to copy any segments...I just don't have any images that have other segments to test with, so I limited the implementation to those 3. Copying the ICC Profile (APP2) segment is not part of the gist from @n7studios.
      3. the JPEG application segments are copied from the original image to any image the editor saves, e.g., itermediate sizes, crops, etc.
    2. when a JPEG image has been rotated by the editor (whether in the function this patch hooks to wp_handle_upload, or when done interactively a user), the EXIF orientation tag value is updated to be "normal" (when the rotated image is saved)
      1. note that WP_Image_Editor_Imagick handles this automatically, thanks to [40123]
      2. if the original image did not contain the EXIF orientation tag then this step is not done (see Questions below)
      3. the way this is accomplished addresses the part that @n7studios was "a bit dubious about" in his gist, by finding the offset into the binary EXIF data where the orientation value is stored and altering that value in an "EXIF-compliant" way (instead of the str_replace() approach used in the gist.. which, yes, could adversely affect other EXIF data)
      4. I also did not include the code from the gist that checks whether the image being saved has a particular JPEG application segment and only copies JPEG segments from the original image if the image being saved doesn't. Why? Because the whole point of 2a is that GD doesn't save application segments so that code was unnecessary

Questions

  1. If the original image does not contain the EXIF orientation tag, should we add it when images are rotated and saved? I don't think so, but thought I'd ask
  2. If an uploaded image has an EXIF orientation of anything other than "normal" (e.g., 2, which means "flipped"), should we modify the image accordingly on upload?
    1. all the previous patches on this ticket and the gist from @n7studios are limited to rotation
    2. the scope of this ticket is just about rotation, but I think we should definately consider flipping on upload as well (see orientation-2.jpg for an example)
    3. if we flip on upload, then we should also set the EXIF orientation to normal when WP_Image_Editor_GD::flip() is used
  3. If the original image contains an EXIF user comment tag (0x9286), the value of that tag is not preserved. Why? Because GD overwrites it with a string like "CREATOR: gd-jpeg v1.0 (using IJG JPEG v90), quality = 82." Should we worry about that?

TODOs

  1. it might be good to add more inline documentation explaining how the binary EXIF manipulation is done
    1. there is some, but more would be better.
    2. for those that don't know, the binary EXIF data is actually TIFF data. Yeah, I know, it's weird that TIFF data is embedded within JPEG data. Even weirder is that TIFFs can contain embedded JPEGs (which can contain embedded TIFF data, etc) :-)
    3. TIFF is pretty complicated so more explanation would probably be helpful for future maintanence of the code
  2. the code that finds the EXIF orientation tag in the binary EXIF data is "technically" incomplete. It currently only looks in the first TIFF IFD (Image File Directory) for the orientation tag. TIFF IFDs can contain "pointers" to other IFDs elsewhere in the data (sometimes called "sub IFDs"). All of the JPEG images I have to test with have the orientation tag within the first IFD (and I don't think I've seen any images in the real world that have it in a "sub IFD", but they do exist). So, I didn't write the code to follow those pointers to sub IFDs...since I wouldn't have any way to test it. So, if anyone has any JPEGs that store the EXIF orientation tag in a sub IFD, please add a few of them as attachments to this ticket and I'll update the patch accordingly
  3. as mentioned above, there is a slight "quirk" when a JPEG is uploaded using the block editor (as opposed to in the Media Library or classic editor "Add Media"). If the image has an EXIF orientation tag that indicates the image is rotated, the image momentarily displays in the rotated orientation in the block editor but then quickly corrects itself. I think this quirk is a result of how the block editor uses WP_REST_Attachments_Controller and someone who knows that REST API endpoint (and how the block editor uses it) better than I will have to fix that
  4. ICC Profiles (APP2 JPEG segment) can span more than 1 application segment (since segments are limited to 64K bytes and some profiles can be longer than that). I've never seen that in the real world, so I don't know how it is accomplished (e.g., does the APP2 segment contain a pointer to where the additional segment where the rest of the profile is stored?) and haven't accounted for it in this patch. See Annex B.4 of Specification ICC.1:2010
  5. write unit tests

@pbiron
5 years ago

EXIF and IPTC metadata from CarolinaRaptorCenter_2005_0132.jpg. This was generated with exiftool and can be used to test that these metadata are preserved by my patch.

#72 @pbiron
5 years ago

Yes, CarolinaRaptorCenter_2005_0132.jpg is the image my gravitar comes from. I'm not just a WP guy, I'm also a nature photographer :-)

#73 @pbiron
5 years ago

I've also a couple other sample images uploaded to a site I developed/managed...which brought this issue to my attention. I've asked the user who uploaded them (and owns the copyright) if I can share them here. When I get that permission I will add them as attachments.

They were shot on an iPhone (oriented the wrong way, of course) and contain EXIF but no IPTC nor ICC Profile segments.

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


5 years ago

@pbiron
5 years ago

Sample image with EXIF orientation tag value that requires rotation on upload. In addition to EXIF, it also contains IPTC and ICC Profile metadata, to test that part of my patch.

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


5 years ago

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


5 years ago

#77 follow-up: @azaozz
5 years ago

Replying to pbiron:

Thanks for the patches and the detailed explanation! :)

Let me try to answer the questions:

If the original image does not contain the EXIF orientation tag, should we add it when images are rotated and saved? I don't think so, but thought I'd ask.

You're right, we shouldn't.

If an uploaded image has an EXIF orientation of anything other than "normal" (e.g., 2, which means "flipped"), should we modify the image accordingly on upload?

Yes. Thinking it is best to support all "orientations" that are possible in EXIF, including "flip" and "mirror".

If the original image contains an EXIF user comment tag (0x9286), the value of that tag is not preserved. Why? Because GD overwrites it with a string like "CREATOR: gd-jpeg v1.0 (using IJG JPEG v90), quality = 82." Should we worry about that?

Thinking "no", at least not in this ticket. This can be handled in another ticket if deemed necessary.

Going through all previous comments, my opinion is that this patch shouldn't introduce preserving of EXIF data in the images produced by GD. There seems to be a possibility of corrupting the rotated image file if we "patch it" on binary level.

Unfortunately there is no PHP extension/library that can (100% reliably) write EXIF in JPEGs. Thinking it is better to avoid any possibility of corrupting the user's images rather than try to do that by writing directly to the files. This is also consistent with the current behaviour.

Of course it will preserve the EXIF when using ImageMagick. Thinking we may also need to fix the image dimensions in the EXIF when we change it from portrait to landscape, or the other way round. Not sure how useful it would be.

This somewhat depends on #47873 but thinking we can do the same for images that need rotation, i.e. create a new rotated image and use it as the "full" size in WP, but keep the original image in case the user wants to download it later. Also generally images that need rotation are photos that will also need the "Big image" treatment.

In that terms thinking we should concentrate on how exactly to do the rotating:

  • Make a new "max-size" rotated image and then create all sub-sizes from it.
  • Rotate the (whole) original image and then make all sub-sizes (in one go).

In both cases the rotation will have to be done while making sub-sizes (use the same source) so it uses less resources.

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

#78 @azaozz
5 years ago

as mentioned above, there is a slight "quirk" when a JPEG is uploaded using the block editor (as opposed to in the Media Library or classic editor "Add Media"). If the image has an EXIF orientation tag that indicates the image is rotated, the image momentarily displays in the rotated orientation in the block editor but then quickly corrects itself.

That happens as when uploading from the Image block, there is a "preview" of the image (shown from js) while the actual image is being uploaded. That preview doesn't follow the EXIF orientation (or rather the browser doesn't). After the image has finished uploading, and sub-sizes were created, the preview is replaced with the "proper" source.

Thinking this can be fixed there, but not very straightforward. Would need to read EXIF from js, then rotate the image before displaying the preview. This may need quite a bit of memory (in the browser) but will probably be fine.

#79 @azaozz
5 years ago

  • Milestone changed from Future Release to 5.3

#80 in reply to: ↑ 77 ; follow-up: @pbiron
5 years ago

Replying to azaozz:

If an uploaded image has an EXIF orientation of anything other than "normal" (e.g., 2, which means "flipped"), should we modify the image accordingly on upload?

Yes. Thinking it is best to support all "orientations" that are possible in EXIF, including "flip" and "mirror".

I think so as well, but stated it as a question to avoid expanding the scope of the ticket in my patch.

Going through all previous comments, my opinion is that this patch shouldn't introduce preserving of EXIF data in the images produced by GD. There seems to be a possibility of corrupting the rotated image file if we "patch it" on binary level.

I was going off what @mikeschroder said in #55, which I interpreted as his opinion that we should not do rotation (or flip/etc) on upload if we couldn't also preserve EXIF (and/or IPTC) in GD.

Unfortunately there is no PHP extension/library that can (100% reliably) write EXIF in JPEGs. Thinking it is better to avoid any possibility of corrupting the user's images rather than try to do that by writing directly to the files. This is also consistent with the current behavior.

I spent a little time trying to put a WP core wrapper around the Pel library, for general EXIF/IPTC editing within core. But quickly decided it wasn't worth it (at least for now, especially considering the effort that would be necessary to do the UI for such an editor).

But my implementation of writing the binary orientation tag in WP_Image_Editor_GD` is heavily influenced by the way Pel does it.

What are the chances of WP putting some pressure upstream to get GD to preserve EXIF/IPTC/etc?

Of course it will preserve the EXIF when using ImageMagick. Thinking we may also need to fix the image dimensions in the EXIF when we change it from portrait to landscape, or the other way round. Not sure how useful it would be.

I don't think that's necessary...the dimensions stored in the original image assume the orientation has been applied.

Another thing I brought up in #core-media chat a few weeks ago was:

suppose you've got an oriented image with an embedded thumbnail. You upload it, it gets rotated and intermediate sizes are created (after rotation). If you then download the files from the upload dir to a Windows machine and look at them in Explorer, you'll notice that some of them are oriented correctly and others are not. Because depending on the size of the embedded thumbnail, Explorer displays it

So, when images are rotated/flipped/etc, should we extract the embedded thumbnail, rotate/flip/etc it and re-embed it? This applies whether the image is rotated/flipped/etc on upload or done interactively by a user. 99% of users will never noticed, but just thought I'd mention it.

@pbiron
5 years ago

example of Windows Exploring using the unrotated embedded thumbnail for an image that has be rotated on upload

#81 in reply to: ↑ 80 @azaozz
5 years ago

Replying to pbiron:

I was going off what @mikeschroder said in #52, which I interpreted as his opinion that we should not do rotation (or flip/etc) on upload if we couldn't also preserve EXIF (and/or IPTC) in GD.

Right, that's in case we overwrite the original uploaded image. I'm thinking we should keep the original but not use it on the front-end when we do the rotation.

Also these images will generally be "big" by both file size and dimensions and not suitable for use on the internet (as they would be unedited photos). #47873 is for implementing the same handling for all of them: keep the original but not use it in WP.

I spent a little time trying to put a WP core wrapper around the Pel library, for general EXIF/IPTC editing within core. But quickly decided it wasn't worth it

Yeah, thinking if we want to use Pel, it would need a new ticket (and quite a bit of testing).

What are the chances of WP putting some pressure upstream to get GD to preserve EXIF/IPTC/etc?

I'd say not very likely :) Even if that's added it would take quite a bit of time until it gets in PHP and WP is able to use it.

So, when images are rotated/flipped/etc, should we extract the embedded thumbnail, rotate/flip/etc it and re-embed it? This applies whether the image is rotated/flipped/etc on upload or done interactively by a user. 99% of users will never noticed, but just thought I'd mention it.

Hm, think the embedded thumbnails were removed/not copied to the sub-sizes (or there was a ticket for that?). This is a basic optimization for web use, would be good to do it.

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


5 years ago

#83 @archon810
5 years ago

Giving https://wordpress.org/plugins/fix-image-rotation/ a try since this 9-year-old bug is still unresolved.

Edit: Seems to do the job. Here is a comparison of before vs after:

https://i.imgur.com/tTyAdQc.png

Last edited 5 years ago by archon810 (previous) (diff)

#84 @azaozz
5 years ago

With #47872 and #47873 already in, thinking the way forward with EXIF rotations in finally "open". Thinking it would be good to summarize all options :)

The "base requirement" is that the original image should be unchanged. It would be ideal if we can always write the EXIF data after rotating the image.

Doing that from PHP by using PEL (or a subset if it) seems the best option. However that would require a lot of non-trivial testing (i.e. write EXIF data in PHP then check if the image file is not corrupted in any way), especially with old, possibly obscure image files. We can't afford corrupting an image while processing it after upload, no matter what :)

The alternative is to rotate the original image only when ImageMagick is available and it can write EXIF, see the part of [40135] with is_callable( setImageOrientation ) ....

Then the "safest", most compatible option is to just leave the original image as-is and only rotate sub-sizes.

A typical case:

  • User uploads an image with EXIF Orientation != 1.
  • The first thing that happens after the attachment post is created is the check for "BIG" image, #47873. By that time the EXIF data is already retrieved. Then:
    • If the image is big, scale it down and rotate it.
    • If the image is not big, or if scaling of big images is disabled (by a plugin), rotate it and save a copy. This will use the same way of keeping the original image as with big images.
  • Image sub-sizes are created from the original image for best quality. The make_subsize() method in the image editor can be extended to look into EXIF and rotate the image before or after resizing.

There are couple of variations. Generally it may be faster (use less resources) if each sub-size is rotated after the resizing. Of course that would depend on how many sub-sizes are being created. However this would not work for cropped images, the source would have to be rotated before cropping.

Seems best to stick to rotating the source before resizing and then creating all sub-sizes from the rotated source.

On the implementation side, thinking WP_Image_Editor will need a maybe_exif_rotate() method that is called before any (batch) cropping or resizing is done with make_subsize(), and possibly before resize().

@azaozz
5 years ago

#85 @azaozz
5 years ago

  • Keywords 2nd-opinion added

In 14459.4.diff:

  • Rotate the new "full" size image when the original uploaded image is "big".
  • If not, and there is EXIF Orientation, make a copy of the original image and rotate it. Then use it as "full" size.
  • Rotate the image source before making sub-sizes.

The patch handles all possible EXIF Orientation scenarios: flipped and/or rotated. May be a bit rough-around-the-edges at the moment but seems to work well here. May need some optimization especially in cases where the big image functionality is disabled (by returning false from the big_image_size_threshold filter).

Pleas stress-test it!

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

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


5 years ago

#87 @kirasong
5 years ago

  • Keywords 2nd-opinion removed
  • Status changed from reviewing to accepted

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


5 years ago

@azaozz
5 years ago

#89 @azaozz
5 years ago

In 14459.5.diff:

  • Some cleanup and a bit better names.
  • Add wp_image_maybe_exif_rotate filter so themes and plugins can prevent rotating the image or correct the retrieved EXIF Orientation value (if they know better).

@pbiron @mikeschroder more testing very much appreciated :) Thinking to commit this tomorrow.

#90 @pbiron
5 years ago

#39957 was marked as a duplicate.

#91 follow-up: @pbiron
5 years ago

After giving 14459.5.diff some thorough testing, I think this is ready to commit! Yeah!

#92 in reply to: ↑ 91 @azaozz
5 years ago

Replying to pbiron:

Thanks for testing!

#93 @azaozz
5 years ago

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

In 46202:

Media/Upload: rotate images on upload according to EXIF Orientation.

Props msaggiorato, wpdavis, markoheijnen, dhuyvetter, msaggiorato, n7studios, triplejumper12, pbiron, mikeschroder, joemcgill, azaozz.

Fixes #14459.

#94 @archon810
5 years ago

Hooray! Landing in 5.3 on Nov 12, right?

#95 @kirasong
5 years ago

  • Keywords needs-dev-note added

This ticket was mentioned in Slack in #cli by pbiron. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by azaozz. View the logs.


5 years ago

#98 follow-up: @tmatsuur
5 years ago

Is this thread already closed?

In an environment where GD is used, JPEG files could not be rotated.

This is due to improper processing of JPEG images that do not support the alpha channel.

class-wp-image-editor-gd.php: 344 (5.3 Beta 2)

public function rotate( $angle ) {
	if ( function_exists( 'imagerotate' ) ) {
		$transparency = imagecolorallocatealpha( $this->image, 255, 255, 255, 127 );
		$rotated      = imagerotate( $this->image, $angle, $transparency );

		if ( is_resource( $rotated ) ) {
			imagealphablending( $rotated, true );
			imagesavealpha( $rotated, true );
			imagedestroy( $this->image );
			$this->image = $rotated;
			$this->update_size();
			return true;
		}
	}
	return new WP_Error( 'image_rotate_error', __( 'Image rotate failed.' ), $this->file );
}

I confirmed that the JPEG image can be rotated normally by correcting as follows.

public function rotate( $angle ) {
	if ( function_exists( 'imagerotate' ) ) {
		if ( 'image/png' === $this->mime_type || 'image/gif' === $this->mime_type ) {
			$transparency = imagecolorallocatealpha( $this->image, 255, 255, 255, 127 );
			$rotated      = imagerotate( $this->image, $angle, $transparency );

			if ( is_resource( $rotated ) ) {
				imagealphablending( $rotated, true );
				imagesavealpha( $rotated, true );
				imagedestroy( $this->image );
				$this->image = $rotated;
				$this->update_size();
				return true;
			}
		} else {
			$rotated      = imagerotate( $this->image, $angle );
			if ( is_resource( $rotated ) ) {
				$this->image = $rotated;
				$this->update_size();
				return true;
			}
		}
	}
	return new WP_Error( 'image_rotate_error', __( 'Image rotate failed.' ), $this->file );
}

The PHP version that confirmed this behavior is 7.2.3.

#99 in reply to: ↑ 98 @azaozz
5 years ago

Replying to tmatsuur:

Created #48202 to track this. Please test the patch there :)

#100 @desrosj
5 years ago

  • Keywords has-dev-note added; needs-dev-note removed

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


4 years ago

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


3 years ago

Note: See TracTickets for help on using tickets.