Encode fixes and improvements

galdor / go-thumbhash

A Go implementation of the Thumbhash image placeholder generation algorithm.

ISC License

76 stars 2 forks source link

Encode fixes and improvements #2

Closed dweymouth closed 7 months ago

dweymouth commented 10 months ago

This PR does the following:

Fixes a bug handling encoding images whose bounds don't start at the origin
Resizes large images before the encoding process
Re-uses allocated memory for the temporary RGBA image, LPQA and other encoding buffers
Adds the large image to the test suite.
- its hash was calculated before adding the resize logic, showing that it didn't change

mholt commented 7 months ago

These changes look really good. I'll probably use your fork until the patch is merged. Do you know about how much faster your changes are?

dweymouth commented 7 months ago

I did not do any benchmarking - all I can confidently say is that if you are doing a lot of encoding, it should no longer be a significant load on allocations and the GC

dweymouth commented 7 months ago

I would guess there's probably an even more "in-place" algorithm that doesn't require converting the whole image at a time to LPQA, and then maybe even the buffers and pools could be gotten rid of, but I didn't want to put that much effort into examining the algorithm and totally changing the implementation when I did this fix :p

mholt commented 7 months ago

I've had a chance to test galdor's current implementation versus yours on a medium-sized dataset of real photos (which I convert to thumbnails using libvips, and then generate the thumbhashes from that). Very rough timings from logs, so no formal benchmarks, but nonetheless:

current: 13 seconds
with patch: 4 seconds

Pretty good speedup!

@galdor I would love to see this merged. :smiley:

dweymouth commented 7 months ago

@mholt I merged this into the main branch of my fork, in case that makes it easier for you to use this in the meantime or if galdor has abandoned this repo. I hope it will get merged here though since I'm not planning on doing much more with my fork for now (though I guess I will if this repo proves to be stale)

mholt commented 7 months ago

@dweymouth Thank you! That's really kind of you.

Go modules are a bit picky however. If I use a replace in go.mod, it either needs to be a local copy (I've cloned down your branch for now), or a fully-qualified Go mod version (e.g. v1.0.0-20240122214204-713ab0e94639) -- which I don't know how to generate myself. If I use your fork with require instead of replace, I get errors because go.mod still has the name of github.com/galdor/go-thumbnail which doesn't match github.com/dweymouth/go-thumbnail :upside_down_face:

Anyway, all that to say I'm just cloning down your fork right now and using my local copy of it. If your fork is tagged (i.e. v1.0.1) or the go.mod is renamed to your GitHub repo path, then I think it'll work to use it directly. You don't have to do either of those, though, at least for now! :slightly_smiling_face:

galdor commented 7 months ago

Sorry about the delay, I missed this PR. Anyway all of it seems reasonable, so it's merged :)

mholt commented 7 months ago

Hurray!! Thanks for seeing this :smiley: And thanks for the great implementation of thumbhash, I'm using it to great benefit.

galdor commented 7 months ago

Happy it helps!