-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use loadFromFile instead of loadFromFileHandle #34297
Conversation
Use loadFromFile instead of loadFromFileHandle while generating the thumbnail. Signed-off-by: Sujith H <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #34297 +/- ##
============================================
+ Coverage 64.76% 64.77% +<.01%
Complexity 18368 18368
============================================
Files 1199 1199
Lines 69550 69550
Branches 1281 1281
============================================
+ Hits 45044 45050 +6
+ Misses 24133 24127 -6
Partials 373 373
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #34297 +/- ##
============================================
+ Coverage 64.76% 64.77% +<.01%
Complexity 18368 18368
============================================
Files 1199 1199
Lines 69550 69550
Branches 1281 1281
============================================
+ Hits 45044 45050 +6
+ Misses 24133 24127 -6
Partials 373 373
Continue to review full report at Codecov.
|
seems to make sense, I was expecting something more complicated when I looked into this. It was probably because I took a different route based on git log / diff / bisect and tried to understand the commits that caused the regression. if this fulfils all use cases as tested in the ticket, and doesn't cause additional performance impact (see local storage vs ext storage), then it should be fine |
I guess to generate a preview we still need the whole file, even thought I'd still prefer to use streams... |
@jvillafanez I think we never used streams before because many of the PHP functions for image processing operate directly on files, like changing orientation. We definitely need to look into improving this, it might require searching for new libraries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks good and thanks for the extensive tests in the original ticket
@sharidas please backport |
Backport to stable10 #34356 |
Use loadFromFile instead of loadFromFileHandle
while generating the thumbnail.
Signed-off-by: Sujith H [email protected]
Description
When
loadFromFileHandle
is used for generating thumbnail of images, the problem found is that it doesn't fix the orientation of images with exif. WhileloadFromFile
handles this. It usesexif_imagetype
before checking the mimetype of the image file. There is a word of caution noted in the https://github.com/owncloud/core/blob/master/lib/private/legacy/image.php#L496 ( reference: http://php.net/manual/en/function.exif-imagetype.php#79283 ). With the changeset the orientation issue while generating preview is resolved.Related Issue
Motivation and Context
This changeset fixes the orientation issue of images while generating preview.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Open tasks: