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

#3717, az-digital/az-quickstart-pantheon#282: Add ImageMagick module. #3724

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

joeparsons
Copy link
Member

@joeparsons joeparsons commented Sep 17, 2024

Description

Release notes

If this change requires release notes: provide a summary of changes, how to
use this change, and any related links. This content will be pasted in the
release notes. Use
markdown format to ensure proper pasting of information.

Make sure to add the release notes label to this PR.

The contrib module ImageMagick has been added to Quickstart as an optional module. This module provides an alternate image toolkit for sites that operate in low memory environments and have experienced issues with generation of image derivatives after Quickstart's usage of WebP for image derivatives in Quickstart 2.11.x.

For sites that wish to enable the alternate image toolkit, after enabling the module the toolkit settings can be found on `/admin/config/media/image-toolkit`

Memory constraints can be specified under `Execution Options` after selecting ImageMagick as the toolkit, with prepended options, e.g. `-limit memory 64MiB`

Related issues

How to test

  1. Add this PR on an existing website
  2. Install ImageMagick module
  3. Enable the Image Magic toolkit under Configuration > Media > Image Toolkit (/admin/config/media/image-toolkit)
  4. Flush out images to be re-rendered (/admin/config/media/image-styles)
  5. Clear cache

Types of changes

Arizona Quickstart (install profile, custom modules, custom theme)

  • Patch release changes
    • Bug fix
    • Accessibility, performance, or security improvement
    • Critical institutional link or brand change
    • Adding experimental module
    • Update experimental module
  • Minor release changes
    • New feature
    • Breaking or visual change to existing behavior
    • Upgrade experimental module to stable
    • Enable existing module by default or database update
    • Non-critical brand change
    • New internal API or API improvement with backwards compatibility
    • Risky or disruptive cleanup to comply with coding standards
    • High-risk or disruptive change (requires upgrade path, risks regression, etc.)
  • Other or unknown
    • Other or unknown

Drupal core

  • Patch release changes
    • Security update
    • Patch level release (non-security bug-fix release)
    • Patch removal that's no longer necessary
  • Minor release changes
    • Major or minor level update
  • Other or unknown
    • Other or unknown

Drupal contrib projects

  • Patch release changes
    • Security update
    • Patch or minor level update
    • Add new module
    • Patch removal that's no longer necessary
  • Minor release changes
    • Major level update
  • Other or unknown
    • Other or unknown

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change requires release notes.

@joeparsons joeparsons added enhancement New feature or request dependencies Pull requests that update a dependency file labels Sep 17, 2024
@joeparsons joeparsons self-assigned this Sep 17, 2024
@danahertzberg
Copy link
Contributor

How to test section is updated

@danahertzberg
Copy link
Contributor

danahertzberg commented Sep 20, 2024

Looks like this is beneficial. Default settings change image quality to 75%. This is what the screenshot comparison uses.

Quality is significantly better for split screen paragraph. Original on left, ImageMagick on the right.
Screenshot 2024-09-20 at 4 19 20 PM

Do we need to consider default settings with this module? Or enabled by default?

@tadean
Copy link
Contributor

tadean commented Sep 25, 2024

Imagemagick apparently supports passing additional arguments to imagemagick:
image

This could be used to supply the -limit argument. This might be a good way to ensure we keep memory usage low on pantheon.

@tadean
Copy link
Contributor

tadean commented Sep 25, 2024

Here's example config that I think imposes memory limits of 64MB:

quality: 92
binaries: imagemagick
imagemagick_version: v6
path_to_binaries: ''
prepend: '-limit memory 64MiB'
log_warnings: true
debug: false
image_formats:
  PNG:
    mime_type: image/png
  JPEG:
    mime_type: image/jpeg
  JPG:
    mime_type: image/jpeg
    weight: 10
    enabled: false
  GIF:
    mime_type: image/gif
  GIF87:
    mime_type: image/gif
    weight: 10
    enabled: false
  SVG:
    mime_type: image/svg+xml
    enabled: false
  WEBP:
    mime_type: image/webp
  AVIF:
    mime_type: image/avif
    enabled: false
  TIFF:
    mime_type: image/tiff
    enabled: false
  PDF:
    mime_type: application/pdf
    enabled: false
  HEIC:
    mime_type: image/heif
    enabled: false
  BMP:
    mime_type: image/x-ms-bmp
    enabled: false
  PSD:
    mime_type: image/x-photoshop
    enabled: false
  WBMP:
    mime_type: image/vnd.wap.wbmp
    enabled: false
  XBM:
    mime_type: image/x-xbitmap
    enabled: false
  ICO:
    mime_type: image/vnd.microsoft.icon
    enabled: false
advanced:
  density: 0
  colorspace: '0'
  profile: ''
  coalesce: false

@trackleft
Copy link
Member

trackleft commented Sep 25, 2024

We did some testing in the Wednesday Workshop for the ideal values to add to execution options and advanced image settings and found that setting the -limit argument to 64MiB generally allowed images to render on Pantheon.
Screenshot 2024-09-25 at 2 21 10 PM

Additionally we set the "Change image resolution to 72 ppi" checkbox.
Turns out we don't need the Change image resolution setting.

@tadean
Copy link
Contributor

tadean commented Sep 25, 2024

It also seems that this limit is environment specific. Basic pantheon sites seem to have a memory limit of 256MB and have the most trouble generating the webp image derivatives, presumably when the decompressed image exceeds its PHP memory limit. The same issue doesn't seem to occur on larger sites, or not nearly as frequently.

@tadean
Copy link
Contributor

tadean commented Sep 25, 2024

-limit memory <AMOUNT> lets imagemagick know the constraint where it should try to do the conversion on disk, rather than in memory.

@joeparsons joeparsons marked this pull request as ready for review October 4, 2024 18:45
@joeparsons joeparsons requested a review from a team as a code owner October 4, 2024 18:45
@joeparsons joeparsons added the patch release Issues to be included in the next patch release label Oct 4, 2024
@tadean
Copy link
Contributor

tadean commented Oct 9, 2024

I think the only risk about including this is would be if the module listed the imagemagick extension as a composer requirement. It does not list such a requirement currently.

@tadean tadean merged commit e778f2b into main Oct 11, 2024
16 checks passed
@tadean tadean deleted the issue/3717-imagemagick branch October 11, 2024 21:01
joeparsons added a commit that referenced this pull request Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request patch release Issues to be included in the next patch release release notes
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants