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

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lerni
Copy link
Contributor

@lerni lerni commented Feb 18, 2024

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 silverstripe/silverstripe-assets#585

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

@jonom
Copy link
Owner

jonom commented Feb 19, 2024

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
Copy link
Contributor Author

lerni commented Feb 19, 2024

Saw the test failures, haven't looked into it - probably related 😉 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 silverstripe/silverstripe-assets#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
Copy link

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
Copy link
Owner

jonom commented Feb 20, 2024

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
Copy link
Contributor Author

lerni commented Feb 20, 2024

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.

@jonom jonom marked this pull request as draft February 20, 2024 21:25
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.

3 participants