Avatar rotation fix for iOS

[modemlooper]

Hey guys,

attached is an edited bp-core-avatars.php

It has a small addition to rotate the image correctly based on exif orientation data. I get asked weekely to fix this and the only way is editing this core file. Once an image hits WP image editor then the orientation data is stripped. So we need to read the raw file before and then add in the rotation to wp_get_image_editor.

In function bp_core_avatar_handle_upload i added an efix variable to read data and then down lower before the file is saved and sized I have image rotate.

Test file and patch it. :D

[DJPaul]

I am assuming this is not a regression.

So, what exactly is the problem; rotated images aren't un-rotated when they appear in the cropper? If that's the case, I agree rotating the image back would be a good enhancement.

[modemlooper]

on iphones and most high end cameras, portrait images are displayed in the wrong orientation. My fix locates the orientation tagged in image meta data and rotates it correctly for display

[modemlooper]

Its not an enhancement. when you try to add an avatar on iOS and use a portrait image it's rotated the wrong way

[boonebgorges]

Am I correct that this is a WordPress bug? I guess I'm OK with doing a band-aid fix for BP avatars, but you should submit a fix upstream as well.

Could you please attach a proper patch file so it'll be easier to see what's going on, especially as we revise the patch? Thanks.

[modemlooper]

WP rejiggles the image meta data into it's own array and drops the efix data which is not need in most uses and is not even included most images. The problem is you have to get efix meta data from raw image so it needs to be read at the upload point. This is actually a bug in mobile safari's upload process. Native apps using iOS media upload switches orientation for you. Apple half assed it.

I uploaded a new bp-core-avatars that suppresses php warning on images without efix data.

[modemlooper]

posting this here for reference in case it's an issue
​http://blog.sucuri.net/2013/07/malware-hidden-inside-jpg-exif-headers.html

[shanebp]

imo. Some kind of orientation handling for avatars is over-due for BP.

5089.1 removes the wp_is_mobile() check.
You don't need to be on a mobile device to encounter exif rotation.

[boonebgorges]

This would be nice to include in 2.1, but I'd like independent verification (ideally across a couple different image sources - standard image upload from digital camera, iOS, Android)

[shanebp]

Sorry, but 5089.1 has major problems... working on a fix.

[shanebp]

5089.2 doesn't rely on an upload dimensions being too big before checking rotation. So resize and rotation are separate adjustments.

It also adds a check re support from the servers' php image library - maybe not required.

I've tested the patch successfully via a laptop and using iPhone-generated images, but independent verification from different image sources is a must.

[r-a-y]

shanebp - Instead of exif_read_data(), can wp_read_image_metadata() be used as an alternative? Might need to use the 'wp_read_image_metadata' filter to fetch the 'Orientation' attribute.

[shanebp]

Maybe is_callable( 'exif_read_data' )) is a better way to check for exif support?
As opposed to wp_image_editor_supports()

[shanebp]

r-a-y, re wp_read_image_metadata and orientation, maybe slated for WP 4.0:
​https://core.trac.wordpress.org/ticket/28916

Meanwhile, yeah we could use the wp_read_image_metadata filter to fetch the 'Orientation' attribute. Thanks for the pointer.
If BP cores prefer that approach, I'll make a new patch.

[shanebp]

Re ​https://core.trac.wordpress.org/ticket/28916 - fixed & closed.

[DJPaul]

@modemlooper does this need fixing still? What needs to be done? Thanks.

[modemlooper]

Let me test latest patch....

Tested, this new uploader is not working properly. Images from camera crash browser and landscape images get cut off. Creating a new ticket as rotation is fixed. go ahead and close this out.

[r-a-y]

does this need fixing still? What needs to be done? Thanks.

This ticket needs a refresh to coincide with ​#WP28916 and the latest changes to BP avatars.

We would still need backward compatibility for WP < 4.0 though since we still support WP 3.8.0 as of BP 2.4.0.

Attached is a first pass at this for trunk. It's basically modemlooper's patch with some mods to support WP 3.8 - WP 3.9.x.

I haven't tested this at all, so mobile devs, please test!

[shanebp]

re 5089.03 patch -
Maybe remove this condition.
if ( wp_is_mobile() ) {

It can be misleading, especially with caching. And why restrict an orientation check?
also - this isn't about just iOS - despite ticket title.

[r-a-y]

Thanks for looking at the patch, shanebp.

Like I said, it's a port of modemlooper's patch, he'll have to tell you why! :) But, I agree in principle.

[shanebp]

I'd guess the conditional was added pro forma. But it isn't really pertinent, imo.

btw - nice backpat in the patch. I didn't know tiff had orientation data. Of course, anyone uploading tiffs should be flogged, imo.

Hopefully, your patch passes muster and the same approach can be used for all instances where image uploads are supported.
Maybe it should be moved up into the parent class: BP_Attachment

[imath]

Wow how could I miss this ticket?? This issue is pretty annoying!!

So first i was able to reproduce. And second we should really fix this for 2.4 imho.

I've been reviewing your patch @r-a-y. Many thanks for it, i think we can improve it a bit.

1. i think an upside down photo could be less than 450px, so we shouldn't wait after the size check to eventually correct the photo orientation.
2. i can understand the wp_is_mobile() check because people often don't use their phone or tablet in the right orientation. But like @shanebp i think we shouldn't presume this can only happen when a mobile upload is done. A good reason to do so is that someone can send you a photo he took upside down of you with his mobile by email, and you could use it from a regular PC.
3. I agree again with @shanebp this trouble is now shared by the Cover Image, so the fix needs to be inside BP_Attachment, so that any class extending it: can use it.
4. I don't think we need to do a WP Version check we can just use ! isset( $meta['orientation'] )
5. I think we can be more direct to fix the backcompat issue with WP < 4.0. A little upper in the code we are using getimagesize() to get the width and height and the third parameter of what this function returns is what WordPress calls $sourceImageType :)

So i'm suggesting 5089.04.patch to fix this issue :

• i've tested it with avatars / cover images
• i've tested it with avatars old UI
• i've run the unit tests under WordPress 3.9.

About the unit tests, i think we should include them even if we need to copy files during it, because i'd feel safer. I suggest to add the upside-down.jpg file to tests/phpunit/assets and use this little file to do our tests. (this upside down image is attached to this ticket).

@shanebp, could you test the patch to confirm it's ok.
If so i'll make a build with it and will update our test drive so that anyone can easily check it's ok using their mobile and turning it round and around before using the camera :)

[imath]

to put in tests/phpunit/assets

[imath]

Ah! trac is checking the orientation exif data :)

If the orientation tag has been edited by trac, you can use this file instead: ​https://www.dropbox.com/s/4266e3ggse2fncr/upside-down.jpg?dl=0

[shanebp]

BP 2.3.3, WP 4.3, Theme 2013

I hacked in the 5089.04 patches for class-bp-attachment.php and class-bp-attachment-avatar.php

I tested the patch just on a laptop using a rotated image as a new avatar.
It worked perfectly.

[ Since I'm not using trunk, I did not test the cover photo patch. ]

I think testing via test drive is essential - perhaps announce it via bpdevel or forum post?

Sure will be nice to get this settled.
It instantly makes the BP Attachments API more viable re custom plugins.

[imath]

Thanks for your feedback.

@mercime posted about the test drive on bedevel already:
​https://bpdevel.wordpress.com/2015/09/23/dev-chat-summary-for-september-16-2015/ (just after "ONGOING")

If you want to test it, just DM me your email using slack, i'll create an account for you :)

I'll update the test drive with the patch tomorrow

[DJPaul]

Quick review:

• phpDoc for new get_image_data() has incorrect @return; it could return a bool as well as an array.
• Are you sure we need to suppress errors on the exif_read_data function call? I can't see any notes on the php.net page explaining why.
• Have you timed how much longer it takes for the tests to run after these new tests? Is it significant? I think the file copying could be slow, especially on a heavily-virtualised environment.
• ABSPATH is set by default with a trailing slash, you don't need one at the start of the rest of the string.

Other than that, I haven't tested it yet, but the code looks fine.

[imath]

Thanks a lot @DJPaul for your feedback.

About exif_read_data i've simply copy/pasted what WordPress was doing in wp_read_image_metadata() but you're right, i think we can remove the suppress errors thing.
So .05 is taking in account your feedback.

I've timed before/and after the 3 new unittests i'm suggesting and it seems the impact is not very significant:

• not multisite: before = 1.17 minutes / after = 1.17 minutes
• multisite: before = 1.59 minutes / after = 1.59 minutes

Maybe we could try to include them and if we see it has a significant impact remove them ? What do you think ?

[DJPaul]

Make it so

[imath]

In 10177:

Add Unit Tests for BP_Attachment_Avatar->shrink() and BP_Attachment_Cover_Image->fit() methods.

See #5089
See #6570

[imath]

In 10178:

Make sure uploaded images are oriented the right way.

When using a mobile device to set an avatar or a cover image, there are cases when the image can be saved upside down. We need to check the exif Orientation of uploaded images and rotate them if needed.

Introduce 2 new methods into the BP_Attachment class:

• BP_Attachment->get_image_data() to get an array of all image datas (including exif datas)
• BP_Attachment->edit_image() to edit the image orientation and/or resize the image.

Use these two new methods within the BP_Attachment_Avatar->shrink() and BP_Attachment_Cover_Image()->fit() methods.

Finally include a unit test for the BP_Attachment->get_image_data() method to check we get the orientation meta for all supported WorPress versions.

Props modemlooper, shanebp, r-a-y, DJPaul

Fixes #5089
See #6570