[wp-trac] [WordPress Trac] #14459: Rotate Full Size Images on Upload

Sun Jul 28 20:32:35 UTC 2019

#14459: Rotate Full Size Images on Upload
-------------------------------------------------+-------------------------
 Reporter:  mrroundhill                          |       Owner:
                                                 |  mikeschroder
     Type:  enhancement                          |      Status:  reviewing
 Priority:  normal                               |   Milestone:  Future
                                                 |  Release
Component:  Media                                |     Version:  3.0
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch needs-testing needs-unit-  |     Focuses:
  tests                                          |
-------------------------------------------------+-------------------------
Changes (by pbiron):

 * keywords:  has-patch => has-patch needs-testing needs-unit-tests

Comment:

 Replying to [comment:34 n7studios]:
 > Patch:
 > https://gist.github.com/n7studios/6a764d46bc1d515ba406
 >
 > 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:
 > {{{#!php
 > $exif_data = str_replace( chr( dechex( $original_orientation ) ) , chr(
 0x1 ), $exif_data );
 > }}}

 [https://core.trac.wordpress.org/attachment/ticket/14459/14459.3.diff
 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
    a. the function hooked is in `/wp-includes/media.php`, the hook is
 added in `/wp-includes/default-filters.php`
    b. 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
    c. see TODO below for one quirk about support in the block editor

 2. adds methods to `WP_Image_Editor_GD` that:
    a. 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)
       i. note that `WP_Image_Editor_Imagick` handles this automatically,
 because Imagick preserves those segments
       ii. 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.
       iii. the JPEG application segments are copied from the original
 image to **any** image the editor saves, e.g., itermediate sizes, crops,
 etc.
    b. 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)
       i. note that `WP_Image_Editor_Imagick` handles this automatically,
 thanks to [https://core.trac.wordpress.org/changeset/40123 [40123]]
       ii. if the original image did not contain the EXIF orientation tag
 then this step is not done (see Questions below)
       iii. 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)
       iv. 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?
    a. all the previous patches on this ticket and the gist from @n7studios
 are limited to rotation
    b. the scope of this ticket is just about rotation, but I think we
 should definately consider flipping on upload as well (see
 [https://core.trac.wordpress.org/attachment/ticket/37140/orientation-2.jpg
 orientation-2.jpg] for an example)
    c. 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
    a. there is some, but more would be better.
    b. 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) :-)
    c. 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
 [http://www.color.org/specification/ICC1v43_2010-12.pdf Specification
 ICC.1:2010]
 5. write unit tests

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/14459#comment:71>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform