Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix image orientation when generating thumbnail #5039

Merged
merged 9 commits into from
May 16, 2019
Merged

Fix image orientation when generating thumbnail #5039

merged 9 commits into from
May 16, 2019

Conversation

prodrigestivill
Copy link
Contributor

@prodrigestivill prodrigestivill commented Apr 9, 2019

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 of Image._getexif() but it will require pillow>=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

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

@prodrigestivill prodrigestivill marked this pull request as ready for review April 9, 2019 18:12
@prodrigestivill
Copy link
Contributor Author

Afects matrix-org/matrix-doc#483

@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #5039 into develop will decrease coverage by 0.01%.
The diff coverage is 38.09%.

@@             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

Copy link
Member

@anoadragon453 anoadragon453 left a 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.

synapse/rest/media/v1/thumbnailer.py Show resolved Hide resolved
@anoadragon453 anoadragon453 requested a review from a team April 12, 2019 10:37
@prodrigestivill
Copy link
Contributor Author

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 either rotate before the scale/crop or copy the EXIF orientation information when generating the thumbnail.

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.

Copy link
Member

@richvdh richvdh left a 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?

synapse/rest/media/v1/thumbnailer.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/thumbnailer.py Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a 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.

synapse/rest/media/v1/thumbnailer.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/thumbnailer.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/thumbnailer.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/thumbnailer.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/thumbnailer.py Outdated Show resolved Hide resolved
@neilisfragile neilisfragile assigned richvdh and unassigned richvdh May 9, 2019
@richvdh richvdh self-requested a review May 15, 2019 15:46
Copy link
Member

@richvdh richvdh left a 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?

@richvdh
Copy link
Member

richvdh commented May 16, 2019

oh urgh the isort failure is something that is fixed in develop. Could you merge in latest develop? sorry, but the rest of the CI won't run until it's fixed.

Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
…low dependency version can be lowered.

Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
Signed-off-by: Pau Rodriguez-Estivill <prodrigestivill@gmail.com>
@richvdh richvdh self-requested a review May 16, 2019 16:15
Copy link
Member

@richvdh richvdh left a 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)

@richvdh richvdh changed the title Media Thumbnailer EXIF rotation Fix image orientation when generating thumbnail May 16, 2019
@richvdh richvdh merged commit f89f688 into matrix-org:develop May 16, 2019
@johansmitsnl
Copy link

Should this be working in the latest docker image? For me it is not working.

@prodrigestivill
Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants