Wrong image rotation #21485

Closed
massa007 opened this Issue Jan 6, 2016 · 51 comments

Projects

None yet
@massa007
massa007 commented Jan 6, 2016

Hi all,

I want to refer to this issue: interfasys/galleryplus#29
I have the same problem and it seems there is no open issue yet.

Pictures are wrong rotated in gallery, as well as thumbnails and file explorer in owncloud.
If I download the pictures it´s all fine and my windows app shows correctly.

20151229_141403

Operating system: Ubuntu Server 14.04

Web server: nginx current version

Database: MySQL

PHP version: Current version, exif enabled

ownCloud version: (see ownCloud admin page): 8.2.1

Updated from an older ownCloud or fresh install: fresh installation

@Xenopathic
Member

When you rotated the image, it added metadata that tells viewers the image should be rotated: http://regex.info/exif.cgi?dummy=on&imgurl=https%3A%2F%2Fcloud.githubusercontent.com%2Fassets%2F16576171%2F12143152%2F8e2892d8-b47e-11e5-9e74-7bfc2b1c5053.jpg (see EXIF Metadata: Orientation: Rotate 90 CW)

We should detect such metadata in preview generation, but there's not much we can do about displaying it in the browser if the browser ignores the metadata...

@massa007
massa007 commented Jan 6, 2016

Just for your info, there is an owncloud clone used by some universities in germany. It is named sciebo. It´s nearly the same as original owncloud just with minor changes. So they used owncloud source for developing their own version.

The picture above was shared within sciebo by one of my friends and i synchronized it directly with my owncloud. In sciebo the rotation is correct but in my owncloud it is wrong.
So there must be something that the clone is doing better than my owncloud.

@MorrisJobke
Member

In sciebo the rotation is correct but in my owncloud it is wrong.
So there must be something that the clone is doing better than my owncloud.

Weird - I don't think they change such internals, but I will ask them.

Back some time I fixed the rotation of the previews and it worked just fine with a lot of test images. See #13448

Maybe also try the original gallery app - that's also what is very likely used at sciebo: https://github.com/owncloud/gallery

I can't reproduce the rotation issues on my local owncloud. Maybe you have an old version of the gd/imagick php module.

@ikeewee
ikeewee commented Jan 8, 2016

Hi,

I'm not sure if this is relevant but I have uploaded an image created on an iPhone 5S to one ownCloud account and it displays correctly in the image gallery however if I share that image to a second account on the same ownCloud server the image displays with incorrect rotation

@oparoz
Contributor
oparoz commented Jan 8, 2016

Thanks @ikeewee .

Could one of you write down a 100% reproducible test case? Preferably one which doesn't involve Gallery, so no full screen preview before sharing per example.

Something like:

  1. upload
  2. thumbnail in Files is OK
  3. share
  4. open 2nd instance
  5. thumbnail is wrong

but more detailed if it needs to be.

@ikeewee
ikeewee commented Jan 8, 2016

These are the steps I can use to reproduce it here. This is on a recent fresh install of ownCloud 8.2.2 running on Linux Mint 17.2

  1. Take photo on iPhone and select the ownCloud app from the iPhone's share menu. App opens, choose Photos folder as destination and wait for photo to sync to server.
  2. Fire up Chrome and log in to ownCloud using first account. Select the Photos folder, thumbnail is correct. Click the grey share link next to the name of the file and share with second account. Log out
  3. Log in to second account. The shared file is in the root folder and the thumbnail is rotated 90 degrees to the left

This also happens if I share the photo directly from within the iOS app

@MorrisJobke MorrisJobke added bug and removed needs info labels Jan 8, 2016
@massa007
massa007 commented Jan 8, 2016

In my case it is very simple:
1: Take a picture with iPhone 6s in upright mode (I hope "upright" is the correct word - home button is at the bottom)
2: Upload picture with any app (in my case Crypto Cloud)
3: Picture is wrong rotated in any view

But I think it is reproducible with any smartphone

First one in horizontal mode
second upright
rotation

@MorrisJobke
Member

@massa007 Can I ask you to provide those images to us for debugging purposes? That would be helpful :)

@massa007
massa007 commented Jan 8, 2016

Of course :)
Thank you already for your help guys!

2016-01-08 13-30-12 0183
2016-01-08 13-30-16 0184

Not sure if only file name is changed when its uploaded here, just tell me if you need my original files in case they are somehow changed within github

@MorrisJobke
Member

Reproduction steps

  • upload those files
  • share the two files individually to another user -> everything works
  • create a folder, put those two files in there and then share the folder to another user -> preview is not rotated
@massa007
massa007 commented Jan 8, 2016

I fear in my case it is always wrong ^^

@oparoz
Contributor
oparoz commented Jan 8, 2016

I suspect that people affected by the general issue don't have the EXIF extension installed... but @MorrisJobke's 2nd use case is probably a different issue 😢

@oparoz
Contributor
oparoz commented Jan 8, 2016

@massa007 Check if the extension is really installed since it seems to fail from the start for you.
For the other reporters, the extension has to have been installed since it works the first time a preview is loaded

@massa007
massa007 commented Jan 8, 2016

I checked with the phpinfo site. Can you tell me how to really check it?

@oparoz
Contributor
oparoz commented Jan 8, 2016

Hmmm... The only proper way would be through a test script. I'm sure someone has written one which list the metadata and published it on the Internet.

@oparoz
Contributor
oparoz commented Jan 11, 2016

This is actually a duplicate of #20484, so closing this.

@oparoz oparoz closed this Jan 11, 2016
@massa007

I Inserted the php code in the corresponding php file with no change..
Do I need to do anything more?

@Rayn0r
Rayn0r commented Jan 27, 2016

I don't think that this issue is a duplicate of #20484.
Issue #20484 is about re-shared images.
The patch introduced there does not fix anything for me. I am currently running oc 8.2.2.2 on my server.

All portrait images I ever uploaded via owncloud app from my Android phone are displayed in landscape mode.
The problem first occurred after upgrading from 8.1.4.2 to 8.2.1.4. It seems like the exif information is not taken into account when displaying the image.

@oparoz oparoz reopened this Jan 27, 2016
@Rayn0r
Rayn0r commented Jan 27, 2016

I have to correct myself. Not all photos are affected.
I just stumbled upon an image that (at least here) does display correctly:
img_20150518_091909
While this one does not:
img_20160126_085118

When opened from the HDD in XnView or Firefox both images are displayed correctly.

@oparoz
Contributor
oparoz commented Jan 27, 2016

I've just tested both of these images on master and they both get the proper orientation.

Don't forget that you have to clear the cache in order to get the new thumbnails generated.

@Rayn0r
Rayn0r commented Jan 28, 2016

I deleted all thumbnails for the particular user. OC then re-created them. Here are the two newly created files from the thumbnail folder for the second image:
2000-1500-with-aspect png
2048-1536-max png

@Rayn0r
Rayn0r commented Jan 28, 2016

I turned on debug for my OC installation and then again deleted all thumbnails.
The log now shows the following when accessing an image in the gallerie.
I extracted only the "message" part from each log entry:

,"message":"fopen(/usr/share/owncloud-data/username/thumbnails/73401/2000-1500-with-aspect.png): failed to open stream: No such file or directory at /usr/share/owncloud/lib/private/files/storage/local.php#247"
"message":"Generating preview for "Photos/IMG_20160126_085118.jpg" with "OC\Preview\JPEG""
"message":"OC_Image->fixOrientation() No readable file path set."
"message":"OC_Image->fixOrientation() Orientation: -1"

Because of the missing "file path" the orientation is set to -1.
So where should "file path" point to and where does its value come from?

@oparoz
Contributor
oparoz commented Jan 28, 2016

Iirc, there is already an issue opened about that problem. Try to look for it and if you find a match, close this one :)

@Rayn0r
Rayn0r commented Jan 28, 2016

I was not able to find an issue that seemed related to this problem... So I did some more digging.

I changed line 364 of /usr/share/owncloud/lib/private/image.php to:
$this->logger->debug('OC_Image->fixOrientation() No readable file path set. FilePath: '.$this->filePath, array('app' => 'core'));
The log now states:

"message":"OC_Image->fixOrientation() No readable file path set. FilePath: /tmp/oc_tmp_NZwlxY-.jpg"

Using inotifywait -m -r -o /root/oc.log /tmp/ I can see that the file is being created, modified, accessed and at some point deleted.
Why does the above function claim, that the filePath is not set?
Could it be a threading problem, where the file is being deleted by another thread, before it can be read by fixOrientation?

@Rayn0r
Rayn0r commented Jan 28, 2016

There is one more odd thing...
After adding some more logging I figured out that the function "cleanFiles" in lib/private/tempmanager.php only cleans a file called /tmp/oc_tmp_NZwlxY but not /tmp/oc_tmp_NZwlxY-jpg.
I was not yet able to identify the function which cleans up the temp jpg file? Can anyone help me out here?

@oparoz
Contributor
oparoz commented Jan 28, 2016

It seems to be specific to your installation. Maybe PHP runs out of memory?
I've just tested on 8.2.2, without encryption and I get the proper orientation.

@Rayn0r
Rayn0r commented Jan 28, 2016

I'm sorry, but there is one thing I missed to mention. Encryption is enabled here...

How would PHP running out of memory delete a file under /tmp prior to being access by fixOrientation?
The temporary jpg is contained in the array $files that is passed to the function cleanFiles in lib/private/tempmanager.php. The jpg file does however not exist anymore.

@oparoz
Contributor
oparoz commented Jan 28, 2016

How would PHP running out of memory delete a file under /tmp prior to being access by fixOrientation?

I was thinking that maybe the cleaning operation is kept separate.

I'm sorry, but there is one thing I missed to mention. Encryption is enabled here...

And that's the reason it doesn't work...

@icewind1991, can this be fixed like shared storage was fixed?

@massa007

Rayn0r I´m glad that you have the same issue but know much better about the system in general... I´m not able to dig deeper in this, but please, if you find any solution, write it here!
FYI: Encryption is enabled here as well

@Rayn0r
Rayn0r commented Feb 1, 2016

Over the past couple of days I tried to find the cause for this issue, but I ran out of ideas where to look in the source and what exactly to search for.
I am still confused by the fact, that the missing temp image used by fixOrientation() did not have seemed to be deleted by the cleanFiles() function.
It would be really great if someone with deeper insight could take a look at this.

@versatileng

I'm facing the same problem:
All images uploaded before I enabled encryption are (still) rotated properly.
All images uploaded/added after I enabled encryption are not rotated at all - rotation is also not done in thumbnails - I disabled all gallery-apps for testing, no rotation, nowhere.
If I download a not properly rotated image, EXIF information is still here, image is rotated correclty by viewers and browsers.
Since I'm using OC for picture gallaries only I would really appreciative any hint or help!

@oparoz oparoz added regression and removed junior job labels Feb 4, 2016
@oparoz oparoz added this to the 9.0-current milestone Feb 4, 2016
@cmonteroluque cmonteroluque modified the milestone: 9.1-next, 9.0-current Feb 22, 2016
@snoopyf snoopyf referenced this issue in owncloud/gallery Mar 31, 2016
Closed

Images on ext. stor.: image rotation corrupt #637

@snoopyf
snoopyf commented Mar 31, 2016

Hi everybody,
stumbled across this issue as I noticed some similar behaviour... as described here:
owncloud/gallery#637

In my case image rotation is corrupted when images are stored in external storages (CIFS/SMB for example). When the same images are uploaded to a "local" folder, everything seems to be fine (for more details have a look at the linked issue)

@josh4trunks
Contributor

I believe this issue started happening in version 8.2.0 though I might have been somewhere else between 8.0-8.2. I've been updated my owncloud, with encryption enabled, since 7.0.0 and started noticing it at one point but never posted a bug report.

With encryption enabled I am seeing the ignoring of EXIF rotation of JPG thumbnails in the files app and the slideshow from the gallery app when you click on a JPG.

@josh4trunks
Contributor

I believe I was able to fix this issue in my testing. Both thumbnails and gallery previews are now respecting exif data for my encrypted files. =]

In https://github.com/owncloud/core/blob/master/lib/private/preview/image.php I moved $image->fixOrientation(); directly below $image->loadFromFile($fileName);
Can someone test this doesn't brake anything and if feel like it write a PR?

Now that thumbnail generate properly is there a proper way to delete my old thumbnails so owncloud generate all new ones? Should I just be clearing the thumbnail folders for all users?
rm -r DATADIR/*/thumbnails/*
Thanks

@josh4trunks
Contributor

@oparoz @cmonteroluque if the fix mentioned here #21485 (comment) works could we backport into 9.0.X? It's a minor bug fix so I don't see an issue?
Thanks

@Rayn0r
Rayn0r commented Mar 31, 2016

So you changed it like this?

--- image.php.org       2016-03-31 12:09:28.431677652 +0200
+++ image.php   2016-03-31 12:09:42.071685741 +0200
@@ -53,10 +53,10 @@
                        $fileName = $fileview->getLocalFile($path);
                }
                $image->loadFromFile($fileName);
+               $image->fixOrientation();
                if ($useTempFile) {
                        unlink($fileName);
                }
-               $image->fixOrientation();
                if ($image->valid()) {
                        $image->scaleDownToFit($maxX, $maxY);

@josh4trunks
Contributor

@Rayn0r exactly
Though I believe we need to clear our old thumbnails to get those to regenerate. Not sure if just removing them is safe, or if there's a better way.

@Rayn0r
Rayn0r commented Mar 31, 2016

It looks like the behaviour was introduced by this commit a281737.

Cleaning up the thumbnail directory solves it for me.

@josh4trunks Thanx for the fix. You are my hero!

@oparoz
Contributor
oparoz commented Mar 31, 2016

Good job @josh4trunks :). That would explain why every backend except local storage can't rotate images. It's a bit surprising since we load the temp file into a PHP resource before deleting it, so it shouldn't have any impact.

scaleDownToFit should be tested, because it could very well be that it's affected by the same problem and that the real fix is to unlink the file at the very end.

@josh4trunks
Contributor

Thanks
Probably best to the unlinking last just in case.

@oparoz
Contributor
oparoz commented Mar 31, 2016

Now that thumbnail generate properly is there a proper way to delete my old thumbnails so owncloud generate all new ones? Should I just be clearing the thumbnail folders for all users?

@josh4trunks - There is one project to do exactly that: owncloud/gallery#22 , but it stalled due to the lack of funding, so yes, you can just delete that thumbnails folder and generate them again while browsing.

@josh4trunks
Contributor

Sounds good. Personally I'll wait to delete my thumbnail folders once this gets merged.

@snoopyf
snoopyf commented Mar 31, 2016

Thanks a lot to josh4trunks for this:

So you changed it like this?

--- image.php.org       2016-03-31 12:09:28.431677652 +0200
+++ image.php   2016-03-31 12:09:42.071685741 +0200
@@ -53,10 +53,10 @@
                      $fileName = $fileview->getLocalFile($path);
             }
               $image->loadFromFile($fileName);
+               $image->fixOrientation();
              if ($useTempFile) {
                       unlink($fileName);
               }
-               $image->fixOrientation();
               if ($image->valid()) {
                       $image->scaleDownToFit($maxX, $maxY);


After changing this and a quick test everything seems to work as expected for me now...

@josh4trunks
Contributor

@oparoz
do you mind submitting a PR for this fix? I'd be great if we could get it in 9.0.1 but maybe it's too late?

@oparoz
Contributor
oparoz commented Apr 2, 2016

@josh4trunks - Probably too late as people still have to test the solution, but just create the PR online, move the unlink to the end. Then ping all the people who have confirmed it fixes things for them.
It might be worth checking if other preview providers aren't affected as well.

@josh4trunks
Contributor

I don't other providers would be affected because nothing else uses exif. k when I have a chance I'll write up a PR

@josh4trunks
Contributor

@oparoz do you know what branch I should work off so this gets backported to 9.0.X?

@oparoz
Contributor
oparoz commented Apr 2, 2016

@josh4trunks Always use master and ask for the backport in the PR.

@josh4trunks
Contributor

@oparoz Great, thanks!

@DeepDiver1975 DeepDiver1975 changed the title from Wrong image rotation to Wrong image rotation Apr 2, 2016
@cmonteroluque
Contributor

Coyping @karlitschek for backport

@karlitschek
Member

nice fix. please backport 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment