verbb / image-resizer

A Craft CMS plugin to resize images on upload.
MIT License
128 stars 14 forks source link

Image-resizer never resizes an image #80

Closed jamie-s-white closed 1 year ago

jamie-s-white commented 1 year ago

Describe the bug

Image-resizer never resizes an image

Steps to reproduce

  1. Upload any image
  2. In logs we get "Resizing would result in a larger file size."
  3. We then downgrade from image-resizer 2.2.1 to 2.1.0
  4. Upload the same image
  5. In the logs we get "Resized successfully. Saved 41% (2.35mb)." (and we can see the image has correctly reduced file size and dimensions - test-image-j.jpg and testing-2-1-0.jpg are the same file, just renamed): Screenshot 2023-09-12 at 15 12 17

Craft CMS version

3.7.16

Plugin version

2.2.1

Multi-site?

No

Additional context

No response

engram-design commented 1 year ago

Probably related to the changed behaviour here which was supposed to drastically improve performance when resizing images. I would have suspected this could mean results were a little bit off, due to using in-memory images and estimating the size that way, but a 2.35mb difference is quite off.

engram-design commented 1 year ago

So I've tried a number of different images to test, and not able to reproduce this one. Are you able to advise your Image Resizer settings, the method you're using to upload (via Assets directly, via an Assets field on an entry, bulk resize element action, etc), and send the image you're using to support@verbb.io?

jamie-s-white commented 1 year ago

Our resizer settings are default, this was a fresh install We uploaded the assets directly Will send image

engram-design commented 1 year ago

Thanks for sending that through. So on my end, that's uploading and resizing correctly.

image image

Are you able to confirm if your project uses ImageMagick or GD for image handling? Easiest way to determine this is in Utilities > PHP Info. There's also a Craft setting to control this - https://craftcms.com/docs/4.x/config/general.html#imagedriver

jamie-s-white commented 1 year ago

Imagick 3.7.0 (ImageMagick 6.9.10-23)

engram-design commented 1 year ago

Imagick 3.5.1 here, I don't think that's going to make a massive difference. It was more if you were using GD vs Imagick. I'm also testing with both local an S3 volumes just in case. Strange.

jamie-s-white commented 1 year ago

Very strange!

As we are using an older version of Craft, and you are unable to replicate, I'm not sure if I will now pursue this bug. My main concern was that there were more fundamental issues with the update which you should be aware of, which there clearly aren't, and it seems there is something specific with our set up.

We are happy using the older version of the plugin without the performance improvements for this client, and I will be mindful if we see this issue again in the future when everything is up to date.

Thank you very much for the work you've put into this ticket.

engram-design commented 1 year ago

I'll continue investigating, and especially if anyone else has similar issues (no one has yet to report it).

jamie-s-white commented 1 year ago

OK! We can't right now, because we are getting client approval on our staging site with the older version, however, if you want some further debug information from our staging environment (for example, the two file sizes it is trying to compare) then I could look to add the lines of debug to the plugin and then send you the resulting log file?

I do now also wonder if it is related to the other ticket I raised:

https://github.com/verbb/image-resizer/issues/79

And if the file size of the original image is always 0, because it is not properly finding it and getting its size, and then it compares it to the new size, which is always greater than 0. (This is purely a hunch, I have not investigated this at all!)

jamie-s-white commented 1 year ago

I've done a further bit of digging myself (because I couldn't get this out of my head until I had pulled this thread):

With 2.1.1:

if (filesize($tempPath) < filesize($path)) { // filesize($tempPath) = 3516406 // filesize($path) = 6534207

With 2.2.1:

$resizedSize = strlen((string)$image->getImagineImage()->get('jpg')) * 10; // Lets check to see if this resize resulted in a larger file - revert if so. if ($resizedSize < filesize($path)) { // filesize($tempPath) = 3516406 // filesize($path) = 6534207 // $resizedSize = 35164060

I notice you have *10 at the end of this function @engram-design, and I'm not sure why... And why this would work on other systems but not ours. But that is clearly what is going on here.

Hope this helps :)

engram-design commented 1 year ago

Thanks for investigating! So for some background, it's incredibly more performant to get the resized image in-memory using $image->getImagineImage()->get('jpg') rather than saving it to the filesystem to just determine how large the file is going to be. There's no other method that I'm aware of to be able to determine if the file is going to be a larger filesize than to actually make it a file, or this approach.

So $image->getImagineImage()->get('jpg') will produce the raw representation of the image, and we use strlen to get the size of that. Multiplying that by 10 was getting closer results for me, but that's indeed off now for me also. I'm not quite sure why it's not an issue for me, but going to chalk it up to the estimation factor here.

I've since refined that through some more testing to get more accurate results. Keen to see if that's the same on your end?

To get this early, run composer require verbb/image-resizer:"dev-craft-3 as 2.2.1".

jamie-s-white commented 1 year ago

We have tested this in our staging environment, and this is working on all assets we have tested this on, thank you!

jamie-s-white commented 1 year ago

Hi,

This is still on the dev branch at the moment. We would like to upgrade to it when it is officially released so that we can resize new image uploads automatically. Do you know what sort of time frame this will be?

Thank you!

engram-design commented 1 year ago

Fixed in 2.2.2