search

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

Performance improvements. #44

Closed mreiden closed 6 years ago

mreiden commented 6 years ago

Speeds things up considerably and uses quite a bit less memory when testing the vegetables_1500x995.png test image.

Basic Symfony stopwatch testing says this is faster than the javascript version. While I hesitate to compare numbers since the hardware and OS is different between my laptop and my dev server, it is much improved. I think the original timing was around 1000ms. The javascript version gets about 490ms on their drag-and-drop demo and this is now getting around 110ms when timing ColorThief::getPalette($thiefImage, 10, 10) which I think is a fair comparison at comparing a quality of every 10 pixels. Using a quality of 1 pixel and timing ColorThief::getPalette($thiefImage, 10, 1) averages about 520ms.

Memory usage is also drastically improved with stopwatch reporting 2MB and the symfony dev toolbar shows the page using 8MB. I think this was over 20MB previously.

mreiden commented 6 years ago

This includes or supersedes my other pull requests.

Sorry it's not split up into multiple commits, but I did add comments and renamed variables to try and indicate RGB color values vs histogram buckets, etc.

ksubileau commented 6 years ago

Good job, I've done some tests and indeed I noticed a considerable performance improvement: around 2 times faster and between 20% to 60% of memory saved with PHP/GD! That's really interresting.

But I'm a little confused between your different PRs that seems to overlap partially but not completely. There's also a lot of changes in one single chunk with apparently differents purposes, so I must admit that it's a little bit hard for me to review. The others PR are clearer to me and almost ready to merge, but why did you open this additionnal PR and didn't push 223e3a7 in #42 for example ? Does this come in addition to #43 ?

mreiden commented 6 years ago

The bitwise OR commit was done to be consistent with the loadImage code.

Using a bitwise OR seems to save about 5ms compared to regular addition if you do it often enough (~2.0 million pixels). Given all the care to make sure bits don't overlap, I thought it might indicate the separation of bits for each color too (RRRRRGGGGGBBBBB).

mreiden commented 6 years ago

I gave up on trying to use separate PRs for things because many of them were dependent on the earlier changes and I noticed there were commits all other the place.

This pull request supersedes #43 and includes #42. We use git very sparingly and I honestly don't know how to do these individual but dependent pull requests correctly.

ksubileau commented 6 years ago

I finally reworked your PR a bit before merging it, but it's still a very great job with a very clear performance gain. I chose not to keep the inlining of the getColorIndex function (ColorThief.php#207), to avoid code duplication and despite the overhead due to the function call. That's a tradeoff between performance and code maintanability. Thank you for your work!