-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix image orientation when generating thumbnail #5039
Fix image orientation when generating thumbnail #5039
Conversation
Afects matrix-org/matrix-doc#483 |
Codecov Report
@@ Coverage Diff @@
## develop #5039 +/- ##
===========================================
- Coverage 62.15% 62.13% -0.02%
===========================================
Files 336 336
Lines 34688 34709 +21
Branches 5683 5687 +4
===========================================
+ Hits 21559 21567 +8
- Misses 11593 11604 +11
- Partials 1536 1538 +2 |
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 otherwise, but we'll need to see if this is the approach we want to take.
The main problem with current Thumbnailer is that creates an scaled or cropped thumbnail and throws away the EXIF orientation information. So for me this problem is independent of what is decided in matrix-org/matrix-doc#483 but it still affects to solve the whole problem. But it can still be decied to copy the EXIF orientation in the thumbnail instead, but I think this would be an strange solution. |
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.
apart from a bit of nitpicking, the code looks reasonable.
However, it seems like it is making a particular decision on https://github.com/matrix-org/matrix-doc/issues/483. It may be a valid decision, but the discussion should happen on that issue, not here.
So please can you make your case on that issue?
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.
Thanks again for this.
I have a few requests for minor changes; it otherwise looks good.
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.
Ok, this looks good, thanks.
Sadly it's failing a couple of checks when I run it against the CI. Can you investigate and fix, please?
oh urgh the isort failure is something that is fixed in |
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
…low dependency version can be lowered. Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
Signed-off-by: Pau Rodriguez-Estivill <[email protected]>
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.
lgtm!
(please avoid force-pushing in general: it makes it hard to see how the PR has been changed since a previous review)
Should this be working in the latest docker image? For me it is not working. |
It should be working on latest official Docker image. Please ensure you are looking at the thumbnail is generated by the sever and not the thumbnail generated by the client or the thumbnail of the thumbnail generated by the client. Normally Matrix clients generate their own thumbnails and upload them without any EXIF information. |
Add EXIF rotation before generating thumbnails.
Fixes matrix-org/matrix-spec-proposals#2384, matrix-org/matrix-spec#292, element-hq/element-web#1999, and element-hq/element-web#2059
For now it needs
pillow>=4.3.0
.It can be moved to use
Image.getexif()
instead ofImage._getexif()
but it will requirepillow>=6.0.0
.This is explained in Pillow 6.0.0 Documentation.
Not sure if the preference is to use the oldest minimum libraries or no issues at all with using latest ones.
Pull Request Checklist