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

Provide better support for high resolution images in mosaico #371

Merged

Conversation

deepak-srivastava
Copy link
Collaborator

@deepak-srivastava deepak-srivastava commented Apr 15, 2020

Overview

The PR attempts to provide better support for high resolution images in mosaico #50.

Technical Details

A new config has been added for user to specify resize scaling factor:

image

When uploading images, the mosaico editor trims it down to very required size (in pixels).
The solution uses scale factor setting to keep some buffer (2x or 3x) so upscale doesn't look distorted or low resolution.

The new method adjustResizeDimensions() part of graphics interface class, makes it work by modifying the requested resize numbers by editor, before they are used for processing by resize and cover image operations/methods.

Tests

A new e2e test case tests adjustResizeDimensions method for various inputs.

Manual test result:

image

If anyone else could test the PR, would be great. cc: @Stoob

PR Testing Notes

  • After applying the PR changes, make sure to clear civicrm cache for new configs to appear.
  • New configs when set, will get applied to only new uploads. For existing images try to delete and re-upload.

@Stoob
Copy link
Contributor

Stoob commented Apr 16, 2020

@deepak-srivastava I have tested and it works great. FYI for others wanting to test here's how. First I made a backup of the existing mosiaco extension directory in my CiviCRM. Then I downloaded the .zip from this branch and copied its files over the existing mosaico extensions directory, replacing the existing code with the new. Then I cleared Civi cache, and went to Administer > CiviMail > Settings to see the 2x/3x options for image sizing.

@mattwire
Copy link
Collaborator

@deepak-srivastava Can you rebase and resolve conflicts please?

@deepak-srivastava
Copy link
Collaborator Author

@mattwire rebase done. Thanks.

@themak1985
Copy link

themak1985 commented May 17, 2020

Hi there,

First, sorry it has taken me so long to respond. I have been testing this a bit and have had some inconsistent results. Not sure what I am doing wrong - but some images are not compressing properly. Attached is a 1920*1080 PNG and JPG. 2.9 and 1.3MB respectively.

My settings are set 3x up to 570px
2x all other sizes.

When I upload, it changes the image to 1602x901 - so the 2x is working, but it does not compress the images at all. I tested the JPG on the mosaico.io website - and it does compress the 1.3MB jpg to about 100KB (at 1x) - I was unable to upload the PNG via mosaico.io due to size restrictions.
Archive.zip

@deepak-srivastava
Copy link
Collaborator Author

@themak1985 Your config "3x up to 570px" covers all the images including the single block images.

Which means when you upload a 1920x1080 image in a 570x200 block, the image gets processed and stored as 1710x962 (where 570*3 = 1710) which in terms of size is almost same as the original image.

If you remove the scaling factor, image is uploaded to a resolution of the block in the template, keeping lower resolution and higher compression.
In other words higher resolution is lower compression.

Note that functionality is not tied to any particular type of image. Png format supports lossless compression and therefore compression appears less than jpg images which support lossy compression.

Now a better config where you can achieve better compression for large images is:

3x upto 285 px
2x for all other sizes.

OR

3x upto 285 px
- no setting for 2x-

Hope that helps.

@mattwire
Copy link
Collaborator

@deepak-srivastava Can you add some docs for this? Maybe create a new "Setup.md" page?

@mattwire
Copy link
Collaborator

mattwire commented Jun 8, 2020

@deepak-srivastava This is just pending documentation so we can merge.

@deepak-srivastava
Copy link
Collaborator Author

@mattwire Have added a setup.md doc. For now it only includes section on scaling factor.

@mattwire
Copy link
Collaborator

mattwire commented Jun 9, 2020

@deepak-srivastava Thanks that looks great. Just two small things:

  1. Please add the new doc to https://github.com/veda-consulting/uk.co.vedaconsulting.mosaico/blob/2.x/mkdocs.yml
  2. Please Put the images directly in this repo under docs/images and then link directly to them (see example here: https://lab.civicrm.org/extensions/stripe/-/blob/master/docs/index.md)

@mattwire
Copy link
Collaborator

mattwire commented Jun 9, 2020

@deepak-srivastava Thanks - with the images please remove the leading /:
ie. ![Screenshot](/images/scaling-factor-resolution-diff.png) changes to ![Screenshot](images/scaling-factor-resolution-diff.png)

@deepak-srivastava
Copy link
Collaborator Author

@mattwire Yeah noticed image were not rendering. Checked in before reading your comment. './' seems to work. Do you still require change?

@mattwire
Copy link
Collaborator

mattwire commented Jun 9, 2020

@deepak-srivastava It has to be images/xx for docs publisher to work properly

@mattwire mattwire merged commit 52388bc into veda-consulting-company:2.x Jun 10, 2020
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.

Support for retina (high dpi) images using Imagemagick
4 participants