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

fix: prevent image uploader converting every image to jpeg #1262

Merged
merged 13 commits into from Dec 7, 2021

Conversation

ikadix
Copy link
Contributor

@ikadix ikadix commented Dec 6, 2021

What does this PR do?

Currently all images are converted to jpeg after going through the cropping process. This removes the ability to upload images that have transparent backgrounds, like company logos. This PR reads the mime type from the image source and attempts to save it as that.

This has the issue of webp failing and also I imagine AVIF, basically anything that JIMP doesn't support.
The error returned by JIMP seems reasonable enough to users IMO, but could add some warning label regarding the images supported?
JIMP lists support for the following formats, but also SVG seems to work just fine.

  • bmp
  • gif
  • jpeg
  • png
  • tiff

This is the error message returned from JIMP in the UI.
2021-12-06-215111_583x150_scrot

Currently an image with a transparent background would get a black background like this
2021-12-06-215326_255x91_scrot

This PR changes this to preserve transparency
2021-12-06-215304_245x101_scrot

The view before uploading a new image has the background replaced and a background is just applied to the image, this works a little nicer than a border around it at the moment I believe.
2021-12-06-215454_721x507_scrot

Type of change

  • New feature (non-breaking change which adds functionality)

How should this be tested?

I have manually tested, each of the image types that JIMP supports, as well as webp to confirm the error message.
I can look to add automated tests for this if you would like, I might need some assistance with that however.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code and corrected any misspellings
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@vercel
Copy link

vercel bot commented Dec 6, 2021

@ikadix is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@ikadix ikadix changed the title fix: image uploader converts all images to jpeg fix: prevent image uploader converting every image to jpeg Dec 6, 2021
@PeerRich
Copy link
Member

PeerRich commented Dec 7, 2021

@emrysal is there any downside of saving as mimeType? in terms of cropping & filesize, etc. if there is no downside, this PR LGTM

@PeerRich PeerRich requested a review from emrysal December 7, 2021 10:04
@ikadix
Copy link
Contributor Author

ikadix commented Dec 7, 2021

When reviewing #1267 I realised that there's a consideration for i18n of the error message given for unsupported MIME Type.
Any guidance on how to handle that would be appreciated 🙌

@emrysal
Copy link
Contributor

emrysal commented Dec 7, 2021

Maybe my thought is wrong here, but rather than using the input image file format, isn't it an simpler approach to use .png instead of jpg for saving images? That would likely result in more support for more file types and would also resolve the transparency issues; without adding any logic?

@ikadix
Copy link
Contributor Author

ikadix commented Dec 7, 2021

Maybe my thought is wrong here, but rather than using the input image file format, isn't it an simpler approach to use .png instead of jpg for saving images? That would likely result in more support for more file types and would also resolve the transparency issues; without adding any logic?

@emrysal You are 100% correct with that thought, removed mime type changes, only PNG matters for transparency support and this actually works with webm now 🙏

Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Awesome!

@PeerRich PeerRich enabled auto-merge (squash) December 7, 2021 17:05
@PeerRich PeerRich merged commit 2312731 into calcom:main Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants