-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add horizontal predictor support #240
Conversation
Any feedback ? If the breaking changes are not welcome I can try and work around them but I was able to build |
Fixed the grammar incompatible with rust 1.61 ! Please re-run the tests I cannot do it manually :( |
Any feedback ? I am using this to compress my images using rust instead of an external tool and if it is helpful to me I'm sure it could help someone else. |
Creating a new |
Well maybe but the method this function is used in does not allow for modification of the buffer it passes: Lines 467 to 490 in 9508118
So there would be copy at some point or another. I can look into changing that but I wanted to limit my changes. I can allocate a new buffer of the same size as image but that seems also like a doubling of the memory, or keep allocating one vec per row and write it directly to the encoder. |
You're calling collect() there which creates a Vec anyway. |
I pushed a version that does not collect and writes strips as soon as they are predicted. If returning a |
Can you check that the tests actually work? I've replaced the predictor code with nothing: Predictor::Horizontal => {
0
} and |
This is what I mean: spoutn1k#1 |
You are right I did not implement tests. Will do that. I tested it by compressing images and opening them with image editors. |
Avoid per-row allocations
Images are corrupted from e00fa6b. Looks like the encoder does not like writing strip by strip. |
Most of the round-trip tests fail for datatypes bigger than |
Thank you |
Thank you ! |
While investigating #237, I came to the realization that the crate lacked predictor support at encoding. This PR attemps to merge the feature in the most efficient way.
TiffEncoder
gains a fluent interfacewith_predictor
allowing users to easily set their preferred method. Using this predictor with the LZW compression makes its average compression ratio go from 0.85:1 (yes, the compressed image is bigger) to 1.3:1.With those changes underway, the clunkyness of the current interface was just too strong. Having the compression as a generic type on the
ImageEncoder
methods serves no purpose other than complicating the code; methods were duplicatedx
andx_with_compression
to account for the type. Adding the predictor to this interface was out of the question:x
x_with_compression
x_with_predictor
x_with_compression_and_predictor
? No thanks.TiffEncoder
also gains awith_compression
fluent interface to set the desired file compression.The final interface looks as such:
This means the
*_with_compression
methods are gone from theTiffEncoder
interface, which means breaking changes.