Skip to content
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

Merged
merged 13 commits into from
Sep 23, 2024
Merged

Conversation

spoutn1k
Copy link
Contributor

@spoutn1k spoutn1k commented Aug 3, 2024

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 interface with_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 duplicated x and x_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 a with_compression fluent interface to set the desired file compression.

The final interface looks as such:

let mut encoder = TiffEncoder::<std::fs::File>::new(compressed)?
        .with_predictor(Predictor::Horizontal)
        .with_compression(Compression::Lzw);

    encoder.write_image::<colortype::RGB8>(
        photo.width(),
        photo.height(),
        photo.as_rgb8().expect("Wrong image format"),
    )?;
-    let mut encoder = TiffEncoder::<std::fs::File>::new(compressed)?;
+    let mut encoder = TiffEncoder::<std::fs::File>::new(compressed)?
+        .with_predictor(Predictor::Horizontal)
+        .with_compression(Compression::Lzw);
+
+    encoder.write_image::<colortype::RGB8>(
-    encoder.write_image_with_compression::<colortype::RGB8, Compresion::Lzw>(
         photo.width(),
         photo.height(),
-        Compression::Lzw,
         photo.as_rgb8().expect("Wrong image format"),
+    )?;

This means the *_with_compression methods are gone from the TiffEncoder interface, which means breaking changes.

@spoutn1k
Copy link
Contributor Author

Any feedback ? If the breaking changes are not welcome I can try and work around them but I was able to build image-rs/image with no issues

@spoutn1k
Copy link
Contributor Author

Fixed the grammar incompatible with rust 1.61 ! Please re-run the tests I cannot do it manually :(

@spoutn1k
Copy link
Contributor Author

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.

@kornelski
Copy link

Creating a new Vec per row is wasteful. You could pass &mut Vec to the predictor and have it append pixels directly to the final buffer.

@spoutn1k
Copy link
Contributor Author

spoutn1k commented Sep 19, 2024

Well maybe but the method this function is used in does not allow for modification of the buffer it passes:

pub fn write_strip(&mut self, value: &[T::Inner]) -> TiffResult<()>
where
[T::Inner]: TiffValue,
{
let samples = self.next_strip_sample_count();
if u64::try_from(value.len())? != samples {
return Err(io::Error::new(
io::ErrorKind::InvalidData,
"Slice is wrong size for strip",
)
.into());
}
// Write the (possible compressed) data to the encoder.
let offset = match self.predictor {
Predictor::None => self.encoder.write_data(value)?,
Predictor::Horizontal => {
let predicted: Vec<T::Inner> = value
.chunks(self.row_samples as usize)
.flat_map(|row| T::horizontal_predict(row).into_iter())
.collect();
self.encoder.write_data(predicted.as_slice())?
}
_ => unreachable!(),

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.

@kornelski
Copy link

You're calling collect() there which creates a Vec anyway.
Don't use flat_map + collect(). Make Vec::with_capacity() and append to it

@spoutn1k
Copy link
Contributor Author

spoutn1k commented Sep 19, 2024

[T::Inner] does not have Clone, so if I want to create the Vec outside the predictor function, I will need to add it ...

I pushed a version that does not collect and writes strips as soon as they are predicted. If returning a Vec is still a performance concern, I will update the ColorType trait to do so.

@kornelski
Copy link

Can you check that the tests actually work?

I've replaced the predictor code with nothing:

Predictor::Horizontal => {
                0
}

and cargo test --all still passes.

@kornelski
Copy link

This is what I mean: spoutn1k#1

@spoutn1k
Copy link
Contributor Author

spoutn1k commented Sep 20, 2024

You are right I did not implement tests. Will do that. I tested it by compressing images and opening them with image editors.

@spoutn1k
Copy link
Contributor Author

Images are corrupted from e00fa6b. Looks like the encoder does not like writing strip by strip.

@spoutn1k
Copy link
Contributor Author

Most of the round-trip tests fail for datatypes bigger than u8. After some testing and manually compressing images it seems the implementation of the "deprediction" in the decoder is the culprit.

@spoutn1k
Copy link
Contributor Author

Fixes #237 and #247 !

@kornelski kornelski merged commit e28ad56 into image-rs:master Sep 23, 2024
@kornelski
Copy link

Thank you

@spoutn1k
Copy link
Contributor Author

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants