jonom / silverstripe-focuspoint

Module for improving automatic image cropping in SilverStripe. Adds simple art-direction control by allowing you to set and crop from a focus point instead of the centre point of an image.
MIT License
109 stars 48 forks source link

use clone not chained methods to work with InterventionBackend & vips #120

Open lerni opened 9 months ago

lerni commented 9 months ago

Silverstripe supports libvips as an image manipulation library per Intervention\Image, but fails with focuspoint. I've made a repo to demonstrate and some details in the readme.

https://github.com/lerni/vanilla-vips

Since Edge 121 now also supports AVIF and libvips does that faster than other drivers, I'm keen to try the new possibilities with https://github.com/silverstripe/silverstripe-assets/pull/585

https://caniuse.com/?search=AVIF

jonom commented 9 months ago

Thanks @lerni... I don't completely understand but it would be awesome to be able to output AVIF in Silverstripe 😃 (especially if we could fallback to the original format based on browser capabilities a la CloudFlare Images).

Do you know if introducing clone here has any potential performance impact? This module has had issues with memory management in the past (fixed by smarter people than me), and I'm keen to avoid a regression.

Also wondering if you know if the test failures are due to your changes, or if the tests might just be broken 😂. Most of the work on this module in the past few years has been done by generous contributors so I'm not as familiar with the code as I used to be!

lerni commented 9 months ago

Saw the test failures, haven't looked into it - probably related :wink: Certainly, the clone approach isn't ideal and the performance concerns are valid. Before digging into tests, I would like to explore other ideas (streams?) on how to get around the problem that chained image manipulations have a similar unlink problem with libvips as described in https://github.com/silverstripe/silverstripe-assets/pull/539

PS: AVIF is not tied to vips. GD & imagick can support it as well, but vips is generally faster, especially with AVIF. I've used some .htaccess magic with webp but tend to abandon that because image-indexing gets worse. In a few months, Edge >= 121 will be available broadly and only Safari older than 16 is problematic (Desktop & mobile currently less than 3%).

forsdahl commented 9 months ago

I doubt those test failures have anyting to do with this PR. Cloning the backend like that probably works, but kind of feels like the wrong way to do it. I think the problem really is in trying to chain manipulations directly on the image backend in applyCrop. I think the more correct solution would be to first do the higher level image manipulation function for scaling (that is doing ScaleHeight or ScaleWidth on the Image object) in FocusPointExtension, and then just doing the cropping or resizing on that scaled variant.

jonom commented 9 months ago

Great to see discussion happening on this. I don't have any time to explore this myself, but happy to review a PR if and when ready. @lerni it seems like you're at a very early stage right now so I would suggest closing this PR and opening a new one later?

lerni commented 9 months ago

Thanks Niklas for you input! I'll have another go at it. Jono - yes, it's in a draft state, but it outlines the problem and hopefully helps getting closer to a solution - all fine closing it anyway.