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

OC_Image doesn't always set imageType correctly #18645

Closed
gvde opened this issue Aug 28, 2015 · 33 comments · Fixed by #34297
Closed

OC_Image doesn't always set imageType correctly #18645

gvde opened this issue Aug 28, 2015 · 33 comments · Fixed by #34297

Comments

@gvde
Copy link

gvde commented Aug 28, 2015

OC_Image has imageType = IMAGETYPE_PNG by default.

Several load functions like loadFromBase64, loadFromData, loadFromFileHandle, load don't (always) set imageType to the correct type of image loaded.

For example, if you load a JPEG with loadFromData() imageType remains on IMAGETYPE_PNG.

Due to this, some other image processing functions don't correctly work or cause errors.

For example you get error "OC_Image->fixOrientation() Image is not a JPEG" if you call fixOrientation() on a JPEG loaded with loadFromData even though it's in fact a JPEG.

imageType should always match the type of image. All functions seems to set mimeType correctly but lack to set imageType...

@karlitschek
Copy link
Contributor

Do you want to open a pull request?

@gvde
Copy link
Author

gvde commented Aug 29, 2015

I don't fully understand how everything in OC_Image works together and there seem to be some more issues to get fixOrientation working. Commit 88db5a365b53e713fe31bcc032fc22e48a56accc sets the imagetype work loadFromData() and loadFromBase64(). It's still missing for load() if it gets passed a resource.
With this commit, fixOrientation fails with "OC_Image->fixOrientation() No readable file path set" if I upload a jpg into a contact. As it doesn't come from a file it looks like a more general issue here how to handle images loaded from other sources than files.
So I think someone who has a better understanding of that should have a look at this as further changes may have implications I don't see...

@PVince81
Copy link
Contributor

@georgehrke @oparoz

@oparoz
Copy link
Contributor

oparoz commented Oct 4, 2015

@gvde The first step would be to list all the methods in image which require a proper imagetype to be set and then the best thing to do is to provide a PR so that we can test various scenarios. The unit/integration tests of Preview should give us a first indication of if there are side-effects.

@oparoz
Copy link
Contributor

oparoz commented Oct 4, 2015

OK, quick assessment.

Imagetype is only set when an image is loaded from file

Affected methods:

  • getOrientation() Should break orientation support
  • preciseResize() Should break transparency support
  • centerCrop()Should break transparency support
  • crop()Should break transparency support

So, it would be good to get a PR which also includes setting the imagetype for resources-

@PVince81 PVince81 added this to the backlog milestone Mar 3, 2016
@PVince81
Copy link
Contributor

PVince81 commented Apr 6, 2017

Please try again with 9.1.4 or 10.0 beta

@PVince81 PVince81 closed this as completed Apr 6, 2017
@zero0cool0
Copy link

Has this issue been fixed in 10.0.9.5? Reason for asking is that EXIF based image rotation is failing for all JPG images in my vanilla 10.0.9.5 installation.
Looking at the code, it seems that fixOrientation is failing because loadFromFileHandle() does not set the imageType but I'm not an expert.

Relevant log file lines...

{"reqId":"BYsRLO4mFkPyHhjYrTDl","level":0,"time":"2018-08-22T20:03:50+02:00","remoteAddr":"172.16.172.83","user":"admin","app":"core","method":"GET","url":"\/owncloud\/index.php\/apps\/gallery\/thumbnails?ids=16&scale=2&square=0&requesttoken=EgooBxMNRR5vUggLO0thImBAA18VGDNhYC0iUhMoeQY%3D%3AuPDdC8tSDkPZbyTRStInvRW43iFfPfOgBf967XtJVkM%3D","message":"Generating preview for \"\/admin\/files\/IMG_1928.JPG\" with \"OC\\Preview\\JPEG\""} {"reqId":"BYsRLO4mFkPyHhjYrTDl","level":0,"time":"2018-08-22T20:03:50+02:00","remoteAddr":"172.16.172.83","user":"admin","app":"core","method":"GET","url":"\/owncloud\/index.php\/apps\/gallery\/thumbnails?ids=16&scale=2&square=0&requesttoken=EgooBxMNRR5vUggLO0thImBAA18VGDNhYC0iUhMoeQY%3D%3AuPDdC8tSDkPZbyTRStInvRW43iFfPfOgBf967XtJVkM%3D","message":"OC_Image->fixOrientation() Image is not a JPEG."} {"reqId":"BYsRLO4mFkPyHhjYrTDl","level":0,"time":"2018-08-22T20:03:50+02:00","remoteAddr":"172.16.172.83","user":"admin","app":"core","method":"GET","url":"\/owncloud\/index.php\/apps\/gallery\/thumbnails?ids=16&scale=2&square=0&requesttoken=EgooBxMNRR5vUggLO0thImBAA18VGDNhYC0iUhMoeQY%3D%3AuPDdC8tSDkPZbyTRStInvRW43iFfPfOgBf967XtJVkM%3D","message":"OC_Image->fixOrientation() Orientation: -1"}

@PVince81
Copy link
Contributor

do you have more details about setup and reproduction steps ? see https://raw.githubusercontent.com/owncloud/core/master/.github/issue_template.md

@PVince81 PVince81 reopened this Aug 23, 2018
@zero0cool0
Copy link

zero0cool0 commented Aug 23, 2018

Steps to reproduce

  1. Install owncloud 10.0.9.5 from scratch
  2. Upload a JPG image which contains EXIF orientation information other than 0°, like the one attached. It doesn't matter if whether through a sync-client or through the web interface.

img_1928

$ exiftool-5.26 IMG_1928.JPG
File Name : IMG_1928.JPG
File Type : JPEG
Orientation : Rotate 180
X Resolution : 72
Y Resolution : 72

Expected behaviour

The thumbnail in the web interface as well as the preview produced by the gallery app should be rotated correctly.

Actual behaviour

JPG images are not rotated at all and appear upside down or rotated by 90° in either direction. I did a very rough call backtrace and see that upon thumbnail generation, fixOrientation() is called by loadFromFileHandle() which does not set the imageType variable like loadFromFile() does, hence fixOrientation() will assume it to be PNG file (the default set in the class) and exit with -1.

Server configuration

Operating system:
Ubuntu 18.04.1

Web server:
Apache 2 bundled with Ubuntu

Database:
MariaDB

PHP version:
7.2.7

ownCloud version: (see ownCloud admin page)
10.0.9.5

Updated from an older ownCloud or fresh install:
Fresh install

Where did you install ownCloud from:
# cat /etc/apt/sources.list.d/owncloud.list deb https://download.owncloud.org/download/repositories/stable/Ubuntu_18.04/ /

Signing status (ownCloud 9.0 and above):

Technical information

The following list covers which files have failed the integrity check. Please read
the previous linked documentation to learn more about the errors and how to fix
them.

Results

  • market
    • EXTRA_FILE
      • README.md

Raw output

Array
(
[market] => Array
(
[EXTRA_FILE] => Array
(
[README.md] => Array
(
[expected] =>
[current] => b53ebb407cb90a5cf9e28463013f38e6aec3d39b4281187366e02b5fd014be144216a088fc50851635acc50cb71c92ad2407b21e8a4f3433b0f73a32ea066c97
)

            )

    )

)

The content of config/config.php:

{
"system": {
"loglevel": 0,
"updatechecker": false,
"instanceid": "ocdp4pbe74dr",
"passwordsalt": "REMOVED SENSITIVE VALUE",
"secret": "REMOVED SENSITIVE VALUE",
"trusted_domains": [
"REMOVED SENSITIVE VALUE"
],
"datadirectory": "/Data/OwnCloud-Data",
"overwrite.cli.url": "REMOVED SENSITIVE VALUE",
"dbtype": "mysql",
"dbname": "owncloud",
"dbuser": "REMOVED SENSITIVE VALUE",
"dbpassword": "REMOVED SENSITIVE VALUE",
"dbhost": "localhost",
"dbtableprefix": "oc_",
"version": "10.0.9.5",
"logtimezone": "Europe/Zurich",
"installed": true,
"mail_from_address": "REMOVED SENSITIVE VALUE",
"mail_smtpmode": "php",
"mail_domain": "REMOVED SENSITIVE VALUE",
"appstore.experimental.enabled": false,
"redis": {
"host": "localhost",
"port": 6379
},
"user_backends": [
{
"class": "OC_User_SMB",
"arguments": [
"localhost"
]
}
],
"enabledPreviewProviders": [
"OC\Preview\PNG",
"OC\Preview\JPEG",
"OC\Preview\GIF",
"OC\Preview\BMP",
"OC\Preview\XBitmap",
"OC\Preview\MP3",
"OC\Preview\TXT",
"OC\Preview\MarkDown",
"OC\Preview\Movie",
"OC\Preview\PDF",
"OC\Preview\Photoshop",
"OC\Preview\Postscript",
"OC\Preview\StarOffice",
"OC\Preview\SVG",
"OC\Preview\TIFF",
"OC\Preview\Font"
],
"theme": "",
"maintenance": false,
"memcache.local": "\OC\Memcache\APCu",
"memcache.distributed": "\OC\Memcache\Redis",
"memcache.locking": "\OC\Memcache\Redis",
"mail_smtpauthtype": "PLAIN",
"mail_smtpauth": 1,
"mail_smtpsecure": "tls",
"mail_smtphost": "REMOVED SENSITIVE VALUE",
"mail_smtpname": "REMOVED SENSITIVE VALUE",
"mail_smtppassword": "REMOVED SENSITIVE VALUE",
"mail_smtpport": "587",
"log_rotate_size": "10485760"
}
}

List of activated apps:

`
Enabled:

  • activity: 2.3.7
  • audioplayer: 2.3.1
  • comments: 0.3.0
  • configreport: 0.1.1
  • dav: 0.3.2
  • duo: 2.5.1
  • federatedfilesharing: 0.3.1
  • federation: 0.1.0
  • files: 1.5.1
  • files_external: 0.7.1
  • files_pdfviewer: 0.9.0
  • files_sharing: 0.10.1
  • files_texteditor: 2.2.1
  • files_trashbin: 0.9.1
  • files_versions: 1.3.0
  • files_videoplayer: 0.9.8
  • firstrunwizard: 1.1
  • gallery: 16.1.0
  • market: 0.2.4
  • notifications: 0.3.4
  • provisioning_api: 0.5.0
  • systemtags: 0.3.0
  • templateeditor: 0.3.1
  • updatenotification: 0.2.1
  • user_external: 0.4
    Disabled:
  • encryption
  • external
    `

Are you using external storage, if yes which one: local/smb/sftp/...
none

Are you using encryption: yes/no
no

Are you using an external user-backend, if yes which one: LDAP/ActiveDirectory/Webdav/...
no

Client configuration

Browser:
Chrome Version 68.0.3440.106 (Official Build) (64-bit)

Operating system:
MAC OS 10.13.6

Logs

{"reqId":"BYsRLO4mFkPyHhjYrTDl","level":0,"time":"2018-08-22T20:03:50+02:00","remoteAddr":"172.16.172.83","user":"admin","app":"core","method":"GET","url":"\/owncloud\/index.php\/apps\/gallery\/thumbnails?ids=16&scale=2&square=0&requesttoken=EgooBxMNRR5vUggLO0thImBAA18VGDNhYC0iUhMoeQY%3D%3AuPDdC8tSDkPZbyTRStInvRW43iFfPfOgBf967XtJVkM%3D","message":"Generating preview for \"\/admin\/files\/IMG_1928.JPG\" with \"OC\\Preview\\JPEG\""} {"reqId":"BYsRLO4mFkPyHhjYrTDl","level":0,"time":"2018-08-22T20:03:50+02:00","remoteAddr":"172.16.172.83","user":"admin","app":"core","method":"GET","url":"\/owncloud\/index.php\/apps\/gallery\/thumbnails?ids=16&scale=2&square=0&requesttoken=EgooBxMNRR5vUggLO0thImBAA18VGDNhYC0iUhMoeQY%3D%3AuPDdC8tSDkPZbyTRStInvRW43iFfPfOgBf967XtJVkM%3D","message":"OC_Image->fixOrientation() Image is not a JPEG."} {"reqId":"BYsRLO4mFkPyHhjYrTDl","level":0,"time":"2018-08-22T20:03:50+02:00","remoteAddr":"172.16.172.83","user":"admin","app":"core","method":"GET","url":"\/owncloud\/index.php\/apps\/gallery\/thumbnails?ids=16&scale=2&square=0&requesttoken=EgooBxMNRR5vUggLO0thImBAA18VGDNhYC0iUhMoeQY%3D%3AuPDdC8tSDkPZbyTRStInvRW43iFfPfOgBf967XtJVkM%3D","message":"OC_Image->fixOrientation() Orientation: -1"}

@PVince81
Copy link
Contributor

Confirmed.

As for gallery, it's strange because someone worked on rotation in recent versions owncloud/gallery#764 and was reported working. I wonder what changed since.

@DeepDiver1975
Copy link
Member

As for gallery, it's strange because someone worked on rotation in recent versions owncloud/gallery#764 and was reported working. I wonder what changed since.

