Repository: image-rs/image-tiff
Pull request #84 (2020-07-24 13:45:54)

Allow custom encoding process
Added custom encoding function, tests and keep the compatibility with existing functions.
My PR #80 was too complicated and with lot of changes so i prefer to split the features for better integration with the lib and ease the review.

Farkal: I don't know how to keep the compatibility with rust 1.34.2 :/ 
```rust
error[E0161]: cannot move a value of type (dyn for<'r> std::ops::FnOnce(&'r mut encoder::DirectoryEncoder<'a, W>) -> std::result::Result<(), error::TiffError> + 'static): the size of (dyn for<'r> std::ops::FnOnce(&'r mut encoder::DirectoryEncoder<'a, W>) -> std::result::Result<(), error::TiffError> + 'static) cannot be statically determine
```
HeroicKatora: > I don't know how to keep the compatibility with rust 1.34.2 :/

You could use `FnMut` instead of `FnOnce` for now. It's slightly less generic but I don't think it's a huge deal. None of the writes requires _moving_ data into the encoder by value. For example [writing tags](https://docs.rs/tiff/0.5.0/tiff/encoder/trait.TiffValue.html#impl-TiffValue-for-%26%27a%20T) takes them by reference instead. Frankly this seems to simplify things as you can take the parameter as a `&mut dyn FnMut` instead of a `Box<_>` which does not require a separate allocation.
birktj: I wonder what the main usecase for this feature would be. Currently one can write:
```rust
let mut image_encoder = encoder.new_image(...);
additional_encoding(image_encoder.encoder());
// do whatever with `image_encoder`
```

As far as I understand this should be equivalent to this using the new feature:
```rust
let mut image_encoder = encoder.new_image_extended(..., aditional_encoding);
// do whatever with `image_encoder`
```
birktj: Ah, I see you have also added an `write_image_extended` method, I can see why something like that could be useful. I wonder if a better method would be to change `write_image` to return the underlying `ImageEncoder`, or possibly add an equivalent function that does this. This would make `write_image` much more applicable and extendable and should make it easy to add any additional information you want using this function.
Farkal: Yes this kind of behavior would be better. In fact `new_image` return the `ImageEncoder` but there is not all the usefull code in `write_image`. So perhaps just put a `write` method on the `ImageEncoder` witch can contain:
```rust
pub fn write(&mut self, data: &[T::Inner]) -> TiffResult<()> {
let num_pix = usize::try_from(self.width)?.checked_mul(usize::try_from(self.height)?)
            .ok_or_else(|| ::std::io::Error::new(
                ::std::io::ErrorKind::InvalidInput,
                "Image width * height exceeds usize"))?;
        if data.len() < num_pix {
            return Err(::std::io::Error::new(
                ::std::io::ErrorKind::InvalidData,
                "Input data slice is undersized for provided dimensions").into());
        }
let mut idx = 0;
        while self.next_strip_sample_count() > 0 {
            let sample_count = usize::try_from(self.next_strip_sample_count())?;
            self.write_strip(&data[idx..idx + sample_count])?;
            idx += sample_count;
        }
        self.finish() // We can include or remove this
}
``` 
So when developping an external lib (geotiff for example) we could just do:
```rust
let mut file = std::io::Cursor::new(Vec::new());
let image_data = vec![0; 100*100*3];
use tiff::encoder::*;

let mut tiff = TiffEncoder::new(&mut file).unwrap();
let mut image = tiff.new_image::<colortype::RGB8>(100, 100).unwrap();

// Add the tags we need with image.encoder.write_tag(Tag::MyTag, "MyValue")

// Encode the data
image.write(data).unwrap();
```
birktj: @Farkal that sounds like a good solution. I would recommend calling the method `write_data` and having the following type signature:
```rust
pub fn write_data(self, data: &[T::Inner]) -> TiffResult<()>;
```
I also think it should end with the `self.finish()` line.
Farkal: I reverted all my changes and added the method to `ImageEncoder` ;)
Farkal: > When you are finished with these you wouldn't mind doing a simple git squash before we merge?

Can't you squash when you merge the PR ? (in gitlab they allow you to make it like a single commit)
But yes i can squash :wink: 
birktj: > Can't you squash when you merge the PR ? (in gitlab they allow you to make it like a single commit)

It seems you are right. Learn something new every day

Patch:
diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs
index 22e39d3..df9a2a5 100644
--- a/src/encoder/mod.rs
+++ b/src/encoder/mod.rs
@@ -407,26 +407,9 @@ impl<W: Write + Seek> TiffEncoder<W> {
     where
         [C::Inner]: TiffValue,
     {
-        let num_pix = usize::try_from(width)?.checked_mul(usize::try_from(height)?)
-            .ok_or_else(|| ::std::io::Error::new(
-                ::std::io::ErrorKind::InvalidInput,
-                "Image width * height exceeds usize"))?;
-        if data.len() < num_pix {
-            return Err(::std::io::Error::new(
-                ::std::io::ErrorKind::InvalidData,
-                "Input data slice is undersized for provided dimensions").into());
-        }
-
         let encoder = DirectoryEncoder::new(&mut self.writer)?;
-        let mut image: ImageEncoder<W, C> = ImageEncoder::new(encoder, width, height)?;
-
-        let mut idx = 0;
-        while image.next_strip_sample_count() > 0 {
-            let sample_count = usize::try_from(image.next_strip_sample_count())?;
-            image.write_strip(&data[idx..idx + sample_count])?;
-            idx += sample_count;
-        }
-        image.finish()
+        let image: ImageEncoder<W, C> = ImageEncoder::new(encoder, width, height)?;
+        image.write_data(data)
     }
 }
 
