kornelski / dssim

Image similarity comparison simulating human perception (multiscale SSIM in Rust)
https://kornel.ski/dssim
GNU Affero General Public License v3.0
1.09k stars 70 forks source link

Allow passing modified_image as a reference #57

Closed mernen closed 4 years ago

mernen commented 5 years ago

Being forced to move an image for comparison was quite inconvenient, as I intended to reuse them several times. As far as I can tell, though, comparisons shouldn't ever need to modify either input, so a reference should be fine.

The borrow gymnastics are necessary to maximize backwards compatibility. Changing function signatures always introduces the possibility of breaking some obscure use case, but I think this qualifies as generalizing to generics.

I wasn't sure how best to include this in the tests, so I just slightly modified one of the tests to ensure both forms are used.

Note: I'm not familiar at all with the codebase! I just changed the signature for compare() and let the compiler guide me.

kornelski commented 5 years ago

Thanks for the PR!

I'm surprised that it works :D

The original intention was to allow reuse of the first argument, but intentionally destroy the second by reusing its memory.

During comparison I need temporary buffers. Instead of allocating new ones, I was going to overwrite existing ones. But that means the original image is lost after the comparison. That's supposed to reduce memory footprint and improve locality.

The C version was much more aggressive about this.

mernen commented 5 years ago

In that case, perhaps DssimImage could implement Clone? Not ideal for my personal use case, as cloning large buffers is quite expensive*, but still much better than calling create_image() multiple times with the same data.

* On second thought, I guess it wouldn't actually be much worse, since I'd save on creating one temporary buffer per comparison

kornelski commented 5 years ago

Oh yes, they should be cloneable. That was an oversight. Fixed.