Update tiff reading - writing methods for f32 and u8 datatype #345

viktorho · 2025-04-07T09:22:19Z

add traits TiffValueConverter
add write_image_tiff_internal(), and example for u8 only
add read_image_tiff_internal(), and example for u8, f32 (experiments)

Copilot

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

crates/kornia-io/src/tiff.rs:52

[nitpick] Consider translating the comment 'Đảm bảo T::Inner là u8' to English for consistency with the rest of the codebase.

    T: ColorType<Inner = u8>,  // Đảm bảo T::Inner là u8

crates/kornia-io/src/tiff.rs:203

[nitpick] The test function name 'test_read_image_tiff_rgb8' does not match the usage of 'read_image_tiff_rgba8'; consider renaming the test function for clarity.

        let image = read_image_tiff_rgba8(&file_path).unwrap();

AS1100K

Make sure to format your code with rustfmt and run the tests as one of your test fails.

AS1100K · 2025-04-07T16:48:17Z

crates/kornia-io/src/error.rs

@@ -26,6 +26,15 @@ pub enum IoError {
    #[error("Error with Jpeg encoding")]
    JpegEncodingError(#[from] jpeg_encoder::EncodingError),

+
+    /// Error decoding the TIFF image.
+    #[error("Error with TIFF decoding")]


Suggested change

#[error("Error with TIFF decoding")]

#[error("Error with TIFF decoding. {0}")]

Let's make the errors more informational. I have also made them in #336

i advocate more to use #[error(transparent)] to forward directly the original errors
https://github.com/kornia/kornia-rs/blob/main/crates/kornia-io/src/stream/error.rs#L5

AS1100K · 2025-04-07T16:48:36Z

crates/kornia-io/src/error.rs

+    TiffError(#[from] tiff::TiffError),
+
+    /// Error to create the TIFF image.
+    #[error("Error with TIFF encoding")]


Suggested change

#[error("Error with TIFF encoding")]

#[error("Error with TIFF encoding. {0}")]

Same

AS1100K · 2025-04-07T16:50:51Z

crates/kornia-io/src/tiff.rs

+    image: &Image<u8, C>,
+) -> Result<(), IoError>
+where 
+    T: ColorType<Inner = u8>,  // Đảm bảo T::Inner là u8


Nitpick, consider writing the comment in english

AS1100K · 2025-04-07T16:55:43Z

crates/kornia-io/src/tiff.rs

+    if image.num_channels() != C {
+        return Err(IoError::TiffEncodingError(
+            format!("Image has {} channels, but expected {}", image.num_channels(), C)
+        ));
+
+    }


Suggested change

if image.num_channels() != C {

return Err(IoError::TiffEncodingError(

format!("Image has {} channels, but expected {}", image.num_channels(), C)

));

}

Consider removing this check, as it is a internal function and the public facing functions ensure that the correct number of channels are provided.

AS1100K · 2025-04-07T16:58:26Z

crates/kornia-io/src/tiff.rs

+/// Reads a TIFF image with a single channel (rgb8).
+/// #Arguments
+/// file_path - The path to the TIFF file.
+/// 


Suggested change

/// Reads a TIFF image with a single channel (rgb8).

/// #Arguments

/// file_path - The path to the TIFF file.

///

/// Reads a TIFF image with a three channel (rgb8).

///

/// # Arguments

///

/// - `file_path` - The path to the TIFF file.

Make it in markdown format, so it render properly on docs.rs

AS1100K · 2025-04-07T17:25:04Z

crates/kornia-io/src/jpeg.rs

+    #[test]
+    fn read_jpeg_f32() -> Result<(), IoError> {
+        let image = read_image_jpeg_mono8("../../tests/data/dog.jpeg")?;
+        assert_eq!(image.cols(), 258);
+        assert_eq!(image.rows(), 195);
+        Ok(())
+    }
+


Suggested change

#[test]

fn read_jpeg_f32() -> Result<(), IoError> {

let image = read_image_jpeg_mono8("../../tests/data/dog.jpeg")?;

assert_eq!(image.cols(), 258);

assert_eq!(image.rows(), 195);

Ok(())

}

Firstly, the name of the test doesn't make sense as jpeg doesn't support f32 and you are using read_image_jpeg_mono8 in it.

And secondly, this doesn't work. It will give ImageCreationError(InvalidChannelShape(150930, 50310)) error.

Make sure to run the tests first.

AS1100K · 2025-04-07T17:42:14Z

crates/kornia-io/src/tiff.rs

+/// 
+/// This trait is used to convert TIFF values (u8 or f32) to a specific type (e.g., u8 or f32).
+/// It provides two methods:
+pub trait TiffValueConverter: Sized {


I think we should have a simple trait (could be internal?) with only one function like type_ and should have it's implementation for all numeric primitive types that could be bound to generic T (P in your case, change it to T so that it's globally same).
The type_ function could be used to verify if the decoded image is of same type, if not return error.

This could be used to to only allow integer primitive types. Also, we should not deal with any conversion and make it up to the user if they want to covert the type.

Then your functions should look something like:

pub trait NumericTrait { fn type_() -> NumericType; } pub fn read_image_tiff_rgba8<T: NumericTrait>(file_path: impl AsRef<Path>) -> Result<Image<T, 4>, IoError> { todo!() } pub fn read_image_tiff_internal<T: NumericTrait>(/* arguments */) -> /* ... */

What do you say @edgarriba ?

sounds good to me

Hello, I think we should provide more details when implementing images for our application. It could be a marker type instead of struct u8 in this context, for example:
let rgb_image: Image<u8, 4> = read_image_any::<Rgba8>("image.jpg")?;
From my vision, this could be more sustainable in case we would provide more features for the fourth channel, like depth or heat instead of alpha.

@edgarriba , @AS1100K
Would it be a good one in this case? If it's good to go, i'm gonna include these for the next pull request

for now we decided in a previous conversation in discord to hold on the read_any and leave generalizations for later

AS1100K · 2025-04-07T17:54:23Z

crates/kornia-io/src/tiff.rs

+    use std::fs;
+    use std::path::PathBuf;
+    use kornia_image::ImageSize;
+    #[test]


Also, add a read_write test

Refer

kornia-rs/crates/kornia-io/src/jpeg.rs

Lines 123 to 139 in d959fb3

fn read_write_jpeg() -> Result<(), IoError> {

let tmp_dir = tempfile::tempdir()?;

create_dir_all(tmp_dir.path())?;

let file_path = tmp_dir.path().join("dog.jpeg");

let image_data = read_image_jpeg_rgb8("../../tests/data/dog.jpeg")?;

write_image_jpeg_rgb8(&file_path, &image_data)?;

let image_data_back = read_image_jpeg_rgb8(&file_path)?;

assert!(file_path.exists(), "File does not exist: {:?}", file_path);

assert_eq!(image_data_back.cols(), 258);

assert_eq!(image_data_back.rows(), 195);

assert_eq!(image_data_back.num_channels(), 3);

Ok(())

}

AS1100K · 2025-04-07T17:54:57Z

crates/kornia-io/src/tiff.rs

+        let image = Image::new(ImageSize { width: 100, height: 100 }, vec![0; 30000]).unwrap();
+        let file_path = PathBuf::from("test_rgb8.tiff");
+        assert!(write_image_tiff_rgb8(&file_path, &image).is_ok());
+        fs::remove_file(file_path).unwrap();


Use tempfile crate for this, as all the similar tests in this crate uses it

AS1100K · 2025-04-07T17:55:07Z

crates/kornia-io/src/tiff.rs

+        let image = Image::new(ImageSize { width: 100, height: 100 }, vec![0; 10000]).unwrap();
+        let file_path = PathBuf::from("test_gray8.tiff");
+        assert!(write_image_tiff_gray8(&file_path, &image).is_ok());
+        fs::remove_file(file_path).unwrap();


edgarriba · 2025-04-07T22:01:17Z

crates/kornia-io/src/error.rs

@@ -26,6 +26,15 @@ pub enum IoError {
    #[error("Error with Jpeg encoding")]
    JpegEncodingError(#[from] jpeg_encoder::EncodingError),

+
+    /// Error decoding the TIFF image.
+    #[error("Error with TIFF decoding")]


i advocate more to use #[error(transparent)] to forward directly the original errors
https://github.com/kornia/kornia-rs/blob/main/crates/kornia-io/src/stream/error.rs#L5

edgarriba · 2025-04-07T22:02:27Z

crates/kornia-io/src/tiff.rs

+};
+use std::fs::File;
+use std::path::Path;
+use std::io::BufWriter;


group properly the imports, probably cargo fmt will do this for you

edgarriba · 2025-04-07T22:06:19Z

crates/kornia-io/src/tiff.rs

+/// 
+/// This trait is used to convert TIFF values (u8 or f32) to a specific type (e.g., u8 or f32).
+/// It provides two methods:
+pub trait TiffValueConverter: Sized {


sounds good to me

edgarriba · 2025-04-07T22:06:58Z

crates/kornia-io/src/tiff.rs

+    }
+
+    let file = File::open(&file_path)?;
+    let mut decoder = Decoder::new(file).map_err(IoError::TiffError)?;


Suggested change

let mut decoder = Decoder::new(file).map_err(IoError::TiffError)?;

let mut decoder = Decoder::new(file)?;

forward error diectly like this

sonarqubecloud · 2025-04-10T13:39:12Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

HO Viktor added 5 commits April 6, 2025 00:03

create tiff function for writing image buffer

4fb430e

update read_image

570bd1f

update read_image_tiff, image shape error

c51efb4

update channels check

1032091

change tiff traits name

1a5796d

edgarriba requested a review from Copilot April 7, 2025 09:40

Copilot AI reviewed Apr 7, 2025

View reviewed changes

AS1100K suggested changes Apr 7, 2025

View reviewed changes

edgarriba reviewed Apr 7, 2025

View reviewed changes

update writting images and commented issues

947be56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update tiff reading - writing methods for f32 and u8 datatype #345

Update tiff reading - writing methods for f32 and u8 datatype #345

viktorho commented Apr 7, 2025

Copilot AI left a comment

AS1100K left a comment

AS1100K Apr 7, 2025

edgarriba Apr 7, 2025

AS1100K Apr 7, 2025

AS1100K Apr 7, 2025

AS1100K Apr 7, 2025

AS1100K Apr 7, 2025

AS1100K Apr 7, 2025

AS1100K Apr 7, 2025

edgarriba Apr 7, 2025

viktorho Apr 9, 2025

viktorho Apr 9, 2025

edgarriba Apr 9, 2025

AS1100K Apr 7, 2025

AS1100K Apr 7, 2025

AS1100K Apr 7, 2025

edgarriba Apr 7, 2025

edgarriba Apr 7, 2025

edgarriba Apr 7, 2025

edgarriba Apr 7, 2025

sonarqubecloud bot commented Apr 10, 2025

	#[error("Error with TIFF decoding")]
	#[error("Error with TIFF decoding. {0}")]

	#[error("Error with TIFF encoding")]
	#[error("Error with TIFF encoding. {0}")]

	fn read_write_jpeg() -> Result<(), IoError> {
	let tmp_dir = tempfile::tempdir()?;
	create_dir_all(tmp_dir.path())?;

	let file_path = tmp_dir.path().join("dog.jpeg");
	let image_data = read_image_jpeg_rgb8("../../tests/data/dog.jpeg")?;
	write_image_jpeg_rgb8(&file_path, &image_data)?;

	let image_data_back = read_image_jpeg_rgb8(&file_path)?;
	assert!(file_path.exists(), "File does not exist: {:?}", file_path);

	assert_eq!(image_data_back.cols(), 258);
	assert_eq!(image_data_back.rows(), 195);
	assert_eq!(image_data_back.num_channels(), 3);

	Ok(())
	}

	let mut decoder = Decoder::new(file).map_err(IoError::TiffError)?;
	let mut decoder = Decoder::new(file)?;

Update tiff reading - writing methods for f32 and u8 datatype #345

Are you sure you want to change the base?

Update tiff reading - writing methods for f32 and u8 datatype #345

Conversation

viktorho commented Apr 7, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

AS1100K left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 10, 2025

Quality Gate passed