this is a pure client side (in browser) feature - this has no influence on the thumbnail generation

@PVince81
Copy link
Contributor

PVince81 commented Aug 23, 2018

Works fine in 10.0.8, broken in 10.0.9.

Regression introduced through one of these:
541178b
d69e53d
d810963
73a54c1
b09846b
7fe9293

from the thumbnail related rework in #31050.

The code path in question leads to https://github.com/owncloud/core/blob/v10.0.9/lib/private/Preview/Image.php#L49 which does not pass image type. Might need to pre-detect based on extension ?

@PVince81
Copy link
Contributor

seems OC_Image::loadFromData as called by OC_Image::loadFromFileHandle does not set $this->imageType, maybe it should do so based on mime type https://github.com/owncloud/core/blob/v10.0.10/lib/private/legacy/image.php#L598

side note: seems this stores the full picture in memory ??

@PVince81
Copy link
Contributor

I had a local try setting imageType, now it says OC_Image->fixOrientation() No readable file path set.. seems that method expects to have a writable file...

@gramels
Copy link

gramels commented Dec 5, 2018

any chance that this will be fixed soon?
The gallery in 10.0.10 is useless without the thumbnail rotation popper working.
Would be migrating to nextcloud a possible fix?

@PVince81
Copy link
Contributor

PVince81 commented Dec 5, 2018

We're going to address various preview issues as part of this project https://github.com/owncloud/files_mediaviewer. Due to the current workload probably not until beginning next year or so (10.1.1)

@gramels
Copy link

gramels commented Dec 5, 2018

Sorry, but I dont get this.
This bug was introduced with 10.0.10 and it will take now several months to be fixed? Do you expect current users to downgrade again?
A picture gallery is a core feature und working image rotation is a basic.

@PVince81
Copy link
Contributor

PVince81 commented Dec 5, 2018

@gramels we're a small team and we currently have other priorities so we cannot start working on this right away. Fixing this is definitely planned, together with other fixes to improve the preview system in general.

I agree that it is unfortunate that a regression is taking that long to fix. There were changes that needed to be made in the preview system (related to object store support with versioning) so fixing this will not be trivial and might require bigger changes in the preview system. I do hope that said changes will be for the best.

Also note that this is open-source so anyone else can also help fixing this.

@gramels
Copy link

gramels commented Dec 5, 2018

So is the message by the owncloud team to owncloud users not to use the gallery?

Did anybody experience the same issue with nextcloud?
This might be a trigger to migrate.

@gramels
Copy link

gramels commented Dec 5, 2018

migrated to nextcloud
seemed fixed in nc 12 & 13 & 14

@TDannhauer
Copy link
Contributor

TDannhauer commented Jan 19, 2019

I have the same issue. @PVince81 : Any details when it will b e fixed?

@sharidas
Copy link
Contributor

sharidas commented Jan 28, 2019

Analysis

I tried to figure out that there is something wrong with https://github.com/owncloud/core/blob/master/lib/private/legacy/image.php#L386, which is causing the rotation of the image shared #18645 (comment). I will update more regarding the same in this comment. Since this analysis is in the initial stage, the more information found would lead us to the actual problem.

Found a method loadFromFile() in https://github.com/owncloud/core/blob/master/lib/private/legacy/image.php#L495.
If we use this method instead of loadFromFileHandle(), in https://github.com/owncloud/core/blob/master/lib/private/Preview/Image.php#L49, I found the orientation of exif image gets corrected. By uploading a few files, I did not found any impact on performance, as such ( at least notable to my eyes ). I tried with a few sample exif images which has orientation. And without loadFromFile() files are seen as mentioned in the issue. After using loadFromFile(), they were getting corrected.

I have also noticed #29319, which replaced loadFromFile, to loadFromFileHandle method.

Using objectstore

Using gallery to verify the preview with
objectstore_preview

The preview from object store version preview for the image file
version_object_store_preview
The preview doesn't come up properly.
On the other hand preview of text file under version panel
objectstore_text_preview_version

PR

Created PR: #34297
Let me know the suggestion if this approach looks good @PVince81 @VicDeo

  • Unit test is not added, the coverage is happening.

  • Verify the preview of text files
    text_preview

  • Verify public link preview
    image_publink_preview

  • Verify trashbin preview
    trashbin_preview

PS: The idea behind public link preview and trashbin preview is inspired from comment : #29319 (review)

@PVince81
Copy link
Contributor

@sharidas please also test with external storage and make sure the file is not downloaded to temp space when on local storage. With ext storage, getLocalFile should download the file from the ext storage to temp space.

@PVince81
Copy link
Contributor

@sharidas also verify version preview: open versions panel and see the thumbnail orientation

@sharidas
Copy link
Contributor

@sharidas please also test with external storage and make sure the file is not downloaded to temp space when on local storage. With ext storage, getLocalFile should download the file from the ext storage to temp space.

While testing with external storage SFTP, I have noticed that getLocalFile method returns file of /tmp/oc_tmp_*.jpg ( obviously for a jpg file ). And when I try to check the file in my local machine:

✘ sujith@sujith-ownCloud  ~/test/owncloud2   stable10  ls /tmp/oc_tmp_y9p5iR-.jpg
ls: cannot access '/tmp/oc_tmp_y9p5iR-.jpg': No such file or directory
 ✘ sujith@sujith-ownCloud  ~/test/owncloud2   stable10 

So I believe it is not downloaded to the tmp location. Let me know if we agree with this observation.

@sharidas
Copy link
Contributor

sharidas commented Jan 30, 2019

@sharidas also verify version preview: open versions panel and see the thumbnail orientation

Regarding preview for version, I am missing bits how to get the version update for image :( I mean after uploading the file, may I know how to update the image so that version panel lists previous version?

Regarding preview for text file the version panel, it is working properly
text_version

Regarding preview for image, the version panel is as shown here
version

@PVince81
Copy link
Contributor

@sharidas depending on your distro the tmp files might be in chroot somewhere in /tmp. try searching for the exact file name. you'll likely need to set a breakpoint to prevent the file getting deleted

@sharidas
Copy link
Contributor

sharidas commented Jan 31, 2019

Analysis with external storage

Scenario by having SFTP as externa storage
  1. Tried to upload an image file to local storage ( not the sftp storage or the folder ), the file path points to the data directory where it resides. Meaning its not copied to the temperory folder ( say /tmp or so )
  2. Tried to upload an image to sftp storage and below is the observation:
    1. The code reaches here https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Common.php#L254
    2. And then https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/LocalTempFileTrait.php#L48
    3. And reaches https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/LocalTempFileTrait.php#L77
    4. By this time the $tmpFile is /tmp/oc_tmp_H1JRmt-.jpg and this file is found in my local machine at /tmp/systemd-private-012a7f2341b643d9881e8537cbb42431-apache2.service-epLRm1/tmp/oc_tmp_H1JRmt-.jpg
    5. After the image is successfully uploaded, I tried:
      ✘ sujith@sujith-ownCloud  /tmp  sudo ls /tmp/systemd-private-012a7f2341b643d9881e8537cbb42431-apache2.service-epLRm1/tmp/oc_tmp_H1JRmt-.jpg
      ls: cannot access '/tmp/systemd-private-012a7f2341b643d9881e8537cbb42431-apache2.service- epLRm1/tmp/oc_tmp_H1JRmt-.jpg': No such file or directory
      ✘ sujith@sujith-ownCloud  /tmp 
      
    6. So yes with external storage it is downloaded to the temp space.

@PVince81 I assume this is what you wanted me to look at from #18645 (comment) and #18645 (comment)

@PVince81
Copy link
Contributor

PVince81 commented Feb 1, 2019

fix will be in 10.1.1 (10.1.0 already is in code freeze)

@depeje
Copy link

depeje commented Apr 9, 2019

@phil-davis
Copy link
Contributor

fix will be in 10.1.1 (10.1.0 already is in code freeze)

Predicting the specific number of a future version is a risky undertaking ;)
There was a "hotfix patch" for a particular bug and that ended up being the version called 10.1.1

The fix for the issue here was backported to stable10 in PR #34356. That will be in the next "regular release". At the time of writing this post the release numbering "will be" 10.2 - but of course the next release could be 10.1.2 10.2 or 11.0 - that just depends on the semver-related content of whatever is in the release.

@depeje
Copy link

depeje commented Apr 9, 2019

Thank you for the explanation :) , it's very helpful. I mainly wanted to save other readers here the hassle of having to find this out the hard way. And I wanted to verify that this patch is indeed coming to a release.

@PVince81
Copy link
Contributor

PVince81 commented Apr 16, 2019

you can already test with the RC1 if you like: https://central.owncloud.org/t/owncloud-server-10-2-0-rc1-released/19499

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.