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

RFC Default to Imagick over GD when available #207

Closed
maxime-rainville opened this issue Jan 22, 2019 · 8 comments
Closed

RFC Default to Imagick over GD when available #207

maxime-rainville opened this issue Jan 22, 2019 · 8 comments

Comments

@maxime-rainville
Copy link
Contributor

maxime-rainville commented Jan 22, 2019

Background

I've been working on silverstripe/silverstripe-framework#8664 which indirectly led me to compare the performance of GD vs Imagick.

Our current ImageManager defaults to using GD as its driver. Developers can decide to use Imagick by adding this in their config.

SilverStripe\Core\Injector\Injector:
  Intervention\Image\ImageManager:
    constructor:
      - { driver: imagick }

On my test case, Imagick is orders of magnitude faster than GD. When generating thumbnails for a thousand 4K image a got the following performance:

  • Imagick completed the job in 412 seconds
  • GD completed the job in 1498 seconds

Both were using about the same amount of memory.

GD is bundled with PHP so we can be rely on it being available. Imagick needs to be installed by your sysadmin, but many hosting environment will have it pre-installed.

My assumptions are that

  • most developers don't care about what image driver their site uses and/or know they can choose a different one
  • many sites out there are hosted on servers where Imagick is available and could benefit from the improve performance
  • most SilverStripe projects wouldn't have complex requirements when it comes to generating images, so Imagick could be transparently swapped for GD.

Proposal

  • Create a new ImageManagerFactory that accepts three possible driver: gd, imagick or auto which would test for the availability of Imagick fallback to GD.
  • Assets could default to using the new ImageManagerFactory, so people would get the performance boost simply by upgrading.
  • Alternatively, the installer could be updated to use ImageManagerFactory in its YML config, so only new project would get it by default.

Things to keep in mind

Acceptance criteria

  • CMS 6 is updated to default to use the Imagick if available and falls back to GD.
  • Developers retain the option to explicitely choose Imagick or GD if they want to.
  • Doc is updated to explain how to configure the Image driver selection
  • Choosing the image driver is achieved without meaningful performance hit.
  • Benchmarks are run on SC to confirm there's a performance gain and findings are added to CMS 6 changelog.
@dnsl48
Copy link
Contributor

dnsl48 commented Jan 22, 2019

Also could consider adding imagick to composer suggest.

Another possible option could be to make the ImageManagerFactory customizable not by auto, but rather by enumerating the list of backends in the YAML in the priority order. That might look like this:

SilverStripe\Core\Injector\Injector:
  Intervention\Image\ImageManager:
    constructor:
      - drivers: [ imagick, gd ]

That would move the default backend out of the code to the configuration level.

@dylangrech92
Copy link

I've just swapped the driver to imagick on a site that receives constant updates and saw a 50% speedup across the board in site load speed time. I can't upvote the suggestion enough!

Really didn't expect this much of a difference

@GuySartorelli
Copy link
Member

GuySartorelli commented Apr 9, 2024

I think this is well worth doing. Changing defaults for configuration is not allowed under our definition of public API - we could opt into this for new projects by adding config to installer, or we could just have this as a CMS 6 enhancement.
I personally lean towards the latter.

@maxime-rainville
Copy link
Contributor Author

A couple extra notes.

Intervention\Image\ImageManager is NOT our class. We can't change anything in there. If we want to have some dynamic picking of what driver to use, the factory approach is the only sensible way of doing this.

Looking at this now, I don't know that we have to provide any option. Basically, the factory would be used by default. If you want to explicitly use GD or Imagick, just use the Injector to instantiate your own. In YML this would look like this.

## Default set up
SilverStripe\Core\Injector\Injector:
  Intervention\Image\ImageManager:
     factory: SilverStripe\Assets\ImageManagerFactory

## Snippet to copy in your project if you GD
SilverStripe\Core\Injector\Injector:
  Intervention\Image\ImageManager:
    constructor:
      - { driver: gd }

## Snippet to copy in your project if you Imagick
SilverStripe\Core\Injector\Injector:
  Intervention\Image\ImageManager:
    constructor:
      - { driver: imagick }

My current assumption is that figuring out if the current server supports Imagic is a trivial process that takes a negligible amount of time. I would like that assumption validated.

@jeremysomar
Copy link

jeremysomar commented May 27, 2024

The way we tend to do it currently is with a check to see whether the extension is loaded

---
Only:
  extensionloaded: imagick
---
SilverStripe\Core\Injector\Injector:
  Intervention\Image\ImageManager:
    constructor:
      - { driver: imagick }

@maxime-rainville
Copy link
Contributor Author

@jeremysomar Didn't think about that. That's actually a lot more straight forward.

@emteknetnz emteknetnz self-assigned this Jun 30, 2024
@emteknetnz
Copy link
Member

Since this is for CMS 6, and we need to upgrade to intervention 3 for CMS 6 (intervention 2 is EOL), we need to do this issue to upgrade to intervention 3 before doing this one. The API for intervention 3 is very different from intervention2

@GuySartorelli
Copy link
Member

This will be done as part of #619, since I need to add yaml config to define the default driver anyway as part of the upgrade.

@GuySartorelli GuySartorelli closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants