Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor: image is not rotated correctly in post editor #313

Closed
designsimply opened this issue Nov 20, 2015 · 21 comments
Closed

Editor: image is not rotated correctly in post editor #313

designsimply opened this issue Nov 20, 2015 · 21 comments
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Feature] Post/Page Editor The editor for editing posts and pages. [Pri] High Address as soon as possible after BLOCKER issues [Type] Bug

Comments

@designsimply
Copy link
Contributor

designsimply commented Nov 20, 2015

From @jeherve

I made no edits to the image, it was automatically backed up from my phone to Google Photos, where I downloaded it, and then uploaded it to the editor from the desktop. It seems like it could be caused by Google Photos. I tried with another portrait picture that didn't go through Google Photos, and it worked properly.

Steps to reproduce:

  1. Open https://wordpress.com/post and select a Jetpack Site
  2. Click on the Insert Media icon
  3. Click on Add New
  4. Find the image in my finder window
  5. Click on it
  6. Watch as the image is being uploaded.

screen shot 2015-08-13 at 3 45 39 pm

1. Click on Insert. 2. Publish post.

Here is how the image looks like in the editor:

screen shot 2015-08-13 at 2 53 56 pm

The problem seems similar for the Featured Image view:

screen shot 2015-08-13 at 2 55 13 pm

From @aduth

The cause appears to be that WordPress strips EXIF rotation data when generating intermediate thumbnail sizes for an image. Since we display the large thumbnail in the editor, it will display in the editor without the proper rotation. On the site, however, Jetpack/Photon instead use the original image passed through Photon and scaled to the desired size. Because it scales the original image, the rotation metadata is correctly preserved.

What are the reasons we shouldn't use Photon URLs for images in the editor? Two I can think of are "What if the site is private?" and "What if the user has disabled Photon?", both of which are testable within Calypso. I could also see an argument being made that we should prefer to use the "original" URL over the Photon URL, though in this case the use of Photon will affect the image's appearance. Should we always use the original (full-size) image in the editor if we determine that the site will use Photon on the front-end?

From @sararosso

Just an update on this. I'm seeing image rotation problems in Chrome but not in Firefox or Safari...
@designsimply designsimply added [Type] Bug [Feature] Post/Page Editor The editor for editing posts and pages. [Pri] High Address as soon as possible after BLOCKER issues [Feature] Media The media screen in Calypso, general media management, or integration with third party media. labels Nov 20, 2015
@designsimply designsimply added this to the Editor: v1.1 milestone Nov 20, 2015
@lancewillett
Copy link
Contributor

Closed https://github.com/Automattic/wp-desktop/issues/147 as a duplicate, raised by @ryanmarkel

Site I'm testing on is ryanmarkel.com (it's hosted on VIP Go as a test site). App version is 1.3.

When uploading images that were taken with my iPhone 6+, the rotation data is not respected when viewing the post within the post editor:

screen shot 2016-04-04 at 13 37 23

The Media Library view displays the image with proper rotation:

r9qqnxtftt

@lancewillett lancewillett changed the title Editor: image is not rotated Editor: image is not rotated correctly in post editor Apr 4, 2016
@rralian
Copy link
Contributor

rralian commented May 5, 2016

@gwwar do think think this one is in your wheelhouse?

@rralian
Copy link
Contributor

rralian commented May 5, 2016

There's also some related background in 8870-gh-calypso-pre-oss

@gwwar
Copy link
Contributor

gwwar commented May 18, 2016

I tested this and confirmed the incorrect behavior in editor with Jetpack sites. Until this is fixed in core, I think it makes sense to use Photon for editor previews if a blog is using Photon for posts.

Editor preview images uses urls like the following:

https://helpingcat.wpsandbox.me/wp-content/uploads/2016/05/DSC_05005.jpg?w=680

While the correct orientation on the post uses urls like:

https://i0.wp.com/helpingcat.wpsandbox.me/wp-content/uploads/2016/05/DSC_05005.jpg?resize=825

@gwwar gwwar removed their assignment Jun 10, 2016
@lancewillett lancewillett removed the [Pri] High Address as soon as possible after BLOCKER issues label Sep 8, 2016
@beaulebens
Copy link
Member

This would have the added benefit (if we use the resize parameter properly, or if we use ?w= on a Photon URL) of loading a more optimized image as well, which would speed things up a bit. The ?w= parameter being added to the "raw" Jetpack URL will do nothing, so a fullsize image will load. Using ?resize=xxx without a "y" dimension (,yyy) will also do nothing.

Photon Docs

@rralian rralian self-assigned this Sep 27, 2016
@artpi
Copy link
Contributor

artpi commented Oct 3, 2016

I did a bit of research on this being fixed in core.

The most relevant thread is this one:
https://core.trac.wordpress.org/ticket/14459

TLDR:

  • On most hosts, the GD library is used for most image operations and that strips EXIF data on any relevant op because its a bit low-level
  • The solution for that would be to properly use EXIF rotation just after upload to properly rotate the base image
  • But for huge images straight from DSLRs on many hosts that makes PHP run out of memory

There is no apparent action to fix that.
The ticket is owned by our own @azaozz - could you pitch in ?

Older ticket:
https://core.trac.wordpress.org/ticket/7042

For me, it seems that counting on this being fixed in core cant keep up with our timeline.
Controbutor @n7studios has developed a plugin that kinda fixes this (although I have not tested it yet) - code here and we could try to go similar route and fix it in Jetpack for non-photon sites.

That would "kinda" fix a bug in core in jetpack instead of core itself and that seems to me like trouble down the road - do we generally even consider it?

@lancewillett lancewillett added the [Pri] High Address as soon as possible after BLOCKER issues label Oct 3, 2016
@artpi artpi assigned artpi and unassigned rralian Oct 4, 2016
@lancewillett
Copy link
Contributor

That would "kinda" fix a bug in core in jetpack instead of core itself and that seems to me like trouble down the road - do we generally even consider it?

We do consider it, carefully commenting the code in question so it can be removed later.

@lancewillett
Copy link
Contributor

@azaozz Can you take a look also?

@azaozz
Copy link
Contributor

azaozz commented Oct 5, 2016

Currently core still depends on the user to rotate the images by hand after uploading. This is (finally) set for fixing in 4.7. The best patch for now is https://core.trac.wordpress.org/attachment/ticket/14459/14459.2.diff. It depends on GD removing the EXIF data from rotated JPEGs. IMagick can write EXIF, so the patch resets/removes "Orientation" after rotating.

The plugin mentioned above is not that reliable. It "hacks" the image files directly (binary) and tries to remove the EXIF orientation. Note the inline comment there:

// @TODO I'm not sure this is the best way of changing the EXIF orientation flag, and could potentially affect
// other EXIF data

@lancewillett
Copy link
Contributor

@azaozz Do you think we should wait until we merge 4.7 to WP.com for this? Or patch / hack it now with a comment?

@azaozz
Copy link
Contributor

azaozz commented Oct 29, 2016

@lancewillett from the latest discussion in the core-media Slack channel, this is probably not going to be in 4.7. The concern is that on shared hosting it will make some uploads fail because it takes too much memory and processor power to rotate a large image, and that when only GD is available on the server, all of the EXIF data will be stripped from the rotated image.

Looking at this ticket again: it is not really a Calypso problem. I'm thinking we can probably implement something like https://core.trac.wordpress.org/attachment/ticket/14459/14459.2.diff as a plugin and use it when uploading from Calypso.

An elegant solution would be to detect this before uploading the image and rotate it and fix the EXIF orientation from JS. This is quite possible in newer browsers, but not sure if it will work for phones, etc. (same problem, high RAM and CPU requirements).

@lancewillett
Copy link
Contributor

CC @aduth for a 2nd opinion on what you think would be the most elegant solution.

@aduth
Copy link
Contributor

aduth commented Nov 7, 2016

The JS solution mentioned by @azaozz could use an approach similar to that in #5020, an unfinished pull request which sought to fix the related #318.

For image rotation after the image uploads, we could use Photon to display the image in the editor, which I believe would show with the correct rotation. See original comment for more details where I mentioned this.

We could also/alternatively apply the CSS image-orientation: from-image style, which still has poor browser support.

@apeatling
Copy link
Member

@aduth How much time do you think it would take to use Photon in the editor like you mention?

@aduth
Copy link
Contributor

aduth commented Dec 12, 2016

@apeatling Might not be too much work. We could probably hook into the plugin we created to artificially downsize images without affecting the saved src, but instead always apply the replacement src (not just for large images) and use Photon:

https://github.com/Automattic/wp-calypso/blob/3238ed8/client/components/tinymce/plugins/media/restrict-size/index.js#L37-L43

11963-gh-calypso-pre-oss

@apeatling
Copy link
Member

Cool, that seems pretty doable. Does anyone on IO have some time to knock that one off? It's one of the top three highest priority issues for Calypso posted by Flow Patrol. It'd be great to close it out.

@aduth
Copy link
Contributor

aduth commented Dec 21, 2016

@apeatling I'll try to take a stab at it, hopefully by end of week, else in the new year.

@aduth
Copy link
Contributor

aduth commented Feb 7, 2017

Apologies for the delay on revisiting this. Shortly after my previous message, I made an attempt to put together a working branch to solve rotation issues based on my thinking that simply passing all images through Photon would be a suitable solution. I encountered a couple issues with this in our resizing utility not being generic enough (related changes in #10264).

#11239 is an attempt at fixing rotation. There are a number of caveats there, including issues with solving rotation for featured image. #11239 only solves the simple case of rotated images in post editor content (not flipped).

@aduth
Copy link
Contributor

aduth commented Mar 20, 2017

From the original issue text, this is now fixed for:

Here is how the image looks like in the editor:

But not:

The problem seems similar for the Featured Image view:

See #11239 for complications around rotation for featured images, specifically around supporting custom thumbnail sizes.

@designsimply
Copy link
Contributor Author

#bug-scrub

Tested and confirmed that inserting images with rotation EXIF metadata into Jetpack sites are now correctly rotated in the post body but are still incorrectly rotated in the featured image area in the Calypso editor:

Video: 1 min

screen shot 2017-04-11 at tue apr 11 4 45 42 pm
Seen at https://wordpress.com/post/alittletestblog.com/13593 running WP 4.7.3 and Jetpack Beta (Build 1736160) c142584 using Chrome 57.0.2987.133 on Mac OS X 10.12.4.

@designsimply
Copy link
Contributor Author

Opened a separate issue in #13145 to address the featured images and closing this issue as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Feature] Post/Page Editor The editor for editing posts and pages. [Pri] High Address as soon as possible after BLOCKER issues [Type] Bug
Projects
None yet
Development

No branches or pull requests

9 participants