@@ -548,10 +531,14 @@ impl<'a, W: Write + Seek> Drop for DirectoryEncoder<'a, W> {
 /// # let mut file = std::io::Cursor::new(Vec::new());
 /// # let image_data = vec![0; 100*100*3];
 /// use tiff::encoder::*;
+/// use tiff::tags::Tag;
 ///
 /// let mut tiff = TiffEncoder::new(&mut file).unwrap();
 /// let mut image = tiff.new_image::<colortype::RGB8>(100, 100).unwrap();
 ///
+/// // You can encode tags here 
+/// image.encoder().write_tag(Tag::Artist, "Image-tiff").unwrap();
+///
 /// let mut idx = 0;
 /// while image.next_strip_sample_count() > 0 {
 ///     let sample_count = image.next_strip_sample_count() as usize;
@@ -561,11 +548,13 @@ impl<'a, W: Write + Seek> Drop for DirectoryEncoder<'a, W> {
 /// image.finish().unwrap();
 /// # }
 /// ```
+/// You can also call write_data function wich will encode by strip and finish
 pub struct ImageEncoder<'a, W: 'a + Write + Seek, C: ColorType> {
     encoder: DirectoryEncoder<'a, W>,
     strip_idx: u64,
     strip_count: u64,
     row_samples: u64,
+    width: u32,
     height: u32,
     rows_per_strip: u64,
     strip_offsets: Vec<u32>,
@@ -608,6 +597,7 @@ impl<'a, W: 'a + Write + Seek, T: ColorType> ImageEncoder<'a, W, T> {
             strip_idx: 0,
             row_samples,
             rows_per_strip,
+            width,
             height,
             strip_offsets: Vec::new(),
             strip_byte_count: Vec::new(),
@@ -652,6 +642,30 @@ impl<'a, W: 'a + Write + Seek, T: ColorType> ImageEncoder<'a, W, T> {
         Ok(())
     }
 
+    /// Write strips from data
+    pub fn write_data(mut self, data: &[T::Inner]) -> TiffResult<()>
+    where
+        [T::Inner]: TiffValue,
+    {
+        let num_pix = usize::try_from(self.width)?.checked_mul(usize::try_from(self.height)?)
+            .ok_or_else(|| ::std::io::Error::new(
+                ::std::io::ErrorKind::InvalidInput,
+                "Image width * height exceeds usize"))?;
+        if data.len() < num_pix {
+            return Err(::std::io::Error::new(
+                ::std::io::ErrorKind::InvalidData,
+                "Input data slice is undersized for provided dimensions").into());
+        }
+        let mut idx = 0;
+        while self.next_strip_sample_count() > 0 {
+            let sample_count = usize::try_from(self.next_strip_sample_count())?;
+            self.write_strip(&data[idx..idx + sample_count])?;
+            idx += sample_count;
+        }
+        self.finish()?;
+        Ok(())
+    }
+
     /// Set image resolution
     pub fn resolution(&mut self, unit: ResolutionUnit, value: Rational) {
         self.encoder.write_tag(Tag::ResolutionUnit, unit.to_u16()).unwrap();


Test patch:
diff --git a/tests/encode_images.rs b/tests/encode_images.rs
index 9d1ecfd..9ba1cd3 100644
--- a/tests/encode_images.rs
+++ b/tests/encode_images.rs
@@ -1,6 +1,6 @@
 extern crate tiff;
 
-use tiff::decoder::{Decoder, DecodingResult};
+use tiff::decoder::{Decoder, DecodingResult, ifd};
 use tiff::encoder::{colortype, TiffEncoder, SRational};
 use tiff::ColorType;
 use tiff::tags::Tag;
@@ -24,14 +24,18 @@ fn encode_decode() {
     {
         let mut tiff = TiffEncoder::new(&mut file).unwrap();
 
-        tiff.write_image::<colortype::RGB8>(100, 100, &image_data)
+        let mut image = tiff.new_image::<colortype::RGB8>(100, 100)
             .unwrap();
+        image.encoder().write_tag(Tag::Artist, "Image-tiff").unwrap();
+        image.write_data(&image_data).unwrap();
+
     }
     {
         file.seek(SeekFrom::Start(0)).unwrap();
         let mut decoder = Decoder::new(&mut file).unwrap();
         assert_eq!(decoder.colortype().unwrap(), ColorType::RGB(8));
         assert_eq!(decoder.dimensions().unwrap(), (100, 100));
+        assert_eq!(decoder.get_tag(Tag::Artist).unwrap(), ifd::Value::Ascii("Image-tiff".into()));
         if let DecodingResult::U8(img_res) = decoder.read_image().unwrap() {
             assert_eq!(image_data, img_res);
         } else {
@@ -259,4 +263,4 @@ fn test_multipage_image() {
         // check the dimensions of the image in the second page
         assert_eq!(img_decoder.dimensions().unwrap(), (3, 3));
     }
-}
+}
\ No newline at end of file