ksubileau / color-thief-php

Grabs the dominant color or a representative color palette from an image. Uses PHP and GD, Imagick or Gmagick.
MIT License
633 stars 64 forks source link

calling `getColor()` runs forever #52

Closed othmar52 closed 1 year ago

othmar52 commented 2 years ago

first of all: thank you for this lib!
i experienced a freezing in one of my applictions when calling \ColorThief\ColorThief::getColor($absolutePath, $quality)
after some debugging i found out that ColorThief did never exit a while loop
this fixed the issue for me

--- ksubileau/color-thief-php/src/ColorThief/ColorThief.php     2022-08-21 17:16:32.600014736 +0200
+++ "ksubileau/color-thief-php/src/ColorThief/ColorThief copy.php"      2022-08-21 17:14:31.940025874 +0200
@@ -448,6 +448,7 @@
         $nIterations = 0;

         while ($nIterations < self::MAX_ITERATIONS) {
+            ++$nIterations;
             if ($nColors >= $target) {
                 return;
             }
@@ -460,7 +461,6 @@

             if (!$vBox->count()) { /* just put it back */
                 $priorityQueue->push($vBox);
-                ++$nIterations;
                 continue;
             }
             // do the cut

EDIT
i did not try to reproduce this in a vanilla scenario but the problematic file seems to be this one (extracted from id3 tags)
c48c2e39121_0__raw0853bcccddde5b5b44b64239219c17bd_par52b4fbd9e2256843fcdbe9d1f35f2875

ryanapt commented 2 years ago

Just wanted to throw a +1 on here. We were having the same issue occasionally while running ColorThief::getPalette() on thousands of images every day. It was rare, but it was happening enough to break/freeze our img processor cron every week or so, and eventually caused extra servers to spin up due to CPU spiking (due to this infinite loop).

Can't say if the patch above is necessarily "correct", but it does fix the infinite loop issue, thanks @othmar52. (I'd rather have this return nothing/a wrong color than freeze the entire php process).

Here's my example tiny image that was somehow triggering this issue (I can get more examples if needed): 8balb02885626_small

ksubileau commented 1 year ago

Unfortunately I did not manage to reproduce the issue on my testing environments with the 2 images provided. This is probably due to a difference in runtime environment or parameters. Could you please detail the runtime environment you are using (OS, with Docker ?, PHP version, image library with version) and the parameter values of the ColorThief::getPalette() or ColorThief::getColor() call ?

Nevertheless, I think it may be related to this commit f8f413db. Could you try to do a test after reverting this commit and tell me if that solves the problem?

ryanapt commented 1 year ago

Hey @ksubileau, thanks for looking into this.

I was able to recreate this issue using your lib v2.0.0 on:

(all x86_64, no docker)

We're just calling it like ColorThief::getColor($filename), nothing special (no optional params).

And, yes, I just confirmed reverting https://github.com/ksubileau/color-thief-php/commit/f8f413db5fdd3bce66bd864d2039bb8fb8bf006 on the original code does (also) fix the issue.

ksubileau commented 1 year ago

Hi, Thank you for the information provided, unfortunately I still haven't managed to reproduce the issue with any of the 2 files proposed (Ubuntu 20.04.2 x86_64 on VirtualBox, PHP 8.0.25, GD, with or without docker), but anyway, the cause and the fix are both clearly identified, so I revert the incriminated commit and release version 2.0.1.